Bug 1255302 - Report FxA WebChannel message handling errors to the sender. r?markh draft
authorKit Cambridge <kcambridge@mozilla.com>
Wed, 06 Apr 2016 23:02:26 -0700
changeset 348701 1096806a4598d9f5d073f73484ad2fe6f5e798aa
parent 348506 b6683e141c47c022598c0caac3ea8ba8c6236d42
child 517909 cd8eb01d4c2bb95aff7df418f7710c99ae6da018
push id14891
push userkcambridge@mozilla.com
push dateThu, 07 Apr 2016 22:55:28 +0000
reviewersmarkh
bugs1255302
milestone48.0a1
Bug 1255302 - Report FxA WebChannel message handling errors to the sender. r?markh MozReview-Commit-ID: IKPxqdqnhnE
services/fxaccounts/FxAccountsWebChannel.jsm
services/fxaccounts/tests/xpcshell/test_web_channel.js
--- a/services/fxaccounts/FxAccountsWebChannel.jsm
+++ b/services/fxaccounts/FxAccountsWebChannel.jsm
@@ -34,16 +34,36 @@ const COMMAND_LOGOUT               = "fx
 const COMMAND_DELETE               = "fxaccounts:delete";
 const COMMAND_SYNC_PREFERENCES     = "fxaccounts:sync_preferences";
 const COMMAND_CHANGE_PASSWORD      = "fxaccounts:change_password";
 
 const PREF_LAST_FXA_USER           = "identity.fxaccounts.lastSignedInUserHash";
 const PREF_SYNC_SHOW_CUSTOMIZATION = "services.sync-setup.ui.showCustomizationDialog";
 
 /**
+ * A helper function that extracts the message and stack from an error object.
+ * Returns a `{ message, stack }` tuple. `stack` will be null if the error
+ * doesn't have a stack trace.
+ */
+function getErrorDetails(error) {
+  let details = { message: String(error), stack: null };
+
+  // Adapted from Console.jsm.
+  if (error.stack) {
+    let frames = [];
+    for (let frame = error.stack; frame; frame = frame.caller) {
+      frames.push(String(frame).padStart(4));
+    }
+    details.stack = frames.join("\n");
+  }
+
+  return details;
+}
+
+/**
  * Create a new FxAccountsWebChannel to listen for account updates
  *
  * @param {Object} options Options
  *   @param {Object} options
  *     @param {String} options.content_uri
  *     The FxA Content server uri
  *     @param {String} options.channel_id
  *     The ID of the WebChannel
@@ -111,16 +131,69 @@ this.FxAccountsWebChannel.prototype = {
       this._webChannelOrigin = Services.io.newURI(this._contentUri, null, null);
       this._registerChannel();
     } catch (e) {
       log.error(e);
       throw e;
     }
   },
 
+  _receiveMessage(message, sendingContext) {
+    let command = message.command;
+    let data = message.data;
+
+    switch (command) {
+      case COMMAND_PROFILE_CHANGE:
+        Services.obs.notifyObservers(null, ON_PROFILE_CHANGE_NOTIFICATION, data.uid);
+        break;
+      case COMMAND_LOGIN:
+        this._helpers.login(data).catch(error =>
+          this._sendError(error, message, sendingContext));
+        break;
+      case COMMAND_LOGOUT:
+      case COMMAND_DELETE:
+        this._helpers.logout(data.uid).catch(error =>
+          this._sendError(error, message, sendingContext));
+        break;
+      case COMMAND_CAN_LINK_ACCOUNT:
+        let canLinkAccount = this._helpers.shouldAllowRelink(data.email);
+
+        let response = {
+          command: command,
+          messageId: message.messageId,
+          data: { ok: canLinkAccount }
+        };
+
+        log.debug("FxAccountsWebChannel response", response);
+        this._channel.send(response, sendingContext);
+        break;
+      case COMMAND_SYNC_PREFERENCES:
+        this._helpers.openSyncPreferences(sendingContext.browser, data.entryPoint);
+        break;
+      case COMMAND_CHANGE_PASSWORD:
+        this._helpers.changePassword(data).catch(error =>
+          this._sendError(error, message, sendingContext));
+        break;
+      default:
+        log.warn("Unrecognized FxAccountsWebChannel command", command);
+        break;
+    }
+  },
+
+  _sendError(error, incomingMessage, sendingContext) {
+    log.error("Failed to handle FxAccountsWebChannel message", error);
+    this._channel.send({
+      command: incomingMessage.command,
+      messageId: incomingMessage.messageId,
+      data: {
+        error: getErrorDetails(error),
+      },
+    }, sendingContext);
+  },
+
   /**
    * Create a new channel with the WebChannelBroker, setup a callback listener
    * @private
    */
   _registerChannel() {
     /**
      * Processes messages that are called back from the FxAccountsChannel
      *
@@ -141,51 +214,20 @@ this.FxAccountsWebChannel.prototype = {
      *
      */
     let listener = (webChannelId, message, sendingContext) => {
       if (message) {
         log.debug("FxAccountsWebChannel message received", message.command);
         if (logPII) {
           log.debug("FxAccountsWebChannel message details", message);
         }
-        let command = message.command;
-        let data = message.data;
-
-        switch (command) {
-          case COMMAND_PROFILE_CHANGE:
-            Services.obs.notifyObservers(null, ON_PROFILE_CHANGE_NOTIFICATION, data.uid);
-            break;
-          case COMMAND_LOGIN:
-            this._helpers.login(data);
-            break;
-          case COMMAND_LOGOUT:
-          case COMMAND_DELETE:
-            this._helpers.logout(data.uid);
-            break;
-          case COMMAND_CAN_LINK_ACCOUNT:
-            let canLinkAccount = this._helpers.shouldAllowRelink(data.email);
-
-            let response = {
-              command: command,
-              messageId: message.messageId,
-              data: { ok: canLinkAccount }
-            };
-
-            log.debug("FxAccountsWebChannel response", response);
-            this._channel.send(response, sendingContext);
-            break;
-          case COMMAND_SYNC_PREFERENCES:
-            this._helpers.openSyncPreferences(sendingContext.browser, data.entryPoint);
-            break;
-          case COMMAND_CHANGE_PASSWORD:
-            this._helpers.changePassword(data);
-            break;
-          default:
-            log.warn("Unrecognized FxAccountsWebChannel command", command);
-            break;
+        try {
+          this._receiveMessage(message, sendingContext);
+        } catch (error) {
+          this._sendError(error, message, sendingContext);
         }
       }
     };
 
     this._channelCallback = listener;
     this._channel = new WebChannel(this._webChannelId, this._webChannelOrigin);
     this._channel.listen(listener);
     log.debug("FxAccountsWebChannel registered: " + this._webChannelId + " with origin " + this._webChannelOrigin.prePath);
@@ -294,19 +336,17 @@ this.FxAccountsWebChannelHelpers.prototy
     let newCredentials = {};
     for (let name of Object.keys(credentials)) {
       if (name == "email" || name == "uid" || FxAccountsStorageManagerCanStoreField(name)) {
         newCredentials[name] = credentials[name];
       } else {
         log.info("changePassword ignoring unsupported field", name);
       }
     }
-    this._fxAccounts.updateUserAccountData(newCredentials).catch(err => {
-      log.error("Failed to update account data on password change", err);
-    });
+    return this._fxAccounts.updateUserAccountData(newCredentials);
   },
 
   /**
    * Get the hash of account name of the previously signed in account
    */
   getPreviousAccountNameHashPref() {
     try {
       return Services.prefs.getComplexValue(PREF_LAST_FXA_USER, Ci.nsISupportsString).data;
--- a/services/fxaccounts/tests/xpcshell/test_web_channel.js
+++ b/services/fxaccounts/tests/xpcshell/test_web_channel.js
@@ -33,16 +33,90 @@ add_test(function () {
   validationHelper({
     content_uri: URL_STRING
   },
   'Error: Missing \'channel_id\' option');
 
   run_next_test();
 });
 
+add_task(function* test_rejection_reporting() {
+  let mockMessage = {
+    command: 'fxaccounts:login',
+    messageId: '1234',
+    data: { email: 'testuser@testuser.com' },
+  };
+
+  let channel = new FxAccountsWebChannel({
+    channel_id: WEBCHANNEL_ID,
+    content_uri: URL_STRING,
+    helpers: {
+      login(accountData) {
+        equal(accountData.email, 'testuser@testuser.com',
+          'Should forward incoming message data to the helper');
+        return Promise.reject(new Error('oops'));
+      },
+    },
+  });
+
+  let promiseSend = new Promise(resolve => {
+    channel._channel.send = (message, context) => {
+      resolve({ message, context });
+    };
+  });
+
+  channel._channelCallback(WEBCHANNEL_ID, mockMessage, mockSendingContext);
+
+  let { message, context } = yield promiseSend;
+
+  equal(context, mockSendingContext, 'Should forward the original context');
+  equal(message.command, 'fxaccounts:login',
+    'Should include the incoming command');
+  equal(message.messageId, '1234', 'Should include the message ID');
+  equal(message.data.error.message, 'Error: oops',
+    'Should convert the error message to a string');
+  notStrictEqual(message.data.error.stack, null,
+    'Should include the stack for JS error rejections');
+});
+
+add_test(function test_exception_reporting() {
+  let mockMessage = {
+    command: 'fxaccounts:sync_preferences',
+    messageId: '5678',
+    data: { entryPoint: 'fxa:verification_complete' }
+  };
+
+  let channel = new FxAccountsWebChannel({
+    channel_id: WEBCHANNEL_ID,
+    content_uri: URL_STRING,
+    helpers: {
+      openSyncPreferences(browser, entryPoint) {
+        equal(entryPoint, 'fxa:verification_complete',
+          'Should forward incoming message data to the helper');
+        throw new TypeError('splines not reticulated');
+      },
+    },
+  });
+
+  channel._channel.send = (message, context) => {
+    equal(context, mockSendingContext, 'Should forward the original context');
+    equal(message.command, 'fxaccounts:sync_preferences',
+      'Should include the incoming command');
+    equal(message.messageId, '5678', 'Should include the message ID');
+    equal(message.data.error.message, 'TypeError: splines not reticulated',
+      'Should convert the exception to a string');
+    notStrictEqual(message.data.error.stack, null,
+      'Should include the stack for JS exceptions');
+
+    run_next_test();
+  };
+
+  channel._channelCallback(WEBCHANNEL_ID, mockMessage, mockSendingContext);
+});
+
 add_test(function test_profile_image_change_message() {
   var mockMessage = {
     command: "profile:change",
     data: { uid: "foo" }
   };
 
   makeObserver(ON_PROFILE_CHANGE_NOTIFICATION, function (subject, topic, data) {
     do_check_eq(data, "foo");
@@ -65,16 +139,17 @@ add_test(function test_login_message() {
 
   let channel = new FxAccountsWebChannel({
     channel_id: WEBCHANNEL_ID,
     content_uri: URL_STRING,
     helpers: {
       login: function (accountData) {
         do_check_eq(accountData.email, 'testuser@testuser.com');
         run_next_test();
+        return Promise.resolve();
       }
     }
   });
 
   channel._channelCallback(WEBCHANNEL_ID, mockMessage, mockSendingContext);
 });
 
 add_test(function test_logout_message() {
@@ -85,16 +160,17 @@ add_test(function test_logout_message() 
 
   let channel = new FxAccountsWebChannel({
     channel_id: WEBCHANNEL_ID,
     content_uri: URL_STRING,
     helpers: {
       logout: function (uid) {
         do_check_eq(uid, 'foo');
         run_next_test();
+        return Promise.resolve();
       }
     }
   });
 
   channel._channelCallback(WEBCHANNEL_ID, mockMessage, mockSendingContext);
 });
 
 add_test(function test_delete_message() {
@@ -105,16 +181,17 @@ add_test(function test_delete_message() 
 
   let channel = new FxAccountsWebChannel({
     channel_id: WEBCHANNEL_ID,
     content_uri: URL_STRING,
     helpers: {
       logout: function (uid) {
         do_check_eq(uid, 'foo');
         run_next_test();
+        return Promise.resolve();
       }
     }
   });
 
   channel._channelCallback(WEBCHANNEL_ID, mockMessage, mockSendingContext);
 });
 
 add_test(function test_can_link_account_message() {
@@ -194,113 +271,119 @@ add_test(function test_helpers_should_al
   };
 
   do_check_true(helpers.shouldAllowRelink('allowed_to_relink@testuser.com'));
   do_check_false(helpers.shouldAllowRelink('not_allowed_to_relink@testuser.com'));
 
   run_next_test();
 });
 
-add_test(function test_helpers_login_without_customize_sync() {
+add_task(function* test_helpers_login_without_customize_sync() {
   let helpers = new FxAccountsWebChannelHelpers({
     fxAccounts: {
       setSignedInUser: function(accountData) {
-        // ensure fxAccounts is informed of the new user being signed in.
-        do_check_eq(accountData.email, 'testuser@testuser.com');
+        return new Promise(resolve => {
+          // ensure fxAccounts is informed of the new user being signed in.
+          do_check_eq(accountData.email, 'testuser@testuser.com');
 
-        // verifiedCanLinkAccount should be stripped in the data.
-        do_check_false('verifiedCanLinkAccount' in accountData);
+          // verifiedCanLinkAccount should be stripped in the data.
+          do_check_false('verifiedCanLinkAccount' in accountData);
 
-        // the customizeSync pref should not update
-        do_check_false(helpers.getShowCustomizeSyncPref());
+          // the customizeSync pref should not update
+          do_check_false(helpers.getShowCustomizeSyncPref());
 
-        // previously signed in user preference is updated.
-        do_check_eq(helpers.getPreviousAccountNameHashPref(), helpers.sha256('testuser@testuser.com'));
+          // previously signed in user preference is updated.
+          do_check_eq(helpers.getPreviousAccountNameHashPref(), helpers.sha256('testuser@testuser.com'));
 
-        run_next_test();
+          resolve();
+        });
       }
     }
   });
 
   // the show customize sync pref should stay the same
   helpers.setShowCustomizeSyncPref(false);
 
   // ensure the previous account pref is overwritten.
   helpers.setPreviousAccountNameHashPref('lastuser@testuser.com');
 
-  helpers.login({
+  yield helpers.login({
     email: 'testuser@testuser.com',
     verifiedCanLinkAccount: true,
     customizeSync: false
   });
 });
 
-add_test(function test_helpers_login_with_customize_sync() {
+add_task(function* test_helpers_login_with_customize_sync() {
   let helpers = new FxAccountsWebChannelHelpers({
     fxAccounts: {
       setSignedInUser: function(accountData) {
-        // ensure fxAccounts is informed of the new user being signed in.
-        do_check_eq(accountData.email, 'testuser@testuser.com');
+        return new Promise(resolve => {
+          // ensure fxAccounts is informed of the new user being signed in.
+          do_check_eq(accountData.email, 'testuser@testuser.com');
 
-        // customizeSync should be stripped in the data.
-        do_check_false('customizeSync' in accountData);
+          // customizeSync should be stripped in the data.
+          do_check_false('customizeSync' in accountData);
 
-        // the customizeSync pref should not update
-        do_check_true(helpers.getShowCustomizeSyncPref());
+          // the customizeSync pref should not update
+          do_check_true(helpers.getShowCustomizeSyncPref());
 
-        run_next_test();
+          resolve();
+        });
       }
     }
   });
 
   // the customize sync pref should be overwritten
   helpers.setShowCustomizeSyncPref(false);
 
-  helpers.login({
+  yield helpers.login({
     email: 'testuser@testuser.com',
     verifiedCanLinkAccount: true,
     customizeSync: true
   });
 });
 
-add_test(function test_helpers_login_with_customize_sync_and_declined_engines() {
+add_task(function* test_helpers_login_with_customize_sync_and_declined_engines() {
   let helpers = new FxAccountsWebChannelHelpers({
     fxAccounts: {
       setSignedInUser: function(accountData) {
-        // ensure fxAccounts is informed of the new user being signed in.
-        do_check_eq(accountData.email, 'testuser@testuser.com');
+        return new Promise(resolve => {
+          // ensure fxAccounts is informed of the new user being signed in.
+          do_check_eq(accountData.email, 'testuser@testuser.com');
 
-        // customizeSync should be stripped in the data.
-        do_check_false('customizeSync' in accountData);
-        do_check_false('declinedSyncEngines' in accountData);
-        do_check_eq(Services.prefs.getBoolPref("services.sync.engine.addons"), false);
-        do_check_eq(Services.prefs.getBoolPref("services.sync.engine.bookmarks"), true);
-        do_check_eq(Services.prefs.getBoolPref("services.sync.engine.history"), true);
-        do_check_eq(Services.prefs.getBoolPref("services.sync.engine.passwords"), true);
-        do_check_eq(Services.prefs.getBoolPref("services.sync.engine.prefs"), false);
-        do_check_eq(Services.prefs.getBoolPref("services.sync.engine.tabs"), true);
+          // customizeSync should be stripped in the data.
+          do_check_false('customizeSync' in accountData);
+          do_check_false('declinedSyncEngines' in accountData);
+          do_check_eq(Services.prefs.getBoolPref("services.sync.engine.addons"), false);
+          do_check_eq(Services.prefs.getBoolPref("services.sync.engine.bookmarks"), true);
+          do_check_eq(Services.prefs.getBoolPref("services.sync.engine.history"), true);
+          do_check_eq(Services.prefs.getBoolPref("services.sync.engine.passwords"), true);
+          do_check_eq(Services.prefs.getBoolPref("services.sync.engine.prefs"), false);
+          do_check_eq(Services.prefs.getBoolPref("services.sync.engine.tabs"), true);
 
-        // the customizeSync pref should be disabled
-        do_check_false(helpers.getShowCustomizeSyncPref());
+          // the customizeSync pref should be disabled
+          do_check_false(helpers.getShowCustomizeSyncPref());
 
-        run_next_test();
+          resolve();
+        });
       }
     }
   });
 
   // the customize sync pref should be overwritten
   helpers.setShowCustomizeSyncPref(true);
 
   do_check_eq(Services.prefs.getBoolPref("services.sync.engine.addons"), true);
   do_check_eq(Services.prefs.getBoolPref("services.sync.engine.bookmarks"), true);
   do_check_eq(Services.prefs.getBoolPref("services.sync.engine.history"), true);
   do_check_eq(Services.prefs.getBoolPref("services.sync.engine.passwords"), true);
   do_check_eq(Services.prefs.getBoolPref("services.sync.engine.prefs"), true);
   do_check_eq(Services.prefs.getBoolPref("services.sync.engine.tabs"), true);
-  helpers.login({
+  yield helpers.login({
     email: 'testuser@testuser.com',
     verifiedCanLinkAccount: true,
     customizeSync: true,
     declinedSyncEngines: ['addons', 'prefs']
   });
 });
 
 add_test(function test_helpers_open_sync_preferences() {
@@ -314,34 +397,36 @@ add_test(function test_helpers_open_sync
       do_check_eq(uri, "about:preferences?entrypoint=fxa%3Averification_complete#sync");
       run_next_test();
     }
   };
 
   helpers.openSyncPreferences(mockBrowser, "fxa:verification_complete");
 });
 
-add_test(function test_helpers_change_password() {
+add_task(function* test_helpers_change_password() {
   let updateCalled = false;
   let helpers = new FxAccountsWebChannelHelpers({
     fxAccounts: {
       updateUserAccountData(credentials) {
-        do_check_true(credentials.hasOwnProperty("email"));
-        do_check_true(credentials.hasOwnProperty("uid"));
-        do_check_true(credentials.hasOwnProperty("kA"));
-        // "foo" isn't a field known by storage, so should be dropped.
-        do_check_false(credentials.hasOwnProperty("foo"));
-        updateCalled = true;
-        return Promise.resolve();
+        return new Promise(resolve => {
+          do_check_true(credentials.hasOwnProperty("email"));
+          do_check_true(credentials.hasOwnProperty("uid"));
+          do_check_true(credentials.hasOwnProperty("kA"));
+          // "foo" isn't a field known by storage, so should be dropped.
+          do_check_false(credentials.hasOwnProperty("foo"));
+          updateCalled = true;
+
+          resolve();
+        });
       }
     }
   });
-  helpers.changePassword({ email: "email", uid: "uid", kA: "kA", foo: "foo" });
+  yield helpers.changePassword({ email: "email", uid: "uid", kA: "kA", foo: "foo" });
   do_check_true(updateCalled);
-  run_next_test();
 });
 
 function run_test() {
   run_next_test();
 }
 
 function makeObserver(aObserveTopic, aObserveFunc) {
   let callback = function (aSubject, aTopic, aData) {