From 3a59e47a0a0dbf10bd2053d3c2b875138374235f Mon Sep 17 00:00:00 2001
From: Mark Hammond <mhammond@skippinet.com.au>
Date: Fri, 23 Jan 2015 12:05:14 +1100
Subject: [PATCH] Bug 1121329 - fixes to promise handling in FxA and Hawk.
 r=ckarlof

---
 services/common/hawkclient.js                 | 13 +++++++----
 services/fxaccounts/FxAccountsClient.jsm      |  2 +-
 services/sync/modules/browserid_identity.js   | 23 +++++++++++++++----
 .../tests/unit/test_browserid_identity.js     |  1 +
 4 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/services/common/hawkclient.js b/services/common/hawkclient.js
index faef283805a..527e6f989e9 100644
--- a/services/common/hawkclient.js
+++ b/services/common/hawkclient.js
@@ -279,10 +279,15 @@ this.HawkClient.prototype = {
     };
 
     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;
diff --git a/services/fxaccounts/FxAccountsClient.jsm b/services/fxaccounts/FxAccountsClient.jsm
index 840a7a87c2b..4dfa897a71b 100644
--- a/services/fxaccounts/FxAccountsClient.jsm
+++ b/services/fxaccounts/FxAccountsClient.jsm
@@ -370,7 +370,7 @@ this.FxAccountsClient.prototype = {
             this,
             "fxaBackoffTimer"
            );
-	}
+        }
         deferred.reject(error);
       }
     );
diff --git a/services/sync/modules/browserid_identity.js b/services/sync/modules/browserid_identity.js
index c0efd366169..ef8d737d22c 100644
--- a/services/sync/modules/browserid_identity.js
+++ b/services/sync/modules/browserid_identity.js
@@ -512,7 +512,9 @@ this.BrowserIDManager.prototype = {
     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...
@@ -527,8 +529,7 @@ this.BrowserIDManager.prototype = {
     // 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 = () => {
@@ -593,7 +594,7 @@ this.BrowserIDManager.prototype = {
         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);
         }
 
@@ -628,6 +629,9 @@ this.BrowserIDManager.prototype = {
       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;
@@ -707,6 +711,17 @@ BrowserIDClusterManager.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
diff --git a/services/sync/tests/unit/test_browserid_identity.js b/services/sync/tests/unit/test_browserid_identity.js
index 4cf39d93fff..961a32fb855 100644
--- a/services/sync/tests/unit/test_browserid_identity.js
+++ b/services/sync/tests/unit/test_browserid_identity.js
@@ -95,6 +95,7 @@ add_task(function test_initialializeWithNoKeys() {
     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() {