Bug 1148504 - Protect Firefox Account state with a critical section. r=rnewman, a=readinglist
authorNick Alexander <nalexander@mozilla.com>
Fri, 27 Mar 2015 08:27:28 -0700
changeset 258210 32b6b2c4a69e
parent 258209 0aedf96a7cdc
child 258211 c5baf4b4a350
push id4620
push userrnewman@mozilla.com
push date2015-04-02 16:21 +0000
treeherdermozilla-beta@27f61020a9e4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman, readinglist
bugs1148504
milestone38.0
Bug 1148504 - Protect Firefox Account state with a critical section. r=rnewman, a=readinglist ======== https://github.com/mozilla-services/android-sync/commit/8b1d353ee8c5f049c65d6ab437d3adee871ae8ec Author: Nick Alexander <nalexander@mozilla.com> Bug 1148504 - Part 2: Make updating Firefox Account state happen in a critical section. It's worth noting that the two consumers of the shared state lock will only race for a very short window -- essentially only when creating or re-connecting an account. That's because Reading List oauth tokens are long-lived and do not expire (yet) in response to remote Account state changes, such as updating the Account password. So Sync and RL will race to initialize the Account state; eventually RL will get an oauth token; and that token will be cached forever until RL produces a 401 for the token or Android expires the token. Since Sync requests a token server token at the start of every sync, the lock will be constantly exercised, but should never block. ======== https://github.com/mozilla-services/android-sync/commit/d7a8611810ebd6872df7ffdcf301e30520fc6ff9 Author: Nick Alexander <nalexander@mozilla.com> Bug 1148504 - Part 1: Reduce scope of section that may set Account state. The only place that might throw a TokenServerException is the token server client code itself. By handling such an exception earlier, we reduce the scope of the section that may update the Firefox Account state. (This comes at the cost of threading AndroidFxAccount into syncWithAssertion, but c'est la vie.) This does not interact with the exist handling of 401s that we might see from the storage endpoint. Those 401s never generated TokenServerExceptions; in fact, they were (essentially) ignored. Since we fetch a fresh token every Sync, what was (and is) expected is that such 401s would be transient and fixed by authenticating with a fresher token. Test plan: manually verify that remotely changing the Firefox Account's password while the device is in the Married state does the following: 1) uses the cached certificate to generate a local assertion; 2) the assertion produces a 401 from the TokenServerException, since the certificate is no longer fresh; 3) the TokenServerException drives the Account state to Cohabiting; 4) the state machine discovers it cannot /sign, driving the Account state to Separated.
mobile/android/base/fxa/authenticator/AndroidFxAccount.java
mobile/android/base/fxa/authenticator/FxAccountAuthenticator.java
mobile/android/base/fxa/sync/FxAccountSyncAdapter.java
mobile/android/base/fxa/sync/FxAccountSyncDelegate.java
mobile/android/base/reading/ReadingListSyncAdapter.java
--- a/mobile/android/base/fxa/authenticator/AndroidFxAccount.java
+++ b/mobile/android/base/fxa/authenticator/AndroidFxAccount.java
@@ -8,16 +8,17 @@ import java.io.UnsupportedEncodingExcept
 import java.net.URISyntaxException;
 import java.security.GeneralSecurityException;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.Semaphore;
 
 import org.mozilla.gecko.AppConstants;
 import org.mozilla.gecko.background.common.GlobalConstants;
 import org.mozilla.gecko.background.common.log.Logger;
 import org.mozilla.gecko.background.fxa.FxAccountUtils;
 import org.mozilla.gecko.db.BrowserContract;
 import org.mozilla.gecko.fxa.FirefoxAccounts;
 import org.mozilla.gecko.fxa.FxAccountConstants;
@@ -30,16 +31,17 @@ import org.mozilla.gecko.sync.setup.Cons
 
 import android.accounts.Account;
 import android.accounts.AccountManager;
 import android.content.ContentResolver;
 import android.content.Context;
 import android.content.Intent;
 import android.content.SharedPreferences;
 import android.os.Bundle;
+import android.util.Log;
 
 /**
  * A Firefox Account that stores its details and state as user data attached to
  * an Android Account instance.
  * <p>
  * Account user data is accessible only to the Android App(s) that own the
  * Account type. Account user data is not removed when the App's private data is
  * cleared.
@@ -646,9 +648,61 @@ public class AndroidFxAccount {
     } catch (UnsupportedEncodingException | GeneralSecurityException e) {
       // Ignore.
     }
     State state = getState();
     setState(state.makeSeparatedState());
     accountManager.setUserData(account, ACCOUNT_KEY_IDP_SERVER, authServerEndpoint);
     accountManager.setUserData(account, ACCOUNT_KEY_TOKEN_SERVER, tokenServerEndpoint);
   }
+
+  /**
+   * Take the lock to own updating any Firefox Account's internal state.
+   *
+   * We use a <code>Semaphore</code> rather than a <code>ReentrantLock</code>
+   * because the callback that needs to release the lock may not be invoked on
+   * the thread that initially acquired the lock. Be aware!
+   */
+  protected static final Semaphore sLock = new Semaphore(1, true /* fair */);
+
+  // Which consumer took the lock?
+  // Synchronized by this.
+  protected String lockTag = null;
+
+  // Are we locked?  (It's not easy to determine who took the lock dynamically,
+  // so we maintain this flag internally.)
+  // Synchronized by this.
+  protected boolean locked = false;
+
+  // Block until we can take the shared state lock.
+  public synchronized void acquireSharedAccountStateLock(final String tag) throws InterruptedException {
+    final long id = Thread.currentThread().getId();
+    this.lockTag = tag;
+    Log.d(Logger.DEFAULT_LOG_TAG, "Thread with tag and thread id acquiring lock: " + lockTag + ", " + id + " ...");
+    sLock.acquire();
+    locked = true;
+    Log.d(Logger.DEFAULT_LOG_TAG, "Thread with tag and thread id acquiring lock: " + lockTag + ", " + id + " ... ACQUIRED");
+  }
+
+  // If we hold the shared state lock, release it.  Otherwise, ignore the request.
+  public synchronized void releaseSharedAccountStateLock() {
+    final long id = Thread.currentThread().getId();
+    Log.d(Logger.DEFAULT_LOG_TAG, "Thread with tag and thread id releasing lock: " + lockTag + ", " + id + " ...");
+    if (locked) {
+      sLock.release();
+      locked = false;
+      Log.d(Logger.DEFAULT_LOG_TAG, "Thread with tag and thread id releasing lock: " + lockTag + ", " + id + " ... RELEASED");
+    } else {
+      Log.d(Logger.DEFAULT_LOG_TAG, "Thread with tag and thread id releasing lock: " + lockTag + ", " + id + " ... NOT LOCKED");
+    }
+  }
+
+  @Override
+  protected synchronized void finalize() {
+    if (locked) {
+      // Should never happen, but...
+      sLock.release();
+      locked = false;
+      final long id = Thread.currentThread().getId();
+      Log.e(Logger.DEFAULT_LOG_TAG, "Thread with tag and thread id releasing lock: " + lockTag + ", " + id + " ... RELEASED DURING FINALIZE");
+    }
+  }
 }
--- a/mobile/android/base/fxa/authenticator/FxAccountAuthenticator.java
+++ b/mobile/android/base/fxa/authenticator/FxAccountAuthenticator.java
@@ -88,36 +88,38 @@ public class FxAccountAuthenticator exte
   public Bundle editProperties(AccountAuthenticatorResponse response, String accountType) {
     Logger.debug(LOG_TAG, "editProperties");
 
     return null;
   }
 
   protected static class Responder {
     final AccountAuthenticatorResponse response;
-    final Account account;
+    final AndroidFxAccount fxAccount;
 
-    public Responder(AccountAuthenticatorResponse response, Account account) {
+    public Responder(AccountAuthenticatorResponse response, AndroidFxAccount fxAccount) {
       this.response = response;
-      this.account = account;
+      this.fxAccount = fxAccount;
     }
 
     public void fail(Exception e) {
       Logger.warn(LOG_TAG, "Responding with error!", e);
+      fxAccount.releaseSharedAccountStateLock();
       final Bundle result = new Bundle();
       result.putInt(AccountManager.KEY_ERROR_CODE, UNKNOWN_ERROR_CODE);
       result.putString(AccountManager.KEY_ERROR_MESSAGE, e.toString());
       response.onResult(result);
     }
 
     public void succeed(String authToken) {
       Logger.info(LOG_TAG, "Responding with success!");
+      fxAccount.releaseSharedAccountStateLock();
       final Bundle result = new Bundle();
-      result.putString(AccountManager.KEY_ACCOUNT_NAME, account.name);
-      result.putString(AccountManager.KEY_ACCOUNT_TYPE, account.type);
+      result.putString(AccountManager.KEY_ACCOUNT_NAME, fxAccount.account.name);
+      result.putString(AccountManager.KEY_ACCOUNT_TYPE, fxAccount.account.type);
       result.putString(AccountManager.KEY_AUTHTOKEN, authToken);
       response.onResult(result);
     }
   }
 
   public abstract static class FxADefaultLoginStateMachineDelegate implements LoginStateMachineDelegate {
     protected final Context context;
     protected final AndroidFxAccount fxAccount;
@@ -174,17 +176,17 @@ public class FxAccountAuthenticator exte
         handleMarried((Married) state);
       }
     }
   }
 
   protected void getOAuthToken(final AccountAuthenticatorResponse response, final AndroidFxAccount fxAccount, final String scope) throws NetworkErrorException {
     Logger.info(LOG_TAG, "Fetching oauth token with scope: " + scope);
 
-    final Responder responder = new Responder(response, fxAccount.getAndroidAccount());
+    final Responder responder = new Responder(response, fxAccount);
 
     final String oauthServerUri = FxAccountConstants.DEFAULT_OAUTH_SERVER_ENDPOINT;
     final String audience;
     try {
       audience = FxAccountUtils.getAudienceForURL(oauthServerUri); // The assertion gets traded in for an oauth bearer token.
     } catch (Exception e) {
       Logger.warn(LOG_TAG, "Got exception fetching oauth token.", e);
       responder.fail(e);
@@ -265,24 +267,35 @@ public class FxAccountAuthenticator exte
       return result;
     }
 
     // If we're asked for an oauth::scope token, try to generate one.
     final String oauthPrefix = "oauth::";
     if (authTokenType != null && authTokenType.startsWith(oauthPrefix)) {
       final String scope = authTokenType.substring(oauthPrefix.length());
       final AndroidFxAccount fxAccount = new AndroidFxAccount(context, account);
+      try {
+        fxAccount.acquireSharedAccountStateLock(LOG_TAG);
+      } catch (InterruptedException e) {
+        Logger.warn(LOG_TAG, "Could not acquire account state lock; return error bundle.");
+        final Bundle bundle = new Bundle();
+        bundle.putInt(AccountManager.KEY_ERROR_CODE, 1);
+        bundle.putString(AccountManager.KEY_ERROR_MESSAGE, "Could not acquire account state lock.");
+        return bundle;
+      }
       getOAuthToken(response, fxAccount, scope);
       return null;
     }
 
     // Otherwise, fail.
-    Logger.warn(LOG_TAG, "Returning null bundle for getAuthToken.");
-
-    return null;
+    Logger.warn(LOG_TAG, "Returning error bundle for getAuthToken with unknown token type.");
+    final Bundle bundle = new Bundle();
+    bundle.putInt(AccountManager.KEY_ERROR_CODE, 2);
+    bundle.putString(AccountManager.KEY_ERROR_MESSAGE, "Unknown token type: " + authTokenType);
+    return bundle;
   }
 
   @Override
   public String getAuthTokenLabel(String authTokenType) {
     Logger.debug(LOG_TAG, "getAuthTokenLabel");
 
     return null;
   }
--- a/mobile/android/base/fxa/sync/FxAccountSyncAdapter.java
+++ b/mobile/android/base/fxa/sync/FxAccountSyncAdapter.java
@@ -110,17 +110,17 @@ public class FxAccountSyncAdapter extend
     @Override
     public void rejectSync() {
       super.rejectSync();
     }
 
     protected final Collection<String> stageNamesToSync;
 
     public SyncDelegate(CountDownLatch latch, SyncResult syncResult, AndroidFxAccount fxAccount, Collection<String> stageNamesToSync) {
-      super(latch, syncResult, fxAccount);
+      super(latch, syncResult);
       this.stageNamesToSync = Collections.unmodifiableCollection(stageNamesToSync);
     }
 
     public Collection<String> getStageNamesToSync() {
       return this.stageNamesToSync;
     }
   }
 
@@ -229,28 +229,30 @@ public class FxAccountSyncAdapter extend
   protected void syncWithAssertion(final String audience,
                                    final String assertion,
                                    final URI tokenServerEndpointURI,
                                    final BackoffHandler tokenBackoffHandler,
                                    final SharedPreferences sharedPrefs,
                                    final KeyBundle syncKeyBundle,
                                    final String clientState,
                                    final SessionCallback callback,
-                                   final Bundle extras) {
+                                   final Bundle extras,
+                                   final AndroidFxAccount fxAccount) {
     final TokenServerClientDelegate delegate = new TokenServerClientDelegate() {
       private boolean didReceiveBackoff = false;
 
       @Override
       public String getUserAgent() {
         return FxAccountConstants.USER_AGENT;
       }
 
       @Override
       public void handleSuccess(final TokenServerToken token) {
         FxAccountUtils.pii(LOG_TAG, "Got token! uid is " + token.uid + " and endpoint is " + token.endpoint + ".");
+        fxAccount.releaseSharedAccountStateLock();
 
         if (!didReceiveBackoff) {
           // We must be OK to touch this token server.
           tokenBackoffHandler.setEarliestNextRequest(0L);
         }
 
         final URI storageServerURI;
         try {
@@ -318,22 +320,34 @@ public class FxAccountSyncAdapter extend
         } catch (Exception e) {
           callback.handleError(globalSession, e);
           return;
         }
       }
 
       @Override
       public void handleFailure(TokenServerException e) {
-        handleError(e);
+        Logger.error(LOG_TAG, "Failed to get token.", e);
+        try {
+          // We should only get here *after* we're locked into the married state.
+          State state = fxAccount.getState();
+          if (state.getStateLabel() == StateLabel.Married) {
+            Married married = (Married) state;
+            fxAccount.setState(married.makeCohabitingState());
+          }
+        } finally {
+          fxAccount.releaseSharedAccountStateLock();
+        }
+        callback.handleError(null, e);
       }
 
       @Override
       public void handleError(Exception e) {
         Logger.error(LOG_TAG, "Failed to get token.", e);
+        fxAccount.releaseSharedAccountStateLock();
         callback.handleError(null, e);
       }
 
       @Override
       public void handleBackoff(int backoffSeconds) {
         // This is the token server telling us to back off.
         Logger.info(LOG_TAG, "Token server requesting backoff of " + backoffSeconds + "s. Backoff handler: " + tokenBackoffHandler);
         didReceiveBackoff = true;
@@ -405,24 +419,16 @@ public class FxAccountSyncAdapter extend
     final CountDownLatch latch = new CountDownLatch(1);
 
     Collection<String> knownStageNames = SyncConfiguration.validEngineNames();
     Collection<String> stageNamesToSync = Utils.getStagesToSyncFromBundle(knownStageNames, extras);
 
     final SyncDelegate syncDelegate = new SyncDelegate(latch, syncResult, fxAccount, stageNamesToSync);
 
     try {
-      final State state;
-      try {
-        state = fxAccount.getState();
-      } catch (Exception e) {
-        syncDelegate.handleError(e);
-        return;
-      }
-
       // This will be the same chunk of SharedPreferences that we pass through to GlobalSession/SyncConfiguration.
       final SharedPreferences sharedPrefs = fxAccount.getSyncPrefs();
 
       final BackoffHandler backgroundBackoffHandler = new PrefsBackoffHandler(sharedPrefs, "background");
       final BackoffHandler rateLimitBackoffHandler = new PrefsBackoffHandler(sharedPrefs, "rate");
 
       // If this sync was triggered by user action, this will be true.
       final boolean isImmediate = (extras != null) &&
@@ -449,16 +455,34 @@ public class FxAccountSyncAdapter extend
       // Set a small scheduled 'backoff' to rate-limit the next sync,
       // and extend the background delay even further into the future.
       schedulePolicy.configureBackoffMillisBeforeSyncing(rateLimitBackoffHandler, backgroundBackoffHandler);
 
       final String tokenServerEndpoint = fxAccount.getTokenServerURI();
       final URI tokenServerEndpointURI = new URI(tokenServerEndpoint);
       final String audience = FxAccountUtils.getAudienceForURL(tokenServerEndpoint);
 
+      try {
+        // The clock starts... now!
+        fxAccount.acquireSharedAccountStateLock(FxAccountSyncAdapter.LOG_TAG);
+      } catch (InterruptedException e) {
+        // OK, skip this sync.
+        syncDelegate.handleError(e);
+        return;
+      }
+
+      final State state;
+      try {
+        state = fxAccount.getState();
+      } catch (Exception e) {
+        fxAccount.releaseSharedAccountStateLock();
+        syncDelegate.handleError(e);
+        return;
+      }
+
       final FxAccountLoginStateMachine stateMachine = new FxAccountLoginStateMachine();
       stateMachine.advance(state, StateLabel.Married, new FxADefaultLoginStateMachineDelegate(context, fxAccount) {
         @Override
         public void handleNotMarried(State notMarried) {
           Logger.info(LOG_TAG, "handleNotMarried: in " + notMarried.getStateLabel());
           schedulePolicy.onHandleFinal(notMarried.getNeededAction());
           syncDelegate.handleCannotSync(notMarried);
         }
@@ -500,26 +524,28 @@ public class FxAccountSyncAdapter extend
               Logger.info(LOG_TAG, "Not syncing (token server).");
               syncDelegate.postponeSync(tokenBackoffHandler.delayMilliseconds());
               return;
             }
 
             final SessionCallback sessionCallback = new SessionCallback(syncDelegate, schedulePolicy);
             final KeyBundle syncKeyBundle = married.getSyncKeyBundle();
             final String clientState = married.getClientState();
-            syncWithAssertion(audience, assertion, tokenServerEndpointURI, tokenBackoffHandler, sharedPrefs, syncKeyBundle, clientState, sessionCallback, extras);
+            syncWithAssertion(audience, assertion, tokenServerEndpointURI, tokenBackoffHandler, sharedPrefs, syncKeyBundle, clientState, sessionCallback, extras, fxAccount);
           } catch (Exception e) {
             syncDelegate.handleError(e);
             return;
           }
         }
       });
 
       latch.await();
     } catch (Exception e) {
       Logger.error(LOG_TAG, "Got error syncing.", e);
       syncDelegate.handleError(e);
+    } finally {
+      fxAccount.releaseSharedAccountStateLock();
     }
 
     Logger.info(LOG_TAG, "Syncing done.");
     lastSyncRealtimeMillis = SystemClock.elapsedRealtime();
   }
 }
--- a/mobile/android/base/fxa/sync/FxAccountSyncDelegate.java
+++ b/mobile/android/base/fxa/sync/FxAccountSyncDelegate.java
@@ -1,42 +1,33 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.fxa.sync;
 
 import java.util.concurrent.CountDownLatch;
 
-import org.mozilla.gecko.fxa.authenticator.AndroidFxAccount;
-import org.mozilla.gecko.fxa.login.Married;
 import org.mozilla.gecko.fxa.login.State;
-import org.mozilla.gecko.fxa.login.State.StateLabel;
-import org.mozilla.gecko.tokenserver.TokenServerException;
 
 import android.content.SyncResult;
 
 public class FxAccountSyncDelegate {
   protected final CountDownLatch latch;
   protected final SyncResult syncResult;
-  protected final AndroidFxAccount fxAccount;
 
-  public FxAccountSyncDelegate(CountDownLatch latch, SyncResult syncResult, AndroidFxAccount fxAccount) {
+  public FxAccountSyncDelegate(CountDownLatch latch, SyncResult syncResult) {
     if (latch == null) {
       throw new IllegalArgumentException("latch must not be null");
     }
     if (syncResult == null) {
       throw new IllegalArgumentException("syncResult must not be null");
     }
-    if (fxAccount == null) {
-      throw new IllegalArgumentException("fxAccount must not be null");
-    }
     this.latch = latch;
     this.syncResult = syncResult;
-    this.fxAccount = fxAccount;
   }
 
   /**
    * No error!  Say that we made progress.
    */
   protected void setSyncResultSuccess() {
     syncResult.stats.numUpdates += 1;
   }
@@ -60,26 +51,16 @@ public class FxAccountSyncDelegate {
 
   public void handleSuccess() {
     setSyncResultSuccess();
     latch.countDown();
   }
 
   public void handleError(Exception e) {
     setSyncResultSoftError();
-    // This is awful, but we need to propagate bad assertions back up the
-    // chain somehow, and this will do for now.
-    if (e instanceof TokenServerException) {
-      // We should only get here *after* we're locked into the married state.
-      State state = fxAccount.getState();
-      if (state.getStateLabel() == StateLabel.Married) {
-        Married married = (Married) state;
-        fxAccount.setState(married.makeCohabitingState());
-      }
-    }
     latch.countDown();
   }
 
   /**
    * When the login machine terminates, we might not be in the
    * <code>Married</code> state, and therefore we can't sync. This method
    * messages as much to the user.
    * <p>
--- a/mobile/android/base/reading/ReadingListSyncAdapter.java
+++ b/mobile/android/base/reading/ReadingListSyncAdapter.java
@@ -161,17 +161,17 @@ public class ReadingListSyncAdapter exte
   public void onPerformSync(final Account account, final Bundle extras, final String authority, final ContentProviderClient provider, final SyncResult syncResult) {
     Logger.setThreadLogTag(ReadingListConstants.GLOBAL_LOG_TAG);
     Logger.resetLogging();
 
     final Context context = getContext();
     final AndroidFxAccount fxAccount = new AndroidFxAccount(context, account);
 
     final CountDownLatch latch = new CountDownLatch(1);
-    final FxAccountSyncDelegate syncDelegate = new FxAccountSyncDelegate(latch, syncResult, fxAccount);
+    final FxAccountSyncDelegate syncDelegate = new FxAccountSyncDelegate(latch, syncResult);
 
     final AccountManager accountManager = AccountManager.get(context);
     // If we have an auth failure that requires user intervention, FxA will show system
     // notifications prompting the user to re-connect as it advances the internal account state.
     // true causes the auth token fetch to return null on failure immediately, rather than doing
     // Mysterious Internal Work to try to get the token.
     final boolean notifyAuthFailure = true;
     try {