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
diff --git a/core/java/android/provider/Settings.java b/core/java/android/provider/Settings.java
index 045ba1f..07d265b 100644
--- a/core/java/android/provider/Settings.java
+++ b/core/java/android/provider/Settings.java
@@ -3021,7 +3021,11 @@
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 @@
@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 @@
+ " 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 @@
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 @@
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 b6fc137..54a3817 100644
--- a/core/tests/coretests/src/android/provider/NameValueCacheTest.java
+++ b/core/tests/coretests/src/android/provider/NameValueCacheTest.java
@@ -28,6 +28,7 @@
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 @@
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 @@
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 @@
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 @@
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 void cleanUp() throws IOException {
mConfigsStorage.clear();
mSettingsStorage.clear();
+ mSettingsCacheGenerationStore.close();
+ mConfigsCacheGenerationStore.close();
}
@Test
@@ -361,16 +386,38 @@
}
@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 7f3b0ff..db6cc1a 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 @@
// 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 @@
(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 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 @@
}
}
+ // 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 @@
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 @@
}
}
+ 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 ba275eb..27c8cdf 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 @@
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 @@
} 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 @@
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 @@
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 @@
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 c388826..4d8705f 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 @@
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 d34fe694..6ec8146 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 @@
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);
