diff --git a/core/tests/coretests/src/android/provider/NameValueCacheTest.java b/core/tests/coretests/src/android/provider/NameValueCacheTest.java index b6fc137471a42..ccf8085f87ff9 100644 --- a/core/tests/coretests/src/android/provider/NameValueCacheTest.java +++ b/core/tests/coretests/src/android/provider/NameValueCacheTest.java @@ -91,7 +91,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 +163,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 +177,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,9 +199,15 @@ 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; + } bundle.putParcelable(Settings.CALL_METHOD_TRACK_GENERATION_KEY, mSettingsCacheGenerationStore); bundle.putInt(Settings.CALL_METHOD_GENERATION_INDEX_KEY, index); @@ -361,16 +377,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..418011acf6f37 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 isNewSetting = false; SettingsState settingsState = peekSettingsStateLocked(key); if (settingsState != null) { + if (!settingsState.hasSetting(name)) { + isNewSetting = 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 (isNewSetting && !isSettingPreDefined(name, type)) { + // Increment the generation number for all null 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);