Bug 1400742 - Remove per-account in-memory cache r=nalexander draft
authorGrigory Kruglov <gkruglov@mozilla.com>
Wed, 20 Sep 2017 18:21:06 -0400
changeset 667947 8bcdd1bc084700694d52bce3a2f1ae536b7fe9e1
parent 667935 319a34bea9e4f3459886b5b9e835bd338320f1fd
child 732550 86a658482ea9b55f306ca986335735be3dfd0098
push id80887
push userbmo:gkruglov@mozilla.com
push dateWed, 20 Sep 2017 22:28:33 +0000
reviewersnalexander
bugs1400742, 1368147, 964854
milestone57.0a1
Bug 1400742 - Remove per-account in-memory cache r=nalexander 1368147 added reading fxa.account.json into the unbundle() codepath for the cases when the in-memory cache isn't populated. This surfaced a race condition: pickling of the fxa.account.json file and running unbundle() (as triggered from various parts of the UI, or othe SyncStatusObservers) will race, and if unbundle wins, it will attempt to read a yet-to-be-created fxa.account.json file, and crash. Fixing the race isn't trivial, but we can avoid it by removing the in-memory cache, thus avoiding having to read the cache key from the pickled file (uid). In-memory cache was added in response to caching/invalidation issues of set/getUserData, see Bug 964854 for the history. The current thinking is that those problems are pre-API16, which hopefully means that we shouldn't encounter them anymore, and thus can remove the workaround entirely. MozReview-Commit-ID: AfL2Jq4IlYT
mobile/android/base/java/org/mozilla/gecko/AccountsHelper.java
mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java
mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/FxAccountAuthenticator.java
mobile/android/services/src/main/java/org/mozilla/gecko/fxa/receivers/FxAccountDeletedService.java
--- a/mobile/android/base/java/org/mozilla/gecko/AccountsHelper.java
+++ b/mobile/android/base/java/org/mozilla/gecko/AccountsHelper.java
@@ -83,20 +83,16 @@ public class AccountsHelper implements B
             Log.e(LOGTAG, "Profile is not allowed to modify accounts!  Ignoring event: " + event);
             if (callback != null) {
                 callback.sendError("Profile is not allowed to modify accounts!");
             }
             return;
         }
 
         if ("Accounts:CreateFirefoxAccountFromJSON".equals(event)) {
-            // As we are about to create a new account, let's ensure our in-memory accounts cache
-            // is empty so that there are no undesired side-effects.
-            AndroidFxAccount.invalidateCaches();
-
             AndroidFxAccount fxAccount = null;
             try {
                 final GeckoBundle json = message.getBundle("json");
                 final String email = json.getString("email");
                 final String uid = json.getString("uid");
                 final boolean verified = json.getBoolean("verified", false);
                 final byte[] unwrapkB = Utils.hex2Byte(json.getString("unwrapBKey"));
                 final byte[] sessionToken = Utils.hex2Byte(json.getString("sessionToken"));
@@ -149,20 +145,16 @@ public class AccountsHelper implements B
                     return;
                 }
             }
             if (callback != null) {
                 callback.sendSuccess(fxAccount != null);
             }
 
         } else if ("Accounts:UpdateFirefoxAccountFromJSON".equals(event)) {
-            // We might be significantly changing state of the account; let's ensure our in-memory
-            // accounts cache is empty so that there are no undesired side-effects.
-            AndroidFxAccount.invalidateCaches();
-
             final Account account = FirefoxAccounts.getFirefoxAccount(mContext);
             if (account == null) {
                 if (callback != null) {
                     callback.sendError("Could not update Firefox Account since none exists");
                 }
                 return;
             }
 
--- 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
@@ -148,31 +148,16 @@ public class AndroidFxAccount {
   protected final Context context;
   private final AccountManager accountManager;
 
   // This is really, really meant to be final. Only changed when account name changes.
   // See Bug 1368147.
   protected volatile Account account;
 
   /**
-   * A cache associating Account name (email address) to a representation of the
-   * account's internal bundle.
-   * <p>
-   * The cache is invalidated entirely when <it>any</it> new Account is added,
-   * because there is no reliable way to know that an Account has been removed
-   * and then re-added.
-   */
-  private static final ConcurrentHashMap<String, ExtendedJSONObject> perAccountBundleCache =
-      new ConcurrentHashMap<>();
-
-  public static void invalidateCaches() {
-    perAccountBundleCache.clear();
-  }
-
-  /**
    * Create an Android Firefox Account instance backed by an Android Account
    * instance.
    * <p>
    * We expect a long-lived application context to avoid life-cycle issues that
    * might arise if the internally cached AccountManager instance surfaces UI.
    * <p>
    * We take care to not install any listeners or observers that might outlive
    * the AccountManager; and Android ensures the AccountManager doesn't outlive
@@ -239,71 +224,51 @@ public class AndroidFxAccount {
     // This should never happen. But it's useful to know if it does, so just crash.
     if (unpickledAccountUID == null) {
       throw new IllegalStateException("Unpickled account UID is null");
     }
 
     // Persist our UID into userData, so that we never have to do this dance again.
     accountManager.setUserData(account, ACCOUNT_KEY_UID, unpickledAccountUID);
 
-    // Our internal caches were also keyed by 'email' instead of 'uid', so we blow them away here
-    // so that they're re-populated correctly later on.
-    invalidateCaches();
-
     return unpickledAccountUID;
   }
 
   /**
    * Saves the given data as the internal bundle associated with this account.
    * @param bundle to write to account.
    */
   private synchronized void persistBundle(ExtendedJSONObject bundle) {
-    perAccountBundleCache.put(getAccountUID(), bundle);
     accountManager.setUserData(account, ACCOUNT_KEY_DESCRIPTOR, bundle.toJSONString());
   }
 
-  /* package-private */ ExtendedJSONObject unbundle() {
-    return unbundle(true);
-  }
-
   /**
    * Retrieve the internal bundle associated with this account.
    * @return bundle associated with account.
    */
-  private synchronized ExtendedJSONObject unbundle(boolean allowCachedBundle) {
-    final String accountUID = getAccountUID();
-
-    if (allowCachedBundle) {
-      final ExtendedJSONObject cachedBundle = perAccountBundleCache.get(accountUID);
-      if (cachedBundle != null) {
-        Logger.debug(LOG_TAG, "Returning cached account bundle.");
-        return cachedBundle;
-      }
-    }
+  /* package-private */ synchronized ExtendedJSONObject unbundle() {
+    final String bundleString = accountManager.getUserData(account, ACCOUNT_KEY_DESCRIPTOR);
 
     final int version = getAccountVersion();
     if (version < CURRENT_ACCOUNT_VERSION) {
       // Needs upgrade. For now, do nothing. We'd like to just put your account
       // into the Separated state here and have you update your credentials.
       return null;
     }
 
     if (version > CURRENT_ACCOUNT_VERSION) {
       // Oh dear.
+      throw new IllegalStateException("Invalid account bundle version. Current: " + CURRENT_ACCOUNT_VERSION + ", bundle version: " + version);
+    }
+
+    if (bundleString == null) {
       return null;
     }
 
-    String bundleString = accountManager.getUserData(account, ACCOUNT_KEY_DESCRIPTOR);
-    if (bundleString == null) {
-      return null;
-    }
-    final ExtendedJSONObject bundle = unbundleAccountV2(bundleString);
-    perAccountBundleCache.put(accountUID, bundle);
-    Logger.info(LOG_TAG, "Account bundle persisted to cache.");
-    return bundle;
+    return unbundleAccountV2(bundleString);
   }
 
   private String getBundleData(String key) {
     ExtendedJSONObject o = unbundle();
     if (o == null) {
       return null;
     }
     return o.getString(key);
@@ -1129,17 +1094,16 @@ public class AndroidFxAccount {
 
           // We tried, we really did.
           if (!updatedAccount.name.equals(newEmail)) {
             Logger.error(LOG_TAG, "Tried to update account name, but it didn't seem to have changed.");
           } else {
             account = updatedAccount;
             migrateSyncSettingsCallback.run();
 
-            invalidateCaches();
             callback.run();
           }
         } catch (OperationCanceledException | IOException | AuthenticatorException e) {
           Logger.error(LOG_TAG, "Unexpected exception while trying to rename an account", e);
           callback.run();
         }
       }
       // Request that callbacks are posted to the current thread.
@@ -1187,19 +1151,16 @@ public class AndroidFxAccount {
 
           accountManager.setUserData(account, ACCOUNT_KEY_RENAME_IN_PROGRESS, null);
           callback.run();
           return;
         }
 
         // It appears that we've successfully removed the account. It's now time to re-add it.
 
-        // Purge our internal state.
-        invalidateCaches();
-
         // Finally, add an Android account with new name and old user data.
         final Account newAccount = new Account(email, FxAccountConstants.ACCOUNT_TYPE);
         final boolean didAdd = accountManager.addAccountExplicitly(newAccount, null, currentUserData);
 
         // Rename succeeded, now let's configure the newly added account.
         if (didAdd) {
           account = newAccount;
 
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/FxAccountAuthenticator.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/FxAccountAuthenticator.java
@@ -54,20 +54,16 @@ public class FxAccountAuthenticator exte
 
   @Override
   public Bundle addAccount(AccountAuthenticatorResponse response,
       String accountType, String authTokenType, String[] requiredFeatures,
       Bundle options)
           throws NetworkErrorException {
     Logger.debug(LOG_TAG, "addAccount");
 
-    // The data associated to each Account should be invalidated when we change
-    // the set of Firefox Accounts on the system.
-    AndroidFxAccount.invalidateCaches();
-
     final Bundle res = new Bundle();
 
     if (!FxAccountConstants.ACCOUNT_TYPE.equals(accountType)) {
       res.putInt(AccountManager.KEY_ERROR_CODE, -1);
       res.putString(AccountManager.KEY_ERROR_MESSAGE, "Not adding unknown account type.");
       return res;
     }
 
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/receivers/FxAccountDeletedService.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/receivers/FxAccountDeletedService.java
@@ -44,21 +44,16 @@ public class FxAccountDeletedService ext
   public static final String LOG_TAG = FxAccountDeletedService.class.getSimpleName();
 
   public FxAccountDeletedService() {
     super(LOG_TAG);
   }
 
   @Override
   protected void onHandleIntent(final Intent intent) {
-    // We have an in-memory accounts cache which we use for a variety of tasks; it needs to be cleared.
-    // It should be fine to invalidate it before doing anything else, as the tasks below do not rely
-    // on this data.
-    AndroidFxAccount.invalidateCaches();
-
     // Intent can, in theory, be null. Bug 1025937.
     if (intent == null) {
       Logger.debug(LOG_TAG, "Short-circuiting on null intent.");
       return;
     }
 
     final Context context = this;