Bug 1121329 - fixes to promise handling in FxA and Hawk. r=ckarlof
☠☠ backed out by 2a2c901bd4d2 ☠ ☠
authorMark Hammond <mhammond@skippinet.com.au>
Fri, 23 Jan 2015 12:05:14 +1100
changeset 252367 316298f580fd1649e8a180abd8aa231d792a944e
parent 252366 a7e7d68537a87b3282cceab5d9ec5fe5fd3da5ea
child 252368 6fc6668a3da60786a4ff026e6b9e7c255dfb37fe
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
@@ -274,20 +274,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);
     }
 
     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) {
         return;
       }
@@ -588,17 +589,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);
@@ -623,16 +624,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 () {
@@ -702,16 +706,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(