From bug 862805 comment 5: Why do we have two completely different sets of SharedPreferences? mobile/android/base/GeckoApp.java 246: return GeckoApp.sAppContext.getSharedPreferences(GeckoApp.PREFS_NAME, 0); 1716: return PreferenceManager.getDefaultSharedPreferences(this) IMO we should eliminate all uses of PreferenceManager: http://stackoverflow.com/questions/18204708/preferencemanager-getdefaultsharedpreferences-vs-getpreferences cc'ing the folks who have the hg blame for the places where we use PreferenceManager, in case they can explain this different usage.

> cc'ing the folks who have the hg blame for the places where we use > PreferenceManager, in case they can explain this different usage. I have no preference, so your preference is good for me. I remember being surprised at this, shaking my head (for the millionth time) at Android, and deciding that migrating was more effort than it's worth.

Notably this'll require a migration step if we're keeping anything useful in PreferenceManager. On the plus side, this should save us IO, not just neatness. (Thanks for filing this, Margaret!)

We'll need to do pref migration anyway to fix per-profile preferences, so we should make sure this gets fixed with it. I'm not sure if per-profile prefs are actually filed -- bug 917986 is the closest I can find. Maybe we can morph this bug to take care of both?

Thumbs up from me. (Extra points if it doesn't bitrot my locale work :P)

(In reply to Brian Nicholson (:bnicholson) from comment #3) > We'll need to do pref migration anyway to fix per-profile preferences, so we > should make sure this gets fixed with it. I'm not sure if per-profile prefs > are actually filed -- bug 917986 is the closest I can find. Maybe we can > morph this bug to take care of both? Per-profile prefs will help mitigate our concerns in bug 862805, so +1 from me.

Morphing this. N.B., there are settings that are per-app, and settings that are per-process. Now's the time to straighten that out!

Comment on attachment 8384615 [details] [diff] [review] Implement scopes for SharedPreferences (r=rnewman) Here's a first take on this. Overview: - Adds a little infrastructure that allow us to trigger shared prefs migrations to move keys around whenever we need to. For now, the first migration will simply move all keys currently in PreferenceManager to the APP scope, except for two Hub-related keys which are per-profile. We can move more keys to be per-profile by triggering new migrations in the future. - Replaces all PreferenceManager.getDefaultSharedPreferences() calls with the new GeckoSharedPrefs API using the appropriate scopes. - I decided to go simple on the thread-safety and just do all the migration inside a synchronized method. I don't expect get() and getForProfileName() to be called very frequently but I added some fast paths for subsequent calls to avoid excessive retention. - Adds a new browser JUnit3 test that covers the migration bits of GeckoSharedPrefs. I wasn't able to actually run the test locally. Need to poke Nick about it. This is the only reason why I haven't requested review yet.

Comment on attachment 8384615 [details] [diff] [review] Implement scopes for SharedPreferences (r=rnewman) Review of attachment 8384615 [details] [diff] [review]: ----------------------------------------------------------------- This is looking pretty reasonable. I would definitely prefer to land without GeckoSharedPrefs holding a static context. You'll need to bump mobile/android/tests/browser/junit3/moz.build to include your new test, but I'll CC you on a ticket to make running said tests easier. ::: mobile/android/base/GeckoSharedPrefs.java @@ +13,5 @@ > +import java.util.List; > +import java.util.Map; > +import java.util.Set; > + > +public final class GeckoSharedPrefs { Class comment always appreciated. And I see our preferred style for comments is different :( Oh well. @@ +15,5 @@ > +import java.util.Set; > + > +public final class GeckoSharedPrefs { > + // Increment it to trigger a new prefs migration > + private static final int CURRENT_MIGRATION = 1; I find migration confusing; does this mean "version"? Like, CURRENT_PREFS_VERSION? @@ +29,5 @@ > + "home_panels", > + "home_locale" > + }; > + > + // Context to get SharedPreferences instances Yeah, let's not save a static context. Why is this migrator static anyway? Why not have it be an object and take a context in its constructor, or better yet, take a context in its members? (Note: I am not against having a static accessor to a single instance.) @@ +100,5 @@ > + switch (scope) { > + case APP: > + return scope.getPrefsName(); > + > + case PROFILE: { Are the parens Fennec style? They're not necessary, right? @@ +114,5 @@ > + private static String getBranchForProfileName(String profileName) { > + return Scope.PROFILE.getPrefsName() + "-" + profileName; > + } > + > + private static synchronized void migrateIfNecessary(Context context) { This context appears to be unused. @@ +144,5 @@ > + * Move all preferences stored in PreferenceManager's default prefs either to > + * APP or PROFILE scopes. The PROFILE scope keys are defined in PROFILE_MIGRATIONS_1. > + */ > + private static void performMigration1() { > + final SharedPreferences.Editor appEditor = Consider taking these |Editor| instances as parameters; in future we're going to chain these, right? And the caller should own the final |commit|. @@ +158,5 @@ > + final List<String> profileMigrations = Arrays.asList(PROFILE_MIGRATIONS_1); > + > + final SharedPreferences pmPrefs = PreferenceManager.getDefaultSharedPreferences(context); > + > + for (Map.Entry<String, ?> entry : pmPrefs.getAll().entrySet()) { This logic seems quite general, but is currently a little tied to PROFILE_MIGRATIONS_1. Can we break this out for testing and re-use in future migrations. @@ +190,5 @@ > + to.putFloat(key, (Float) value); > + } else if (value instanceof Integer) { > + to.putInt(key, (Integer) value); > + } else if (value instanceof Set) { > + to.putStringSet(key, (Set<String>) value); You want to do *something* (log? I prefer to throw checked) in a final else case. Also, I vaguely recall that string sets are not supported well in some versions of Android. Ah yes, no can do pre v11: http://developer.android.com/reference/android/content/SharedPreferences.Editor.html#putStringSet%28java.lang.String,%20java.util.Set%3Cjava.lang.String%3E%29 http://developer.android.com/reference/android/content/SharedPreferences.html#getStringSet%28java.lang.String,%20java.util.Set%3Cjava.lang.String%3E%29 ::: mobile/android/tests/browser/junit3/src/tests/TestGeckoSharedPrefs.java @@ +19,5 @@ > +/** > + * Test GeckoSharedPrefs migrations. > + */ > +public class TestGeckoSharedPrefs extends BrowserTestCase { > + public void testGeckoSharedPrefs() { You can have multiple |test*| methods; all will get run (in unspecified order). So if this is just testing migration, you might name it |testMigration| (or |testGeckoSharedPrefsMigration|). @@ +37,5 @@ > + assertTrue(pmPrefs.getAll().isEmpty()); > + > + final SharedPreferences.Editor pmEditor = pmPrefs.edit(); > + > + // migration path a bit more thoroughly nit: caps, full sentences (throughout). @@ +38,5 @@ > + > + final SharedPreferences.Editor pmEditor = pmPrefs.edit(); > + > + // migration path a bit more thoroughly > + // Insert a key for each type to exercise the the... @@ +59,5 @@ > + assertEquals(pmPrefs.getAll().size(), 7); > + > + // Trigger migration by getting prefs for a scope > + appPrefs = GeckoSharedPrefs.get(GeckoSharedPrefs.Scope.APP); > + assertEquals(pmPrefs.getAll().size(), 0); nit: in JUnit 3, expected comes first.

Comment on attachment 8384615 [details] [diff] [review] Implement scopes for SharedPreferences (r=rnewman) Review of attachment 8384615 [details] [diff] [review]: ----------------------------------------------------------------- Concur with all of nalexander's comments. ::: mobile/android/base/DataReportingNotification.java @@ -25,5 @@ > private static final String LOGTAG = "DataReportNotification"; > > public static final String ALERT_NAME_DATAREPORTING_NOTIFICATION = "datareporting-notification"; > > - private static final String DEFAULT_PREFS_BRANCH = AppConstants.ANDROID_PACKAGE_NAME + "_preferences"; You need to migrate this branch. ::: mobile/android/base/GeckoProfile.java @@ +146,5 @@ > } > > public static boolean removeProfile(Context context, String profileName) { > + // Clear all shared prefs for the given profile > + GeckoSharedPrefs.getForProfileName(profileName) Be prepared for the profile to not exist. ::: mobile/android/base/GeckoSharedPrefs.java @@ +89,5 @@ > + } > + > + public static SharedPreferences getForProfileName(String profileName, EnumSet<Flags> flags) { > + if (!flags.contains(Flags.DISABLE_MIGRATIONS)) { > + migrateIfNecessary(context); Surely the migration needs to know which profile's prefs are being migrated? They're different files. You're also going to need to know the default profile, or otherwise figure out the default destination, because the next step after this patch is to migrate a whole bunch of app-level prefs into per-profile prefs.

(In reply to Nick Alexander :nalexander from comment #9) > Comment on attachment 8384615 [details] [diff] [review] > Implement scopes for SharedPreferences (r=rnewman) > > Review of attachment 8384615 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is looking pretty reasonable. I would definitely prefer to land > without GeckoSharedPrefs holding a static context. Fixed, the context is an argument of each getter method now. > You'll need to bump mobile/android/tests/browser/junit3/moz.build to include > your new test, but I'll CC you on a ticket to make running said tests easier. Done. > ::: mobile/android/base/GeckoSharedPrefs.java > @@ +13,5 @@ > > +import java.util.List; > > +import java.util.Map; > > +import java.util.Set; > > + > > +public final class GeckoSharedPrefs { > > Class comment always appreciated. And I see our preferred style for > comments is different :( Oh well. Done. > @@ +15,5 @@ > > +import java.util.Set; > > + > > +public final class GeckoSharedPrefs { > > + // Increment it to trigger a new prefs migration > > + private static final int CURRENT_MIGRATION = 1; > > I find migration confusing; does this mean "version"? Like, > CURRENT_PREFS_VERSION? Renamed to PREFS_VERSION for clarity. > @@ +29,5 @@ > > + "home_panels", > > + "home_locale" > > + }; > > + > > + // Context to get SharedPreferences instances > > Yeah, let's not save a static context. Why is this migrator static anyway? > Why not have it be an object and take a context in its constructor, or > better yet, take a context in its members? (Note: I am not against having a > static accessor to a single instance.) The API is all static because it shouldn't really hold any state. This is meant to be just a thin wrapper around context.getSharedPreferences() to allow us to do scoping and migrations. The context is an argument of the getter methods now. > @@ +100,5 @@ > > + switch (scope) { > > + case APP: > > + return scope.getPrefsName(); > > + > > + case PROFILE: { > > Are the parens Fennec style? They're not necessary, right? This code in gone in the new patch. > @@ +114,5 @@ > > + private static String getBranchForProfileName(String profileName) { > > + return Scope.PROFILE.getPrefsName() + "-" + profileName; > > + } > > + > > + private static synchronized void migrateIfNecessary(Context context) { > > This context appears to be unused. Ditto. > @@ +144,5 @@ > > + * Move all preferences stored in PreferenceManager's default prefs either to > > + * APP or PROFILE scopes. The PROFILE scope keys are defined in PROFILE_MIGRATIONS_1. > > + */ > > + private static void performMigration1() { > > + final SharedPreferences.Editor appEditor = > > Consider taking these |Editor| instances as parameters; in future we're > going to chain these, right? And the caller should own the final |commit|. Good point. Done. > @@ +158,5 @@ > > + final List<String> profileMigrations = Arrays.asList(PROFILE_MIGRATIONS_1); > > + > > + final SharedPreferences pmPrefs = PreferenceManager.getDefaultSharedPreferences(context); > > + > > + for (Map.Entry<String, ?> entry : pmPrefs.getAll().entrySet()) { > > This logic seems quite general, but is currently a little tied to > PROFILE_MIGRATIONS_1. Can we break this out for testing and re-use in > future migrations. Good idea. Done. > @@ +190,5 @@ > > + to.putFloat(key, (Float) value); > > + } else if (value instanceof Integer) { > > + to.putInt(key, (Integer) value); > > + } else if (value instanceof Set) { > > + to.putStringSet(key, (Set<String>) value); > > You want to do *something* (log? I prefer to throw checked) in a final else > case. Also, I vaguely recall that string sets are not supported well in > some versions of Android. Ah yes, no can do pre v11: > > http://developer.android.com/reference/android/content/SharedPreferences. > Editor.html#putStringSet%28java.lang.String,%20java.util.Set%3Cjava.lang. > String%3E%29 > http://developer.android.com/reference/android/content/SharedPreferences. > html#getStringSet%28java.lang.String,%20java.util.Set%3Cjava.lang. > String%3E%29 Added the version check for string set keys. Forgot to do the throw in a final else. Will do. > ::: mobile/android/tests/browser/junit3/src/tests/TestGeckoSharedPrefs.java > @@ +19,5 @@ > > +/** > > + * Test GeckoSharedPrefs migrations. > > + */ > > +public class TestGeckoSharedPrefs extends BrowserTestCase { > > + public void testGeckoSharedPrefs() { > > You can have multiple |test*| methods; all will get run (in unspecified > order). So if this is just testing migration, you might name it > |testMigration| (or |testGeckoSharedPrefsMigration|). Done. > @@ +37,5 @@ > > + assertTrue(pmPrefs.getAll().isEmpty()); > > + > > + final SharedPreferences.Editor pmEditor = pmPrefs.edit(); > > + > > + // migration path a bit more thoroughly > > nit: caps, full sentences (throughout). Ugh, this comment got scrambled for some reason. Fixed. > @@ +38,5 @@ > > + > > + final SharedPreferences.Editor pmEditor = pmPrefs.edit(); > > + > > + // migration path a bit more thoroughly > > + // Insert a key for each type to exercise the > > the... > > @@ +59,5 @@ > > + assertEquals(pmPrefs.getAll().size(), 7); > > + > > + // Trigger migration by getting prefs for a scope > > + appPrefs = GeckoSharedPrefs.get(GeckoSharedPrefs.Scope.APP); > > + assertEquals(pmPrefs.getAll().size(), 0); > > nit: in JUnit 3, expected comes first. Fixed.

(In reply to Richard Newman [:rnewman] from comment #10) > Comment on attachment 8384615 [details] [diff] [review] > Implement scopes for SharedPreferences (r=rnewman) > > Review of attachment 8384615 [details] [diff] [review]: > ----------------------------------------------------------------- > > Concur with all of nalexander's comments. > > ::: mobile/android/base/DataReportingNotification.java > @@ -25,5 @@ > > private static final String LOGTAG = "DataReportNotification"; > > > > public static final String ALERT_NAME_DATAREPORTING_NOTIFICATION = "datareporting-notification"; > > > > - private static final String DEFAULT_PREFS_BRANCH = AppConstants.ANDROID_PACKAGE_NAME + "_preferences"; > > You need to migrate this branch. This is just a redundant branch name that is, in practice, equivalent to using PreferenceManager.getDefaultSharedPreferences(context). See: https://github.com/android/platform_frameworks_base/blob/master/core/java/android/preference/PreferenceManager.java#L373 I confirmed this in local tests. Should these be migrated to profile scope? We can do this but I'd rather do it in a separate patch/migration (this is why I implemented the notion of incremental migrations anyway). I'd like to keep this patch as small as possible. > ::: mobile/android/base/GeckoProfile.java > @@ +146,5 @@ > > } > > > > public static boolean removeProfile(Context context, String profileName) { > > + // Clear all shared prefs for the given profile > > + GeckoSharedPrefs.getForProfileName(profileName) > > Be prepared for the profile to not exist. Good point. The new patch only removes prefs if the profile was successfully removed. > ::: mobile/android/base/GeckoSharedPrefs.java > @@ +89,5 @@ > > + } > > + > > + public static SharedPreferences getForProfileName(String profileName, EnumSet<Flags> flags) { > > + if (!flags.contains(Flags.DISABLE_MIGRATIONS)) { > > + migrateIfNecessary(context); > > Surely the migration needs to know which profile's prefs are being migrated? > They're different files. > > You're also going to need to know the default profile, or otherwise figure > out the default destination, because the next step after this patch is to > migrate a whole bunch of app-level prefs into per-profile prefs. This is what the first migration does. For now, I only moved two keys to the profile scope (default profile in this case). We can move more later. For the sake of simplicity, the migration only moves keys to the default profile as this is what we need now. And the version tracking is not per-profile. We'll have to revisit this if we ever support proper profile switching. We don't need to do migration for the guest profile as it's, by design, supposed to start with an empty profile. Feel free to suggest ways to make this more robust. But let's no over-engineer this for use cases that we don't even have yet.

Just to be clear: the reason why I'm only migrating keys to the default profile is that I'm *moving* the keys around. For example, in the case of PreferenceManager, I copy all keys to app/profile scopes then clear all its keys. This is why the version is stored in the app scope: once the keys are migrated, they won't exist in their original source anymore. This means we wouldn't be able to perform the same migration for a different profile later. Another way to approach this would to leave the migrated keys around so that we can run the same migration for different profiles at a later time. So, for example, we'd keep all keys in PreferenceManager after migrating them to profile X so that we're still able to perform the same migration for profile Y later. In which case, the prefs version could be stored in the profile scope. However, part of the initial migration is about moving PreferenceManager keys to *app* scope too. This means we'd have to have a version for the app scope and another for the profile scope to avoid migrating app-scoped keys again when performing multiple profile migrations for the same version. I expect future migrations to be about moving keys from app to profile scope. The same idea applies: we'd need to keep app keys around to allow us to do multiple profile migrations for the same keys. Thoughts?

Comment on attachment 8388592 [details] [diff] [review] Implement scopes for SharedPreferences (r=rnewman) Review of attachment 8388592 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delayed review; end of the cycle, and it was a busy one for me :) This is looking really good. ::: mobile/android/base/GeckoSharedPrefs.java @@ +19,5 @@ > +/** > + * {@code GeckoSharedPrefs} Provides scoped SharedPreferences instances. > + * You should use this API instead of using Context.getSharedPreferences() > + * directly. There are three methods to get scoped SharedPreferences instances: > + * nit: trailing ws. I think this means you're using Eclipse! Yay! @@ +166,5 @@ > + > + appEditor.commit(); > + profileEditor.commit(); > + > + migrationDone = true; I'm not hugely fussed about this, but I don't think this guard is thread-safe. It is technically possible for two migrations to race, the first completing everything save setting the flag, and the second then doing everything before the first sets the flag. Is it clear that the second racer won't overwrite or delete the first racer's work? Even if it is clear (I have not thought through this), code that looks thread-safe but actually isn't, or even isn't *obviously* thread-safe, is worse than code that is *obviously not* thread-safe. We expect migrations to be uncommon: can we use simpler primitives (just synchronized?) to stop races in migration? This might mean having the flag check before entering the synchronized block... @@ +211,5 @@ > + } else if (value instanceof Integer) { > + to.putInt(key, (Integer) value); > + } else if (Build.VERSION.SDK_INT >= 11 && value instanceof Set) { > + to.putStringSet(key, (Set<String>) value); > + } I'd still like to see this throw on failure. And probably not handle Sets. instanceof, here, is not awesome; and Set<non-String> will cause bad things. ::: mobile/android/tests/browser/junit3/src/tests/TestGeckoSharedPrefs.java @@ +48,5 @@ > + usedPrefs.add(name); > + return wrappedContext.getSharedPreferences(PREFIX + name, mode); > + } > + > + public void clearAllPrefs() { This is good form, but let's do better: we can do the context initialization and clearing in setUp and tearDown. See http://junit.sourceforge.net/junit3.8.1/javadoc/junit/framework/TestCase.html @@ +114,5 @@ > + pmEditor.putString("string_key", "23"); > + pmEditor.putFloat("float_key", 23.3f); > + > + if (Build.VERSION.SDK_INT >= 11) { > + final Set<String> strSet = new HashSet<String>(); Is it important that we handle string sets? I'd prefer to dis-allow them if they're not required, rather than to always warp them with fragile version logic.

Comment on attachment 8388592 [details] [diff] [review] Implement scopes for SharedPreferences (r=rnewman) Review of attachment 8388592 [details] [diff] [review]: ----------------------------------------------------------------- One overall comment: in-line migrations mean that a prefs *access* can now cause multiple disk writes. Parts of the app that were cheating re strict mode will now break during migrations. Please take a look at call sites and make sure that this isn't going to trip us up! ::: mobile/android/base/CrashReporter.java @@ +234,1 @@ > Might as well fix trailing whitespace as you go. ::: mobile/android/base/GeckoSharedPrefs.java @@ +133,5 @@ > + final Editor appEditor = appPrefs.edit(); > + > + // The migration always moves prefs to the default profile, not > + // the current one. We might have to revisit this if we ever support > + // multiple profiles. Can this interact poorly with guest profiles? Or are we safe because you can't launch a guest profile without running the default profile first? @@ +166,5 @@ > + > + appEditor.commit(); > + profileEditor.commit(); > + > + migrationDone = true; Re Nick: this method *is* synchronized. The synchronization stops concurrent migrations, while the migrationDone flag efficiently stops repeat migrations within the same process lifespan.

> > + migrationDone = true; > > Re Nick: this method *is* synchronized. The synchronization stops concurrent > migrations, while the migrationDone flag efficiently stops repeat migrations > within the same process lifespan. mea culpa; I just missed it when reading the methods. Then this is solid!

(In reply to Nick Alexander :nalexander from comment #15) > Comment on attachment 8388592 [details] [diff] [review] > Implement scopes for SharedPreferences (r=rnewman) > > Review of attachment 8388592 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sorry for the delayed review; end of the cycle, and it was a busy one for me > :) This is looking really good. > > ::: mobile/android/base/GeckoSharedPrefs.java > @@ +19,5 @@ > > +/** > > + * {@code GeckoSharedPrefs} Provides scoped SharedPreferences instances. > > + * You should use this API instead of using Context.getSharedPreferences() > > + * directly. There are three methods to get scoped SharedPreferences instances: > > + * > > nit: trailing ws. I think this means you're using Eclipse! Yay! Fixed. > @@ +166,5 @@ > > + > > + appEditor.commit(); > > + profileEditor.commit(); > > + > > + migrationDone = true; > > I'm not hugely fussed about this, but I don't think this guard is > thread-safe. It is technically possible for two migrations to race, the > first completing everything save setting the flag, and the second then doing > everything before the first sets the flag. Is it clear that the second > racer won't overwrite or delete the first racer's work? > > Even if it is clear (I have not thought through this), code that looks > thread-safe but actually isn't, or even isn't *obviously* thread-safe, is > worse than code that is *obviously not* thread-safe. We expect migrations > to be uncommon: can we use simpler primitives (just synchronized?) to stop > races in migration? This might mean having the flag check before entering > the synchronized block... As rnewman has already noted, the migration is all done in a synchronized method. And the migration control members are only accessed from this synchronized method. > @@ +211,5 @@ > > + } else if (value instanceof Integer) { > > + to.putInt(key, (Integer) value); > > + } else if (Build.VERSION.SDK_INT >= 11 && value instanceof Set) { > > + to.putStringSet(key, (Set<String>) value); > > + } > > I'd still like to see this throw on failure. And probably not handle Sets. > instanceof, here, is not awesome; and Set<non-String> will cause bad things. Fair enough, done. > ::: mobile/android/tests/browser/junit3/src/tests/TestGeckoSharedPrefs.java > @@ +48,5 @@ > > + usedPrefs.add(name); > > + return wrappedContext.getSharedPreferences(PREFIX + name, mode); > > + } > > + > > + public void clearAllPrefs() { > > This is good form, but let's do better: we can do the context initialization > and clearing in setUp and tearDown. See > > http://junit.sourceforge.net/junit3.8.1/javadoc/junit/framework/TestCase.html Good point, done. > @@ +114,5 @@ > > + pmEditor.putString("string_key", "23"); > > + pmEditor.putFloat("float_key", 23.3f); > > + > > + if (Build.VERSION.SDK_INT >= 11) { > > + final Set<String> strSet = new HashSet<String>(); > > Is it important that we handle string sets? I'd prefer to dis-allow them if > they're not required, rather than to always warp them with fragile version > logic. Removed.

(In reply to Richard Newman [:rnewman] from comment #16) > Comment on attachment 8388592 [details] [diff] [review] > Implement scopes for SharedPreferences (r=rnewman) > > Review of attachment 8388592 [details] [diff] [review]: > ----------------------------------------------------------------- > > One overall comment: in-line migrations mean that a prefs *access* can now > cause multiple disk writes. Parts of the app that were cheating re strict > mode will now break during migrations. Please take a look at call sites and > make sure that this isn't going to trip us up! Yeah, I was a bit concerned about this too. I had a closer look at where the migration is going to be triggered on startup. The first access to GeckoSharedPrefs will happen here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1307 Which happens in GeckoApp's onCreate() in the UI thread. I changed the migration code to ensure the Editor.commit() calls are made in the background thread while still blocking the thread that triggered the migration. This will avoid any StrictMode issues while still ensuring the migration is fully done whenever the GeckoSharedPrefs is used for the first time. > ::: mobile/android/base/CrashReporter.java > @@ +234,1 @@ > > > > Might as well fix trailing whitespace as you go. > > ::: mobile/android/base/GeckoSharedPrefs.java > @@ +133,5 @@ > > + final Editor appEditor = appPrefs.edit(); > > + > > + // The migration always moves prefs to the default profile, not > > + // the current one. We might have to revisit this if we ever support > > + // multiple profiles. > > Can this interact poorly with guest profiles? Or are we safe because you > can't launch a guest profile without running the default profile first? What kind of potential issue do you have in mind? Just to be clearer: the point of this comment is that the ideal implementation of this ProfileManager migration would migrate the profile-scoped keys to all existing (non-guest) profiles but I'm only migrating the keys to the default profile as this is what we need for now.

Comment on attachment 8397886 [details] [diff] [review] Handle null background thread in isOnBackgroundThread() (r=rnewman) sBackgroundThread can be null if you can isOnBackgroundThread() before the first getBackgroundHandler() call. This happens in the instrumentation test I added.

Comment on attachment 8397887 [details] [diff] [review] Implement scopes for SharedPreferences (r=rnewman) Covers the review comments. This forces the migration to happen in the background thread to avoid any StrictMode issues.

Comment on attachment 8397887 [details] [diff] [review] Implement scopes for SharedPreferences (r=rnewman) Review of attachment 8397887 [details] [diff] [review]: ----------------------------------------------------------------- I gave this a quick once over, focusing on the test code, and it looks good to me! ::: mobile/android/base/CrashReporter.java @@ +234,1 @@ > nit: kill ws throughout! ::: mobile/android/base/preferences/MultiChoicePreference.java @@ +203,5 @@ > if (value == getPersistedBoolean(!value)) { > // It's already there, so the same as persisting > return true; > } > nit: kill ws!

Comment on attachment 8397886 [details] [diff] [review] Handle null background thread in isOnBackgroundThread() (r=rnewman) Review of attachment 8397886 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/util/ThreadUtils.java @@ +157,5 @@ > return isOnThread(getUiThread()); > } > > public static boolean isOnBackgroundThread() { > + if (sBackgroundThread == null) { Please also make sBackgroundThread (and sUiThread, I guess) volatile.

Comment on attachment 8397887 [details] [diff] [review] Implement scopes for SharedPreferences (r=rnewman) Review of attachment 8397887 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for your patience, Lucas. Looks good. ::: mobile/android/base/GeckoApp.java @@ +242,5 @@ > return GeckoApp.getAppSharedPreferences(); > } > > public static SharedPreferences getAppSharedPreferences() { > + return GeckoSharedPrefs.forApp(sAppContext); Worth adding a null check here to get a clearer exception? It's possible for a different activity to end up calling GeckoApp.getAppSharedPreferences() before GeckoApp.onCreate has been called. ::: mobile/android/base/GeckoProfile.java @@ +147,5 @@ > } > } > > public static boolean removeProfile(Context context, String profileName) { > + boolean success = false; I think this can safely be final boolean success; javac's dumb, but not totally stupid at flow analysis! @@ +156,5 @@ > return true; > } > + > + if (success) { > + // Clear all shared prefs for the given profile Nit: trailing period. @@ +225,5 @@ > private static void removeGuestProfile(Context context) { > try { > + // Clear all shared prefs for the guest profile > + GeckoSharedPrefs.forProfileName(context, getGuestProfile(context).getName()) > + .edit().clear().commit(); Can we put this in a separate try block, so that even if this fails for some reason, we still attempt to delete the guest profile directory? ::: mobile/android/base/GeckoSharedPrefs.java @@ +20,5 @@ > +import java.util.Map; > +import java.util.Set; > + > +/** > + * {@code GeckoSharedPrefs} Provides scoped SharedPreferences instances. s/Pro/pro @@ +71,5 @@ > + public enum Flags { > + DISABLE_MIGRATIONS > + } > + > + // Used when fetching profile-scoped prefs Nit: period. (Throughout.) @@ +83,5 @@ > + * Returns an app-scoped SharedPreferences instance. You can disable > + * migrations by using the DISABLE_MIGRATIONS flag. > + */ > + public static SharedPreferences forApp(Context context, EnumSet<Flags> flags) { > + if (!flags.contains(Flags.DISABLE_MIGRATIONS)) { Check for flags == null. @@ +118,5 @@ > + * You can disable migrations by using the DISABLE_MIGRATION flag. > + */ > + public static SharedPreferences forProfileName(Context context, String profileName, > + EnumSet<Flags> flags) { > + if (!flags.contains(Flags.DISABLE_MIGRATIONS)) { null check. @@ +181,5 @@ > + migrationDone = true; > + } > + > + private static void performMigration(Context context) { > + Log.d(LOGTAG, "Performing migration"); Perhaps move this log item to after the currentVersion check?

(In reply to Richard Newman [:rnewman] from comment #25) > Comment on attachment 8397886 [details] [diff] [review] > Handle null background thread in isOnBackgroundThread() (r=rnewman) > > Review of attachment 8397886 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/util/ThreadUtils.java > @@ +157,5 @@ > > return isOnThread(getUiThread()); > > } > > > > public static boolean isOnBackgroundThread() { > > + if (sBackgroundThread == null) { > > Please also make sBackgroundThread (and sUiThread, I guess) volatile. Good point, done.

(In reply to Richard Newman [:rnewman] from comment #26) > Comment on attachment 8397887 [details] [diff] [review] > Implement scopes for SharedPreferences (r=rnewman) > > Review of attachment 8397887 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks for your patience, Lucas. Looks good. > > ::: mobile/android/base/GeckoApp.java > @@ +242,5 @@ > > return GeckoApp.getAppSharedPreferences(); > > } > > > > public static SharedPreferences getAppSharedPreferences() { > > + return GeckoSharedPrefs.forApp(sAppContext); > > Worth adding a null check here to get a clearer exception? > > It's possible for a different activity to end up calling > GeckoApp.getAppSharedPreferences() before GeckoApp.onCreate has been called. Done, also made sAppContext volatile. > ::: mobile/android/base/GeckoProfile.java > @@ +147,5 @@ > > } > > } > > > > public static boolean removeProfile(Context context, String profileName) { > > + boolean success = false; > > I think this can safely be > > final boolean success; > > javac's dumb, but not totally stupid at flow analysis! Done. > @@ +156,5 @@ > > return true; > > } > > + > > + if (success) { > > + // Clear all shared prefs for the given profile > > Nit: trailing period. Done. > @@ +225,5 @@ > > private static void removeGuestProfile(Context context) { > > try { > > + // Clear all shared prefs for the guest profile > > + GeckoSharedPrefs.forProfileName(context, getGuestProfile(context).getName()) > > + .edit().clear().commit(); > > Can we put this in a separate try block, so that even if this fails for some > reason, we still attempt to delete the guest profile directory? Used the same logic then in removeProfile(). > ::: mobile/android/base/GeckoSharedPrefs.java > @@ +20,5 @@ > > +import java.util.Map; > > +import java.util.Set; > > + > > +/** > > + * {@code GeckoSharedPrefs} Provides scoped SharedPreferences instances. > > s/Pro/pro Fixed. > @@ +71,5 @@ > > + public enum Flags { > > + DISABLE_MIGRATIONS > > + } > > + > > + // Used when fetching profile-scoped prefs > > Nit: period. (Throughout.) Fixed. > @@ +83,5 @@ > > + * Returns an app-scoped SharedPreferences instance. You can disable > > + * migrations by using the DISABLE_MIGRATIONS flag. > > + */ > > + public static SharedPreferences forApp(Context context, EnumSet<Flags> flags) { > > + if (!flags.contains(Flags.DISABLE_MIGRATIONS)) { > > Check for flags == null. Done. > @@ +118,5 @@ > > + * You can disable migrations by using the DISABLE_MIGRATION flag. > > + */ > > + public static SharedPreferences forProfileName(Context context, String profileName, > > + EnumSet<Flags> flags) { > > + if (!flags.contains(Flags.DISABLE_MIGRATIONS)) { > > null check. Done. > @@ +181,5 @@ > > + migrationDone = true; > > + } > > + > > + private static void performMigration(Context context) { > > + Log.d(LOGTAG, "Performing migration"); > > Perhaps move this log item to after the currentVersion check? Good point, done.

https://hg.mozilla.org/integration/fx-team/rev/b13253fea342 https://hg.mozilla.org/integration/fx-team/rev/0140e7b8fa0d

https://hg.mozilla.org/mozilla-central/rev/b13253fea342 https://hg.mozilla.org/mozilla-central/rev/0140e7b8fa0d

This caused a large regression in Android ts-paint: Message: 2 Date: Wed, 02 Apr 2014 06:23:07 -0000 From: nobody@cruncher.build.mozilla.org To: dev-tree-management@lists.mozilla.org Subject: <Regression> Fx-Team - Ts, Paint - Android 4.0.4 - 147% Message-ID: <20140402062307.C67371044E3@cruncher.srv.releng.scl3.mozilla.com> Content-Type: text/plain; charset="us-ascii" Regression: Fx-Team - Ts, Paint - Android 4.0.4 - 147% increase --------------------------------------------------------------- Previous: avg 3465.806 stddev 48.658 of 12 runs up to revision c8d89b743158 New : avg 8570.888 stddev 1208.493 of 12 runs since revision 0140e7b8fa0d Change : +5105.082 (147% / z=104.918) Graph : http://mzl.la/1pMMaeq Changeset range: http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=c8d89b743158&tochange=0140e7b8fa0d Changesets: * http://hg.mozilla.org/integration/fx-team/rev/b13253fea342 : Lucas Rocha <lucasr@mozilla.com> - Bug 940575 - Handle null background thread in isOnBackgroundThread() (r=rnewman) : http://bugzilla.mozilla.org/show_bug.cgi?id=940575 * http://hg.mozilla.org/integration/fx-team/rev/0140e7b8fa0d : Lucas Rocha <lucasr@mozilla.com> - Bug 940575 - Implement scopes for SharedPreferences (r=rnewman) : http://bugzilla.mozilla.org/show_bug.cgi?id=940575 Bugs: * http://bugzilla.mozilla.org/show_bug.cgi?id=940575 - Implement per-profile SharedPreferences, eliminating uses of PreferenceManager ------------------------------ Message: 3 Date: Wed, 02 Apr 2014 06:23:40 -0000 From: nobody@cruncher.build.mozilla.org To: dev-tree-management@lists.mozilla.org Subject: <Regression> Fx-Team - Ts, Paint - Android 2.2 (Native) - 290% Message-ID: <20140402062340.A005E1044E3@cruncher.srv.releng.scl3.mozilla.com> Content-Type: text/plain; charset="us-ascii" Regression: Fx-Team - Ts, Paint - Android 2.2 (Native) - 290% increase ---------------------------------------------------------------------- Previous: avg 3553.952 stddev 260.998 of 12 runs up to revision c8d89b743158 New : avg 13842.983 stddev 548.475 of 12 runs since revision 0140e7b8fa0d Change : +10289.032 (290% / z=39.422) Graph : http://mzl.la/1pMMpGq Changeset range: http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=c8d89b743158&tochange=0140e7b8fa0d Changesets: * http://hg.mozilla.org/integration/fx-team/rev/b13253fea342 : Lucas Rocha <lucasr@mozilla.com> - Bug 940575 - Handle null background thread in isOnBackgroundThread() (r=rnewman) : http://bugzilla.mozilla.org/show_bug.cgi?id=940575 * http://hg.mozilla.org/integration/fx-team/rev/0140e7b8fa0d : Lucas Rocha <lucasr@mozilla.com> - Bug 940575 - Implement scopes for SharedPreferences (r=rnewman) : http://bugzilla.mozilla.org/show_bug.cgi?id=940575 Bugs: * http://bugzilla.mozilla.org/show_bug.cgi?id=940575 - Implement per-profile SharedPreferences, eliminating uses of PreferenceManager

Comment on attachment 8397887 [details] [diff] [review] Implement scopes for SharedPreferences (r=rnewman) Review of attachment 8397887 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoApp.java @@ +1770,5 @@ > return shouldRestore; > } > > private String getSessionRestorePreference() { > + return getAppSharedPreferences().getString(GeckoPreferences.PREFS_RESTORE_SESSION, "quit"); Does this change where the session store pref is found, possibly making ts_paint run through the session store logic in a way that it previously did not?

(In reply to Nick Alexander :nalexander from comment #32) > Comment on attachment 8397887 [details] [diff] [review] > Implement scopes for SharedPreferences (r=rnewman) > > Review of attachment 8397887 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/GeckoApp.java > @@ +1770,5 @@ > > return shouldRestore; > > } > > > > private String getSessionRestorePreference() { > > + return getAppSharedPreferences().getString(GeckoPreferences.PREFS_RESTORE_SESSION, "quit"); > > Does this change where the session store pref is found, possibly making > ts_paint run through the session store logic in a way that it previously did > not? I'd expect Fennec to be uninstalled before installing the new APK to run the talos test. The app shouldn't have any SharedPrefs keys when the test starts. In any case, GeckoSharedPrefs migrates all PreferenceManager keys to app/profile scopes before getAppSharedPreferences() returns. There's some overhead from the migration here but I wouldn't expect it to take 3 seconds to finish. My guess is that the thread mangling used in the migration code performs quite poorly on single core devices.

FYI: I posted a patch in bug 991160 that fixes the major regression caused by these patches here.

The following changesets are now in Firefox Nightly: > b13253fea342 Bug 940575 - Handle null background thread in isOnBackgroundThread() (r=rnewman) > 0140e7b8fa0d Bug 940575 - Implement scopes for SharedPreferences (r=rnewman) Nightly Build Information: ID: 20140402030201 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6 Version: 31.0a1 TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786 URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central Download Links: > Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2 > Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2 > Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2 > Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg > Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe > Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe Previous Nightly Build Information: ID: 20140401030203 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4 Version: 31.0a1 TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8 URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central

It's not a problem anymore since bug 991160 landed, but just to point out that the regression was due to a race condition when waiting for the background thread [1]. The task is posted to the background thread outside of the synchronized wait block [2], which means if the task runs before we ever enter the synchronized block, migrationLock.notifyAll() will be called before migrationLock.wait() and that prevents the wake up from happening. Our wait() ends up timing out. Also Object.wait() should be used in a condition loop to handle spurious wakeups [3]. So all things considered, it's probably best to do things on the main thread to avoid these gotchas :) [1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoSharedPrefs.java?rev=0140e7b8fa0d#162 [2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoSharedPrefs.java?rev=0140e7b8fa0d#173 [3] http://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#wait%28%29

Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release). Filter on epic-hub-bugs.