Bug 1288480 Rework mozAddonManager error handling r=kmag a=kwierso CLOSED TREE
authorAndrew Swan <aswan@mozilla.com>
Thu, 21 Jul 2016 11:35:41 -0700
changeset 331152 10501352b0eef4ecf19dd4d5b1d307ebb212cf5e
parent 331151 6b180266ac16e3226be33319ff710ddfa85f5836
child 331153 e416493c5ebde4e001c0667526628a48099caaa3
push id9858
push userjlund@mozilla.com
push dateMon, 01 Aug 2016 14:37:10 +0000
treeherdermozilla-aurora@203106ef6cb6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag, kwierso
bugs1288480
milestone50.0a1
Bug 1288480 Rework mozAddonManager error handling r=kmag a=kwierso CLOSED TREE This ended up being a bigger change than I had hoped for but it updates the WebAPITask helper in amWebAPI.js so that errors returned from the parent process are immediately wrapped into Error objects from the content page. In this way, programming errors or other internal errors don't leak out to mozAddonManager users. The way the previous code managed window references using "this" was already a bit fussy, this patch only makes it worse. But I think this basic logical structure here is right and since this bug is responsible for widespread breakage, I'd like to get this checked in and then clean it up in a follow-up. MozReview-Commit-ID: 98PgbWKsHIN
toolkit/mozapps/extensions/amWebAPI.js
--- a/toolkit/mozapps/extensions/amWebAPI.js
+++ b/toolkit/mozapps/extensions/amWebAPI.js
@@ -36,23 +36,19 @@ const APIBroker = {
     let payload = message.data;
 
     switch (message.name) {
       case MSG_PROMISE_RESULT: {
         if (!this._promises.has(payload.callbackID)) {
           return;
         }
 
-        let { resolve, reject } = this._promises.get(payload.callbackID);
+        let resolve = this._promises.get(payload.callbackID);
         this._promises.delete(payload.callbackID);
-
-        if ("resolve" in payload)
-          resolve(payload.resolve);
-        else
-          reject(payload.reject);
+        resolve(payload);
         break;
       }
 
       case MSG_INSTALL_EVENT: {
         let install = this._installMap.get(payload.id);
         if (!install) {
           let err = new Error(`Got install event for unknown install ${payload.id}`);
           Cu.reportError(err);
@@ -66,20 +62,20 @@ const APIBroker = {
         if (this._eventListener) {
           this._eventListener(payload);
         }
       }
     }
   },
 
   sendRequest: function(type, ...args) {
-    return new Promise((resolve, reject) => {
+    return new Promise(resolve => {
       let callbackID = this._nextID++;
 
-      this._promises.set(callbackID, { resolve, reject });
+      this._promises.set(callbackID, resolve);
       Services.cpmm.sendAsyncMessage(MSG_PROMISE_REQUEST, { type, callbackID, args });
     });
   },
 
   setAddonListener(callback) {
     this._eventListener = callback;
     if (callback) {
       Services.cpmm.addMessageListener(MSG_ADDON_EVENT, this);
@@ -115,51 +111,69 @@ function AddonInstall(window, properties
   this.handlers = new Map();
 
   for (let key of Object.keys(properties)) {
     this[key] = properties[key];
   }
 }
 
 /**
- * API methods should return promises from the page, this is a simple wrapper
- * to make sure of that. It also automatically wraps objects when necessary.
+ * API methods all return promises from content.  They also follow a
+ * similar pattern of sending a request to the parent process, then
+ * wrapping the returned object or error appropriately for the page.
+ * We must take care only to wrap and reject with errors that are meant
+ * to be visible to content, and not internal errors.
+ * This function is a wrapper to handle the common bits.
+ * 
+ *   apiRequest is the name of the command to invoke in the parent process
+ *   apiArgs is a callable that takes the content-provided args for the
+ *           method and returns the arguments to send in the request to
+ *           the parent process.
+ *   if processor is non-null, it is called on the returned object to
+ *           convert the result from the parent process back to an
+ *           object appropriate for content.
+ *
+ * Both apiArgs and processor are called with "this" bound to the value
+ * that is held when the actual api method was called.
  */
-function WebAPITask(generator) {
+function WebAPITask(apiRequest, apiArgs, processor) {
   return function(...args) {
-    let task = Task.async(generator.bind(this));
-
     let win = this.window;
-
-    let wrapForContent = (obj) => {
-      if (obj instanceof Addon) {
-        return win.Addon._create(win, obj);
-      }
-      if (obj instanceof AddonInstall) {
-        return win.AddonInstall._create(win, obj);
-      }
-
-      return obj;
-    }
+    let boundApiArgs = apiArgs.bind(this);
+    let boundProcessor = processor ? processor.bind(this) : null;
 
     return new win.Promise((resolve, reject) => {
-      task(...args).then(wrapForContent)
-                   .then(resolve, reject);
+      Task.spawn(function* () {
+        let sendArgs = boundApiArgs(...args);
+        let result = yield APIBroker.sendRequest(apiRequest, ...sendArgs);
+        if ("reject" in result) {
+          let err = new win.Error(result.reject.message);
+          // We don't currently put any other properties onto Errors
+          // generated by mozAddonManager.  If/when we do, they will
+          // need to get copied here.
+          reject(err);
+          return;
+        }
+
+        let obj = result.resolve;
+        if (boundProcessor) {
+          obj = boundProcessor(obj);
+        }
+        resolve(obj);
+      }).catch(err => {
+        Cu.reportError(err);
+        reject(new win.Error("Unexpected internal error"));
+      });
     });
   }
 }
 
 Addon.prototype = {
-  uninstall: WebAPITask(function*() {
-    return yield APIBroker.sendRequest("addonUninstall", this.id);
-  }),
-
-  setEnabled: WebAPITask(function* (value) {
-    return yield APIBroker.sendRequest("addonSetEnabled", this.id, value);
-  }),
+  uninstall: WebAPITask("addonUninstall", function() { return [this.id]; }),
+  setEnabled: WebAPITask("addonSetEnabled", function(value) {return [this.id, value]; }),
 };
 
 const INSTALL_EVENTS = [
   "onDownloadStarted",
   "onDownloadProgress",
   "onDownloadEnded",
   "onDownloadCancelled",
   "onDownloadFailed",
@@ -176,52 +190,49 @@ AddonInstall.prototype = {
     for (let key of Object.keys(data)) {
       this[key] = data[key];
     }
 
     let event = new this.window.Event(data.event);
     this.__DOM_IMPL__.dispatchEvent(event);
   },
 
-  install: WebAPITask(function*() {
-    yield APIBroker.sendRequest("addonInstallDoInstall", this.id);
-  }),
-
-  cancel: WebAPITask(function*() {
-    yield APIBroker.sendRequest("addonInstallCancel", this.id);
-  }),
+  install: WebAPITask("addonInstallDoInstall", function() { return  [this.id]; }),
+  cancel: WebAPITask("addonInstallCancel", function() { return  [this.id]; }),
 };
 
 function WebAPI() {
 }
 
 WebAPI.prototype = {
   init(window) {
     this.window = window;
     this.allInstalls = [];
     this.listenerCount = 0;
 
     window.addEventListener("unload", event => {
       APIBroker.sendCleanup(this.allInstalls);
     });
   },
 
-  getAddonByID: WebAPITask(function*(id) {
-    let addonInfo = yield APIBroker.sendRequest("getAddonByID", id);
-    return addonInfo ? new Addon(this.window, addonInfo) : null;
+  getAddonByID: WebAPITask("getAddonByID", id => [id], function(addonInfo) {
+    if (!addonInfo) {
+      return null;
+    }
+    let addon = new Addon(this.window, addonInfo);
+    return this.window.Addon._create(this.window, addon);
   }),
 
-  createInstall: WebAPITask(function*(options) {
-    let installInfo = yield APIBroker.sendRequest("createInstall", options);
+  createInstall: WebAPITask("createInstall", options => [options], function(installInfo) {
     if (!installInfo) {
       return null;
     }
     let install = new AddonInstall(this.window, installInfo);
     this.allInstalls.push(installInfo.id);
-    return install;
+    return this.window.AddonInstall._create(this.window, install);
   }),
 
   eventListenerWasAdded(type) {
     if (this.listenerCount == 0) {
       APIBroker.setAddonListener(data => {
         let event = new this.window.AddonEvent(data.event, data);
         this.__DOM_IMPL__.dispatchEvent(event);
       });