Bug 1401318 - Fix some of the 'shared-state' access problems around Account r=nalexander
authorGrigory Kruglov <gkruglov@mozilla.com>
Tue, 10 Oct 2017 13:33:50 -0400
changeset 385366 e464d1cc05a6cc106bd96fd2e48676f5574e0096
parent 385365 e3834abf326e8dd949dc28475cb3606d890c50a1
child 385367 a94bf3908a349df271d6b090e8391a386ef29252
push id52993
push usergkruglov@mozilla.com
push dateTue, 10 Oct 2017 18:43:10 +0000
treeherderautoland@e464d1cc05a6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnalexander
bugs1401318, 1407316
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1401318 - Fix some of the 'shared-state' access problems around Account r=nalexander This patch fixes several symptoms of shared state (internal 'account' instance) getting out-of-sync with the world. We maintain a representation of an internal Account in the AndroidFxAccount, but nothing is preventing that representation to become irrelevant in certain situations. This patch ensures we 'update our internal cache', so to speak, before trying to act upon it. Changes in the 'profile JSON fetched' flow are necessary to support the 'email might change' case. Locking is necessary to ensure correct behaviour in case of overlapping syncing and profile fetching. Changes in 'getState' are necessary to ensure we behave correctly when a long-lived AndroidFxAccount instance is interrogated (as in the Sync Prefs UI) after internal account changes. There are likely to be other "symptoms", but this patch aims to be safely upliftable in order to support changing of a primary email. See Bug 1407316 for further root-cause analysis and proposed solution. MozReview-Commit-ID: AXmTBMzL2cf
mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java
@@ -696,16 +696,23 @@ public class AndroidFxAccount {
 
   private void broadcastAccountStateChangedIntent() {
     final Intent intent = new Intent(FxAccountConstants.ACCOUNT_STATE_CHANGED_ACTION);
     intent.putExtra(Constants.JSON_KEY_ACCOUNT, account.name);
     LocalBroadcastManager.getInstance(context).sendBroadcast(intent);
   }
 
   public synchronized State getState() {
+    // Ensure we're working with the latest 'account' state. It might have changed underneath us.
+    // Note that this "state refresh" is inefficient for some callers, since they might have
+    // created this object (and thus fetched an account from the accounts system) just moments ago.
+    // Other callers maintain this object for a while, and thus might be out-of-sync with the world.
+    // See Bug 1407316 for higher-order improvements that will make this unnecessary.
+    account = FirefoxAccounts.getFirefoxAccount(context);
+
     String stateLabelString = getBundleData(BUNDLE_KEY_STATE_LABEL);
     String stateString = getBundleData(BUNDLE_KEY_STATE);
     if (stateLabelString == null || stateString == null) {
       throw new IllegalStateException("stateLabelString and stateString must not be null, but: " +
           "(stateLabelString == null) = " + (stateLabelString == null) +
           " and (stateString == null) = " + (stateString == null));
     }
 
@@ -717,16 +724,20 @@ public class AndroidFxAccount {
       throw new IllegalStateException("could not get state", e);
     }
   }
 
   /**
    * <b>For debugging only!</b>
    */
   public void dump() {
+    // Make sure we dump using the latest 'account'. It might have changed since this object was
+    // initialized.
+    account = FirefoxAccounts.getFirefoxAccount(context);
+
     if (!FxAccountUtils.LOG_PERSONAL_INFORMATION) {
       return;
     }
     ExtendedJSONObject o = toJSONObject();
     ArrayList<String> list = new ArrayList<String>(o.keySet());
     Collections.sort(list);
     for (String key : list) {
       FxAccountUtils.pii(LOG_TAG, key + ": " + o.get(key));
@@ -1041,66 +1052,84 @@ public class AndroidFxAccount {
     }
     if (!profileJSON.containsKey("email")) {
       Logger.error(LOG_TAG, "Profile JSON missing email key");
       callback.run();
       return;
     }
     final String email = profileJSON.getString("email");
 
-    // If primary email didn't change, there's nothing for us to do.
-    if (account.name.equals(email)) {
+    // To prevent race against a shared state, we need to hold a lock which is released after
+    // account is renamed, or it's determined that the rename isn't necessary.
+    try {
+      acquireSharedAccountStateLock(LOG_TAG);
+    } catch (InterruptedException e) {
+      Logger.error(LOG_TAG, "Could not acquire a shared account state lock.");
       callback.run();
       return;
     }
 
-    Logger.info(LOG_TAG, "Renaming Android Account.");
-    FxAccountUtils.pii(LOG_TAG, "Renaming Android account from " + account.name + " to " + email);
+    try {
+      // Update our internal state. It's possible that the account changed underneath us while
+      // we were fetching profile information. See Bug 1401318.
+      account = FirefoxAccounts.getFirefoxAccount(context);
 
-    // Then, get the currently auto-syncing authorities.
-    // We'll toggle these on once we re-add the account.
-    final Map<String, Boolean> currentAuthoritiesToSync = getAuthoritiesToSyncAutomaticallyMap();
+      // If primary email didn't change, there's nothing for us to do.
+      if (account.name.equals(email)) {
+        callback.run();
+        return;
+      }
 
-    // We also need to manually carry over current sync intervals.
-    final Map<String, List<PeriodicSync>> periodicSyncsForAuthorities = new HashMap<>();
-    for (String authority : currentAuthoritiesToSync.keySet()) {
-      periodicSyncsForAuthorities.put(authority, ContentResolver.getPeriodicSyncs(account, authority));
-    }
+      Logger.info(LOG_TAG, "Renaming Android Account.");
+      FxAccountUtils.pii(LOG_TAG, "Renaming Android account from " + account.name + " to " + email);
+
+      // Then, get the currently auto-syncing authorities.
+      // We'll toggle these on once we re-add the account.
+      final Map<String, Boolean> currentAuthoritiesToSync = getAuthoritiesToSyncAutomaticallyMap();
 
-    final Runnable migrateSyncSettings = new Runnable() {
-      @Override
-      public void run() {
-        // Set up auto-syncing for the newly added account.
-        setAuthoritiesToSyncAutomaticallyMap(currentAuthoritiesToSync);
+      // We also need to manually carry over current sync intervals.
+      final Map<String, List<PeriodicSync>> periodicSyncsForAuthorities = new HashMap<>();
+      for (String authority : currentAuthoritiesToSync.keySet()) {
+        periodicSyncsForAuthorities.put(authority, ContentResolver.getPeriodicSyncs(account, authority));
+      }
+
+      final Runnable migrateSyncSettings = new Runnable() {
+        @Override
+        public void run() {
+          // Set up auto-syncing for the newly added account.
+          setAuthoritiesToSyncAutomaticallyMap(currentAuthoritiesToSync);
 
-        // Set up all of the periodic syncs we had prior.
-        for (String authority : periodicSyncsForAuthorities.keySet()) {
-          final List<PeriodicSync> periodicSyncs = periodicSyncsForAuthorities.get(authority);
-          for (PeriodicSync periodicSync : periodicSyncs) {
-            ContentResolver.addPeriodicSync(
-                    account,
-                    periodicSync.authority,
-                    periodicSync.extras,
-                    periodicSync.period
-            );
+          // Set up all of the periodic syncs we had prior.
+          for (String authority : periodicSyncsForAuthorities.keySet()) {
+            final List<PeriodicSync> periodicSyncs = periodicSyncsForAuthorities.get(authority);
+            for (PeriodicSync periodicSync : periodicSyncs) {
+              ContentResolver.addPeriodicSync(
+                      account,
+                      periodicSync.authority,
+                      periodicSync.extras,
+                      periodicSync.period
+              );
+            }
           }
         }
-      }
-    };
+      };
+
+      // On API21+, we can simply "rename" the account, which will recreate it carrying over user data.
+      // Our regular "account was just deleted" side-effects will not run.
+      if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
+        doOptionalProfileRename21Plus(email, migrateSyncSettings, callback);
 
-    // On API21+, we can simply "rename" the account, which will recreate it carrying over user data.
-    // Our regular "account was just deleted" side-effects will not run.
-    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
-      doOptionalProfileRename21Plus(email, migrateSyncSettings, callback);
-
-    // Prior to API21, we have to perform this operation manually: make a copy of the user data,
-    // delete current account, and then re-create it.
-    // We also need to ensure our regular "account was just deleted" side-effects are not invoked.
-    } else {
-      doOptionalProfileRenamePre21(email, migrateSyncSettings, callback);
+        // Prior to API21, we have to perform this operation manually: make a copy of the user data,
+        // delete current account, and then re-create it.
+        // We also need to ensure our regular "account was just deleted" side-effects are not invoked.
+      } else {
+        doOptionalProfileRenamePre21(email, migrateSyncSettings, callback);
+      }
+    } finally {
+      releaseSharedAccountStateLock();
     }
   }
 
   @TargetApi(Build.VERSION_CODES.LOLLIPOP)
   private void doOptionalProfileRename21Plus(final String newEmail, final Runnable migrateSyncSettingsCallback, final Runnable callback) {
     final Account currentAccount = new Account(account.name, account.type);
     accountManager.renameAccount(currentAccount, newEmail, new AccountManagerCallback<Account>() {
       @Override