Bug 1121329 - fixes to promise handling in FxA and Hawk. r=ckarlof
authorMark Hammond <mhammond@skippinet.com.au>
Wed, 28 Jan 2015 10:11:08 +1100
changeset 253354 48821a0e6f729b851a61056b5a38f710da0f61ac
parent 253353 d8a317f73bbc2e79177a88a932da60ce98c17ec0
child 253355 378edc6d1ff981e94e0bd7662db5aebc685aa7e4
push id4610
push userjlund@mozilla.com
push dateMon, 30 Mar 2015 18:32:55 +0000
treeherdermozilla-beta@4df54044d9ef [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersckarlof
bugs1121329
milestone38.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 1121329 - fixes to promise handling in FxA and Hawk. r=ckarlof
services/common/hawkclient.js
services/fxaccounts/FxAccountsClient.jsm
services/sync/modules/browserid_identity.js
services/sync/tests/unit/test_browserid_identity.js
--- a/services/common/hawkclient.js
+++ b/services/common/hawkclient.js
@@ -257,20 +257,25 @@ this.HawkClient.prototype = {
     }
 
     let extra = {
       now: this.now(),
       localtimeOffsetMsec: this.localtimeOffsetMsec,
     };
 
     let request = this.newHAWKAuthenticatedRESTRequest(uri, credentials, extra);
-    if (method == "post" || method == "put" || method == "patch") {
-      request[method](payloadObj, onComplete);
-    } else {
-      request[method](onComplete);
+    try {
+      if (method == "post" || method == "put" || method == "patch") {
+        request[method](payloadObj, onComplete);
+      } else {
+        request[method](onComplete);
+      }
+    } catch (ex) {
+      log.error("Failed to make hawk request", ex);
+      deferred.reject(ex);
     }
 
     return deferred.promise;
   },
 
   /*
    * The prefix used for all notifications sent by this module.  This
    * allows the handler of notifications to be sure they are handling
--- a/services/fxaccounts/FxAccountsClient.jsm
+++ b/services/fxaccounts/FxAccountsClient.jsm
@@ -365,17 +365,17 @@ this.FxAccountsClient.prototype = {
           this.backoffError = error;
           // Schedule clearing of cached-error-as-flag.
           CommonUtils.namedTimer(
             this._clearBackoff,
             error.retryAfter * 1000,
             this,
             "fxaBackoffTimer"
            );
-	}
+        }
         deferred.reject(error);
       }
     );
 
     return deferred.promise;
   },
 };
 
--- a/services/sync/modules/browserid_identity.js
+++ b/services/sync/modules/browserid_identity.js
@@ -507,33 +507,34 @@ this.BrowserIDManager.prototype = {
       return false;
     }
     if (this._token.expiration < this._now()) {
       return false;
     }
     return true;
   },
 
-  // Refresh the sync token for our user.
+  // Refresh the sync token for our user. Returns a promise that resolves
+  // with a token (which may be null in one sad edge-case), or rejects with an
+  // error.
   _fetchTokenForUser: function() {
     let tokenServerURI = Svc.Prefs.get("tokenServerURI");
     if (tokenServerURI.endsWith("/")) { // trailing slashes cause problems...
       tokenServerURI = tokenServerURI.slice(0, -1);
     }
     let log = this._log;
     let client = this._tokenServerClient;
     let fxa = this._fxaService;
     let userData = this._signedInUser;
 
     // We need kA and kB for things to work.  If we don't have them, just
     // return null for the token - sync calling unlockAndVerifyAuthState()
     // before actually syncing will setup the error states if necessary.
     if (!this._canFetchKeys()) {
-      log.info("_fetchTokenForUser has no keys to use.");
-      return null;
+      return Promise.resolve(null);
     }
 
     log.info("Fetching assertion and token from: " + tokenServerURI);
 
     let maybeFetchKeys = () => {
       // This is called at login time and every time we need a new token - in
       // the latter case we already have kA and kB, so optimise that case.
       if (userData.kA && userData.kB) {
@@ -589,17 +590,17 @@ this.BrowserIDManager.prototype = {
       })
       .then(null, err => {
         // TODO: unify these errors - we need to handle errors thrown by
         // both tokenserverclient and hawkclient.
         // A tokenserver error thrown based on a bad response.
         if (err.response && err.response.status === 401) {
           err = new AuthenticationError(err);
         // A hawkclient error.
-        } else if (err.code === 401) {
+        } else if (err.code && err.code === 401) {
           err = new AuthenticationError(err);
         }
 
         // TODO: write tests to make sure that different auth error cases are handled here
         // properly: auth error getting assertion, auth error getting token (invalid generation
         // and client-state error)
         if (err instanceof AuthenticationError) {
           this._log.error("Authentication error in _fetchTokenForUser: " + err);
@@ -624,16 +625,19 @@ this.BrowserIDManager.prototype = {
 
   // Returns a promise that is resolved when we have a valid token for the
   // current user stored in this._token.  When resolved, this._token is valid.
   _ensureValidToken: function() {
     if (this.hasValidToken()) {
       this._log.debug("_ensureValidToken already has one");
       return Promise.resolve();
     }
+    // reset this._token as a safety net to reduce the possibility of us
+    // repeatedly attempting to use an invalid token if _fetchTokenForUser throws.
+    this._token = null;
     return this._fetchTokenForUser().then(
       token => {
         this._token = token;
       }
     );
   },
 
   getResourceAuthenticator: function () {
@@ -703,16 +707,27 @@ function BrowserIDClusterManager(service
   ClusterManager.call(this, service);
 }
 
 BrowserIDClusterManager.prototype = {
   __proto__: ClusterManager.prototype,
 
   _findCluster: function() {
     let endPointFromIdentityToken = function() {
+      // The only reason (in theory ;) that we can end up with a null token
+      // is when this.identity._canFetchKeys() returned false.  In turn, this
+      // should only happen if the master-password is locked or the credentials
+      // storage is screwed, and in those cases we shouldn't have started
+      // syncing so shouldn't get here anyway.
+      // But better safe than sorry! To keep things clearer, throw an explicit
+      // exception - the message will appear in the logs and the error will be
+      // treated as transient.
+      if (!this.identity._token) {
+        throw new Error("Can't get a cluster URL as we can't fetch keys.");
+      }
       let endpoint = this.identity._token.endpoint;
       // For Sync 1.5 storage endpoints, we use the base endpoint verbatim.
       // However, it should end in "/" because we will extend it with
       // well known path components. So we add a "/" if it's missing.
       if (!endpoint.endsWith("/")) {
         endpoint += "/";
       }
       log.debug("_findCluster returning " + endpoint);
--- a/services/sync/tests/unit/test_browserid_identity.js
+++ b/services/sync/tests/unit/test_browserid_identity.js
@@ -90,16 +90,17 @@ add_task(function test_initialializeWith
     delete identityConfig.fxaccount.user.kB;
     // there's no keyFetchToken by default, so the initialize should fail.
     configureFxAccountIdentity(browseridManager, identityConfig);
 
     yield browseridManager.initializeWithCurrentIdentity();
     yield browseridManager.whenReadyToAuthenticate.promise;
     do_check_eq(Status.login, LOGIN_SUCCEEDED, "login succeeded even without keys");
     do_check_false(browseridManager._canFetchKeys(), "_canFetchKeys reflects lack of keys");
+    do_check_eq(browseridManager._token, null, "we don't have a token");
 });
 
 add_test(function test_getResourceAuthenticator() {
     _("BrowserIDManager supplies a Resource Authenticator callback which returns a Hawk header.");
     configureFxAccountIdentity(browseridManager);
     let authenticator = browseridManager.getResourceAuthenticator();
     do_check_true(!!authenticator);
     let req = {uri: CommonUtils.makeURI(