From 8ad9587673591e92ae306a87cec5bb7728064686 Mon Sep 17 00:00:00 2001 From: Song Chun Fan Date: Thu, 9 Mar 2023 02:14:39 +0000 Subject: [PATCH] Revert "Revert "[SettingsProvider] tracking generation of non-pr..." Revert submission 21910984 Reason for revert: Reland ag/21729397 with changes as follows * Always fetch a new generation tracker if the generation number changes * Also merged ag/21876270 (minor improvement) with this CL Reverted changes: /q/submissionid:21910984 BUG: 228619157 Test: atest android.provider.NameValueCacheTest Test: atest CtsNetTestCasesLatestSdk:android.net.cts.ConnectivityManagerTest#testUidsAllowedOnRestrictedNetworks Test: atest android.widget.TextViewPrecomputedTextPerfTest#testOnMeasure_RandomText Change-Id: Ib0c8c4e25dfa5cb0b01559e60bd95d2ea668decc --- core/java/android/provider/Settings.java | 33 ++++++---- .../android/provider/NameValueCacheTest.java | 65 ++++++++++++++++--- .../settings/GenerationRegistry.java | 32 ++++++++- .../providers/settings/SettingsProvider.java | 32 ++++++--- .../providers/settings/SettingsState.java | 6 ++ .../settings/GenerationRegistryTest.java | 20 ++++++ 6 files changed, 153 insertions(+), 35 deletions(-) diff --git a/core/java/android/provider/Settings.java b/core/java/android/provider/Settings.java index 045ba1f82b7ca..07d265b43ab4d 100644 --- a/core/java/android/provider/Settings.java +++ b/core/java/android/provider/Settings.java @@ -3021,7 +3021,11 @@ public final class Settings { public void destroy() { try { - mArray.close(); + // If this process is the system server process, mArray is the same object as + // the memory int array kept inside SetingsProvider, so skipping the close() + if (!Settings.isInSystemServer()) { + mArray.close(); + } } catch (IOException e) { Log.e(TAG, "Error closing backing array", e); } @@ -3190,9 +3194,7 @@ public final class Settings { @UnsupportedAppUsage public String getStringForUser(ContentResolver cr, String name, final int userHandle) { final boolean isSelf = (userHandle == UserHandle.myUserId()); - int currentGeneration = -1; boolean needsGenerationTracker = false; - if (isSelf) { synchronized (NameValueCache.this) { final GenerationTracker generationTracker = mGenerationTrackers.get(name); @@ -3204,27 +3206,31 @@ public final class Settings { + " in package:" + cr.getPackageName() + " and user:" + userHandle); } + // When a generation number changes, remove cached value, remove the old + // generation tracker and request a new one mValues.remove(name); + generationTracker.destroy(); + mGenerationTrackers.remove(name); } else if (mValues.containsKey(name)) { if (DEBUG) { Log.i(TAG, "Cache hit for setting:" + name); } return mValues.get(name); } - currentGeneration = generationTracker.getCurrentGeneration(); - } else { - needsGenerationTracker = true; } } + if (DEBUG) { + Log.i(TAG, "Cache miss for setting:" + name + " for user:" + + userHandle); + } + // Generation tracker doesn't exist or the value isn't cached + needsGenerationTracker = true; } else { if (DEBUG || LOCAL_LOGV) { Log.v(TAG, "get setting for user " + userHandle + " by user " + UserHandle.myUserId() + " so skipping cache"); } } - if (DEBUG) { - Log.i(TAG, "Cache miss for setting:" + name + " for user:" + userHandle); - } // Check if the target settings key is readable. Reject if the caller is not system and // is trying to access a settings key defined in the Settings.Secure, Settings.System or @@ -3324,11 +3330,10 @@ public final class Settings { mGenerationTrackers.put(name, new GenerationTracker(name, array, index, generation, mGenerationTrackerErrorHandler)); - currentGeneration = generation; } } - if (mGenerationTrackers.get(name) != null && currentGeneration - == mGenerationTrackers.get(name).getCurrentGeneration()) { + if (mGenerationTrackers.get(name) != null + && !mGenerationTrackers.get(name).isGenerationChanged()) { if (DEBUG) { Log.i(TAG, "Updating cache for setting:" + name); } @@ -3374,8 +3379,8 @@ public final class Settings { String value = c.moveToNext() ? c.getString(0) : null; synchronized (NameValueCache.this) { - if (mGenerationTrackers.get(name) != null && currentGeneration - == mGenerationTrackers.get(name).getCurrentGeneration()) { + if (mGenerationTrackers.get(name) != null + && !mGenerationTrackers.get(name).isGenerationChanged()) { if (DEBUG) { Log.i(TAG, "Updating cache for setting:" + name + " using query"); } diff --git a/core/tests/coretests/src/android/provider/NameValueCacheTest.java b/core/tests/coretests/src/android/provider/NameValueCacheTest.java index b6fc137471a42..54a38178e7cee 100644 --- a/core/tests/coretests/src/android/provider/NameValueCacheTest.java +++ b/core/tests/coretests/src/android/provider/NameValueCacheTest.java @@ -28,6 +28,7 @@ import static org.mockito.Mockito.when; import android.content.ContentProvider; import android.content.IContentProvider; import android.os.Bundle; +import android.os.Parcel; import android.platform.test.annotations.Presubmit; import android.test.mock.MockContentResolver; import android.util.MemoryIntArray; @@ -91,7 +92,7 @@ public class NameValueCacheTest { mConfigsCacheGenerationStore = new MemoryIntArray(2); mConfigsCacheGenerationStore.set(0, 123); mConfigsCacheGenerationStore.set(1, 456); - mSettingsCacheGenerationStore = new MemoryIntArray(2); + mSettingsCacheGenerationStore = new MemoryIntArray(3); mSettingsCacheGenerationStore.set(0, 234); mSettingsCacheGenerationStore.set(1, 567); mConfigsStorage = new HashMap<>(); @@ -163,6 +164,10 @@ public class NameValueCacheTest { Bundle incomingBundle = invocationOnMock.getArgument(4); String key = invocationOnMock.getArgument(3); String value = incomingBundle.getString(Settings.NameValueTable.VALUE); + boolean newSetting = false; + if (!mSettingsStorage.containsKey(key)) { + newSetting = true; + } mSettingsStorage.put(key, value); int currentGeneration; // Different settings have different generation codes @@ -173,12 +178,18 @@ public class NameValueCacheTest { currentGeneration = mSettingsCacheGenerationStore.get(1); mSettingsCacheGenerationStore.set(1, ++currentGeneration); } + if (newSetting) { + // Tracking the generation of all unset settings. + // Increment when a new setting is inserted + currentGeneration = mSettingsCacheGenerationStore.get(2); + mSettingsCacheGenerationStore.set(2, ++currentGeneration); + } return null; }); // Returns the value corresponding to a setting, or null if the setting - // doesn't have a value stored for it. Returns the generation key if the value isn't null - // and the caller asked for the generation key. + // doesn't have a value stored for it. Returns the generation key + // if the caller asked for the generation key. when(mMockIContentProvider.call(any(), eq(Settings.Secure.CONTENT_URI.getAuthority()), eq(Settings.CALL_METHOD_GET_SECURE), any(), any(Bundle.class))).thenAnswer( invocationOnMock -> { @@ -189,14 +200,26 @@ public class NameValueCacheTest { Bundle bundle = new Bundle(); bundle.putSerializable(Settings.NameValueTable.VALUE, value); - if (value != null && incomingBundle.containsKey( + if (incomingBundle.containsKey( Settings.CALL_METHOD_TRACK_GENERATION_KEY)) { - int index = key.equals(SETTING) ? 0 : 1; + int index; + if (value != null) { + index = key.equals(SETTING) ? 0 : 1; + } else { + // special index for unset settings + index = 2; + } + // Manually make a copy of the memory int array to mimic sending it over IPC + Parcel p = Parcel.obtain(); + mSettingsCacheGenerationStore.writeToParcel(p, 0); + p.setDataPosition(0); + MemoryIntArray parcelArray = MemoryIntArray.CREATOR.createFromParcel(p); bundle.putParcelable(Settings.CALL_METHOD_TRACK_GENERATION_KEY, - mSettingsCacheGenerationStore); + parcelArray); bundle.putInt(Settings.CALL_METHOD_GENERATION_INDEX_KEY, index); bundle.putInt(Settings.CALL_METHOD_GENERATION_KEY, mSettingsCacheGenerationStore.get(index)); + p.recycle(); } return bundle; }); @@ -206,6 +229,8 @@ public class NameValueCacheTest { public void cleanUp() throws IOException { mConfigsStorage.clear(); mSettingsStorage.clear(); + mSettingsCacheGenerationStore.close(); + mConfigsCacheGenerationStore.close(); } @Test @@ -361,16 +386,38 @@ public class NameValueCacheTest { } @Test - public void testCaching_nullSetting() throws Exception { + public void testCaching_unsetSetting() throws Exception { String returnedValue = Settings.Secure.getString(mMockContentResolver, SETTING); verify(mMockIContentProvider, times(1)).call(any(), any(), eq(Settings.CALL_METHOD_GET_SECURE), any(), any(Bundle.class)); assertThat(returnedValue).isNull(); String cachedValue = Settings.Secure.getString(mMockContentResolver, SETTING); - // Empty list won't be cached + // The first unset setting's generation number is cached + verifyNoMoreInteractions(mMockIContentProvider); + assertThat(cachedValue).isNull(); + + String returnedValue2 = Settings.Secure.getString(mMockContentResolver, SETTING2); verify(mMockIContentProvider, times(2)).call(any(), any(), eq(Settings.CALL_METHOD_GET_SECURE), any(), any(Bundle.class)); - assertThat(cachedValue).isNull(); + assertThat(returnedValue2).isNull(); + + String cachedValue2 = Settings.Secure.getString(mMockContentResolver, SETTING); + // The second unset setting's generation number is cached + verifyNoMoreInteractions(mMockIContentProvider); + assertThat(cachedValue2).isNull(); + + Settings.Secure.putString(mMockContentResolver, SETTING, "a"); + // The generation for unset settings should have changed + returnedValue2 = Settings.Secure.getString(mMockContentResolver, SETTING2); + verify(mMockIContentProvider, times(3)).call(any(), any(), + eq(Settings.CALL_METHOD_GET_SECURE), any(), any(Bundle.class)); + assertThat(returnedValue2).isNull(); + + // The generation tracker for the first setting should have change because it's set now + returnedValue = Settings.Secure.getString(mMockContentResolver, SETTING); + verify(mMockIContentProvider, times(4)).call(any(), any(), + eq(Settings.CALL_METHOD_GET_SECURE), any(), any(Bundle.class)); + assertThat(returnedValue).isEqualTo("a"); } } diff --git a/packages/SettingsProvider/src/com/android/providers/settings/GenerationRegistry.java b/packages/SettingsProvider/src/com/android/providers/settings/GenerationRegistry.java index 7f3b0ff8c8386..db6cc1a39ef12 100644 --- a/packages/SettingsProvider/src/com/android/providers/settings/GenerationRegistry.java +++ b/packages/SettingsProvider/src/com/android/providers/settings/GenerationRegistry.java @@ -16,6 +16,7 @@ package com.android.providers.settings; +import android.annotation.NonNull; import android.os.Bundle; import android.provider.Settings; import android.util.ArrayMap; @@ -59,6 +60,10 @@ final class GenerationRegistry { // Maximum size of an individual backing store static final int MAX_BACKING_STORE_SIZE = MemoryIntArray.getMaxSize(); + // Use an empty string to track the generation number of all non-predefined, unset settings + // The generation number is only increased when a new non-predefined setting is inserted + private static final String DEFAULT_MAP_KEY_FOR_UNSET_SETTINGS = ""; + public GenerationRegistry(Object lock) { mLock = lock; } @@ -72,6 +77,10 @@ final class GenerationRegistry { (SettingsState.getTypeFromKey(key) == SettingsState.SETTINGS_TYPE_CONFIG); // Only store the prefix if the mutated setting is a config final String indexMapKey = isConfig ? (name.split("/")[0] + "/") : name; + incrementGenerationInternal(key, indexMapKey); + } + + private void incrementGenerationInternal(int key, @NonNull String indexMapKey) { synchronized (mLock) { final MemoryIntArray backingStore = getBackingStoreLocked(key, /* createIfNotExist= */ false); @@ -87,7 +96,8 @@ final class GenerationRegistry { final int generation = backingStore.get(index) + 1; backingStore.set(index, generation); if (DEBUG) { - Slog.i(LOG_TAG, "Incremented generation for setting:" + indexMapKey + Slog.i(LOG_TAG, "Incremented generation for " + + (indexMapKey.isEmpty() ? "unset settings" : "setting:" + indexMapKey) + " key:" + SettingsState.keyToString(key) + " at index:" + index); } @@ -98,6 +108,18 @@ final class GenerationRegistry { } } + // A new, non-predefined setting has been inserted, increment the tracking number for all unset + // settings + public void incrementGenerationForUnsetSettings(int key) { + final boolean isConfig = + (SettingsState.getTypeFromKey(key) == SettingsState.SETTINGS_TYPE_CONFIG); + if (isConfig) { + // No need to track new settings for configs + return; + } + incrementGenerationInternal(key, DEFAULT_MAP_KEY_FOR_UNSET_SETTINGS); + } + /** * Return the backing store's reference, the index and the current generation number * of a cached setting. If it was not in the backing store, first create the entry in it before @@ -124,8 +146,8 @@ final class GenerationRegistry { bundle.putInt(Settings.CALL_METHOD_GENERATION_KEY, backingStore.get(index)); if (DEBUG) { - Slog.i(LOG_TAG, "Exported index:" + index - + " for setting:" + indexMapKey + Slog.i(LOG_TAG, "Exported index:" + index + " for " + + (indexMapKey.isEmpty() ? "unset settings" : "setting:" + indexMapKey) + " key:" + SettingsState.keyToString(key)); } } catch (IOException e) { @@ -135,6 +157,10 @@ final class GenerationRegistry { } } + public void addGenerationDataForUnsetSettings(Bundle bundle, int key) { + addGenerationData(bundle, key, /* indexMapKey= */ DEFAULT_MAP_KEY_FOR_UNSET_SETTINGS); + } + public void onUserRemoved(int userId) { final int secureKey = SettingsState.makeKey( SettingsState.SETTINGS_TYPE_SECURE, userId); diff --git a/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java b/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java index ba275ebca1682..27c8cdfe98f33 100644 --- a/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java +++ b/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java @@ -2327,11 +2327,15 @@ public class SettingsProvider extends ContentProvider { result.putString(Settings.NameValueTable.VALUE, (setting != null && !setting.isNull()) ? setting.getValue() : null); - if ((setting != null && !setting.isNull()) || isSettingPreDefined(name, type)) { - // Don't track generation for non-existent settings unless the name is predefined - synchronized (mLock) { + synchronized (mLock) { + if ((setting != null && !setting.isNull()) || isSettingPreDefined(name, type)) { + // Individual generation tracking for predefined settings even if they are unset mSettingsRegistry.mGenerationRegistry.addGenerationData(result, SettingsState.makeKey(type, userId), name); + } else { + // All non-predefined, unset settings are tracked using the same generation number + mSettingsRegistry.mGenerationRegistry.addGenerationDataForUnsetSettings(result, + SettingsState.makeKey(type, userId)); } } return result; @@ -2345,7 +2349,8 @@ public class SettingsProvider extends ContentProvider { } else if (type == SETTINGS_TYPE_SYSTEM) { return sAllSystemSettings.contains(name); } else { - return false; + // Consider all config settings predefined because they are used by system apps only + return type == SETTINGS_TYPE_CONFIG; } } @@ -2354,14 +2359,13 @@ public class SettingsProvider extends ContentProvider { Bundle result = new Bundle(); result.putSerializable(Settings.NameValueTable.VALUE, keyValues); if (trackingGeneration) { - // Track generation even if the namespace is empty because this is for system apps synchronized (mLock) { + // Track generation even if namespace is empty because this is for system apps only mSettingsRegistry.mGenerationRegistry.addGenerationData(result, - mSettingsRegistry.getSettingsLocked(SETTINGS_TYPE_CONFIG, - UserHandle.USER_SYSTEM).mKey, prefix); + SettingsState.makeKey(SETTINGS_TYPE_CONFIG, UserHandle.USER_SYSTEM), + prefix); } } - return result; } @@ -3052,10 +3056,15 @@ public class SettingsProvider extends ContentProvider { final int key = makeKey(type, userId); boolean success = false; + boolean wasUnsetNonPredefinedSetting = false; SettingsState settingsState = peekSettingsStateLocked(key); if (settingsState != null) { + if (!isSettingPreDefined(name, type) && !settingsState.hasSetting(name)) { + wasUnsetNonPredefinedSetting = true; + } success = settingsState.insertSettingLocked(name, value, - tag, makeDefault, forceNonSystemPackage, packageName, overrideableByRestore); + tag, makeDefault, forceNonSystemPackage, packageName, + overrideableByRestore); } if (success && criticalSettings != null && criticalSettings.contains(name)) { @@ -3064,6 +3073,11 @@ public class SettingsProvider extends ContentProvider { if (forceNotify || success) { notifyForSettingsChange(key, name); + if (wasUnsetNonPredefinedSetting) { + // Increment the generation number for all non-predefined, unset settings, + // because a new non-predefined setting has been inserted + mGenerationRegistry.incrementGenerationForUnsetSettings(key); + } } if (success) { logSettingChanged(userId, name, type, CHANGE_TYPE_INSERT); diff --git a/packages/SettingsProvider/src/com/android/providers/settings/SettingsState.java b/packages/SettingsProvider/src/com/android/providers/settings/SettingsState.java index c3888268a61db..4d8705f135af3 100644 --- a/packages/SettingsProvider/src/com/android/providers/settings/SettingsState.java +++ b/packages/SettingsProvider/src/com/android/providers/settings/SettingsState.java @@ -759,6 +759,12 @@ final class SettingsState { mPackageToMemoryUsage.put(packageName, newSize); } + public boolean hasSetting(String name) { + synchronized (mLock) { + return hasSettingLocked(name); + } + } + @GuardedBy("mLock") private boolean hasSettingLocked(String name) { return mSettings.indexOfKey(name) >= 0; diff --git a/packages/SettingsProvider/test/src/com/android/providers/settings/GenerationRegistryTest.java b/packages/SettingsProvider/test/src/com/android/providers/settings/GenerationRegistryTest.java index d34fe6943153c..6ec8146baee0b 100644 --- a/packages/SettingsProvider/test/src/com/android/providers/settings/GenerationRegistryTest.java +++ b/packages/SettingsProvider/test/src/com/android/providers/settings/GenerationRegistryTest.java @@ -151,6 +151,26 @@ public class GenerationRegistryTest { checkBundle(b, 0, 1, false); } + @Test + public void testUnsetSettings() throws IOException { + final GenerationRegistry generationRegistry = new GenerationRegistry(new Object()); + final int secureKey = SettingsState.makeKey(SettingsState.SETTINGS_TYPE_SECURE, 0); + final String testSecureSetting = "test_secure_setting"; + Bundle b = new Bundle(); + generationRegistry.addGenerationData(b, secureKey, testSecureSetting); + checkBundle(b, 0, 1, false); + generationRegistry.addGenerationDataForUnsetSettings(b, secureKey); + checkBundle(b, 1, 1, false); + generationRegistry.addGenerationDataForUnsetSettings(b, secureKey); + // Test that unset settings always have the same index + checkBundle(b, 1, 1, false); + generationRegistry.incrementGenerationForUnsetSettings(secureKey); + // Test that the generation number of the unset settings have increased + generationRegistry.addGenerationDataForUnsetSettings(b, secureKey); + checkBundle(b, 1, 2, false); + } + + private void checkBundle(Bundle b, int expectedIndex, int expectedGeneration, boolean isNull) throws IOException { final MemoryIntArray array = getArray(b);