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
--- 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;