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

--HG--
extra : source : f2cd195ea3898c9c46e7f58bbcaa3292a2793554

toolkit/mozapps/extensions/amWebAPI.js

@@ -41,13 +41,9 @@ const APIBroker = {
           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;
       }
 
@@ -71,10 +67,10 @@ const APIBroker = {
   },
 
   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 });
     });
   },
@@ -120,41 +116,59 @@ function AddonInstall(window, properties) {
 }
 
 /**
- * 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 = [
@@ -181,13 +195,8 @@ AddonInstall.prototype = {
     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() {
@@ -204,19 +213,21 @@ WebAPI.prototype = {
     });
   },
 
-  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) {