Fix a race between requesting/loading/writing SharedPreferences.
Part of the race existed before, but it was made much worse with
apply().
Corresponding CTS test is Icc6e638a6a
Change-Id: Ic5cfa467fb7f1859cb7b44d417412219c0621965
diff --git a/core/java/android/app/ContextImpl.java b/core/java/android/app/ContextImpl.java
index 879670e..09ef710 100644
--- a/core/java/android/app/ContextImpl.java
+++ b/core/java/android/app/ContextImpl.java
@@ -325,7 +325,7 @@
}
throw new RuntimeException("Not supported in system context");
}
-
+
private static File makeBackupFile(File prefsFile) {
return new File(prefsFile.getPath() + ".bak");
}
@@ -337,55 +337,54 @@
@Override
public SharedPreferences getSharedPreferences(String name, int mode) {
SharedPreferencesImpl sp;
+ File prefsFile;
+ boolean needInitialLoad = false;
synchronized (sSharedPrefs) {
sp = sSharedPrefs.get(name);
- if (sp != null && !sp.hasFileChanged()) {
- //Log.i(TAG, "Returning existing prefs " + name + ": " + sp);
+ if (sp != null && !sp.hasFileChangedUnexpectedly()) {
return sp;
}
- }
- File f = getSharedPrefsFile(name);
-
- FileInputStream str = null;
- File backup = makeBackupFile(f);
- if (backup.exists()) {
- f.delete();
- backup.renameTo(f);
- }
-
- // Debugging
- if (f.exists() && !f.canRead()) {
- Log.w(TAG, "Attempt to read preferences file " + f + " without permission");
- }
-
- Map map = null;
- if (f.exists() && f.canRead()) {
- try {
- str = new FileInputStream(f);
- map = XmlUtils.readMapXml(str);
- str.close();
- } catch (org.xmlpull.v1.XmlPullParserException e) {
- Log.w(TAG, "getSharedPreferences", e);
- } catch (FileNotFoundException e) {
- Log.w(TAG, "getSharedPreferences", e);
- } catch (IOException e) {
- Log.w(TAG, "getSharedPreferences", e);
+ prefsFile = getSharedPrefsFile(name);
+ if (sp == null) {
+ sp = new SharedPreferencesImpl(prefsFile, mode, null);
+ sSharedPrefs.put(name, sp);
+ needInitialLoad = true;
}
}
- synchronized (sSharedPrefs) {
- if (sp != null) {
- //Log.i(TAG, "Updating existing prefs " + name + " " + sp + ": " + map);
- sp.replace(map);
- } else {
- sp = sSharedPrefs.get(name);
- if (sp == null) {
- sp = new SharedPreferencesImpl(f, mode, map);
- sSharedPrefs.put(name, sp);
+ synchronized (sp) {
+ if (needInitialLoad && sp.isLoaded()) {
+ // lost the race to load; another thread handled it
+ return sp;
+ }
+ File backup = makeBackupFile(prefsFile);
+ if (backup.exists()) {
+ prefsFile.delete();
+ backup.renameTo(prefsFile);
+ }
+
+ // Debugging
+ if (prefsFile.exists() && !prefsFile.canRead()) {
+ Log.w(TAG, "Attempt to read preferences file " + prefsFile + " without permission");
+ }
+
+ Map map = null;
+ if (prefsFile.exists() && prefsFile.canRead()) {
+ try {
+ FileInputStream str = new FileInputStream(prefsFile);
+ map = XmlUtils.readMapXml(str);
+ str.close();
+ } catch (org.xmlpull.v1.XmlPullParserException e) {
+ Log.w(TAG, "getSharedPreferences", e);
+ } catch (FileNotFoundException e) {
+ Log.w(TAG, "getSharedPreferences", e);
+ } catch (IOException e) {
+ Log.w(TAG, "getSharedPreferences", e);
}
}
- return sp;
+ sp.replace(map);
}
+ return sp;
}
private File getPreferencesDir() {
@@ -2712,6 +2711,10 @@
private static final class SharedPreferencesImpl implements SharedPreferences {
+ // Lock ordering rules:
+ // - acquire SharedPreferencesImpl.this before EditorImpl.this
+ // - acquire mWritingToDiskLock before EditorImpl.this
+
private final File mFile;
private final File mBackupFile;
private final int mMode;
@@ -2719,6 +2722,7 @@
private Map<String, Object> mMap; // guarded by 'this'
private long mTimestamp; // guarded by 'this'
private int mDiskWritesInFlight = 0; // guarded by 'this'
+ private boolean mLoaded = false; // guarded by 'this'
private final Object mWritingToDiskLock = new Object();
private static final Object mContent = new Object();
@@ -2729,6 +2733,7 @@
mFile = file;
mBackupFile = makeBackupFile(file);
mMode = mode;
+ mLoaded = initialContents != null;
mMap = initialContents != null ? initialContents : new HashMap<String, Object>();
FileStatus stat = new FileStatus();
if (FileUtils.getFileStatus(file.getPath(), stat)) {
@@ -2737,7 +2742,23 @@
mListeners = new WeakHashMap<OnSharedPreferenceChangeListener, Object>();
}
- public boolean hasFileChanged() {
+ // Has this SharedPreferences ever had values assigned to it?
+ boolean isLoaded() {
+ synchronized (this) {
+ return mLoaded;
+ }
+ }
+
+ // Has the file changed out from under us? i.e. writes that
+ // we didn't instigate.
+ public boolean hasFileChangedUnexpectedly() {
+ synchronized (this) {
+ if (mDiskWritesInFlight > 0) {
+ // If we know we caused it, it's not unexpected.
+ Log.d(TAG, "disk write in flight, not unexpected.");
+ return false;
+ }
+ }
FileStatus stat = new FileStatus();
if (!FileUtils.getFileStatus(mFile.getPath(), stat)) {
return true;
@@ -2748,8 +2769,9 @@
}
public void replace(Map newContents) {
- if (newContents != null) {
- synchronized (this) {
+ synchronized (this) {
+ mLoaded = true;
+ if (newContents != null) {
mMap = newContents;
}
}
