Bug 1190680: Part 2 - [webext] Add initial support for lastError callbacks. r=billm
authorKris Maglione <maglione.k@gmail.com>
Fri, 29 Jan 2016 18:38:08 -0800
changeset 282374 32faeeebe2e0b4a1c667b16781ac4d0b6cde62e5
parent 282373 0573e9dfa2b8fbe1c01426768b1f0edfa2f1153d
child 282375 38874d98cf4fffcfb4b208de8667d17c8312f9cb
push id29957
push userphilringnalda@gmail.com
push dateSat, 30 Jan 2016 17:15:45 +0000
treeherdermozilla-central@8be8932a2e0f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbillm
bugs1190680
milestone47.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 1190680: Part 2 - [webext] Add initial support for lastError callbacks. r=billm
browser/components/extensions/ext-tabs.js
browser/components/extensions/test/browser/browser.ini
browser/components/extensions/test/browser/browser_ext_lastError.js
browser/components/extensions/test/browser/browser_ext_tabs_executeScript_bad.js
toolkit/components/extensions/Extension.jsm
toolkit/components/extensions/ExtensionContent.jsm
toolkit/components/extensions/ExtensionUtils.jsm
toolkit/components/extensions/MessageChannel.jsm
toolkit/components/extensions/Schemas.jsm
toolkit/components/extensions/ext-extension.js
toolkit/components/extensions/ext-runtime.js
toolkit/components/extensions/schemas/extension.json
toolkit/components/extensions/schemas/runtime.json
--- a/browser/components/extensions/ext-tabs.js
+++ b/browser/components/extensions/ext-tabs.js
@@ -461,17 +461,17 @@ extensions.registerSchemaAPI("tabs", nul
             if (matches(window, tab)) {
               result.push(tab);
             }
           }
         }
         runSafe(context, callback, result);
       },
 
-      _execute: function(tabId, details, kind, callback) {
+      _execute: function(tabId, details, kind) {
         let tab = tabId !== null ? TabManager.getTab(tabId) : TabManager.activeTab;
         let mm = tab.linkedBrowser.messageManager;
 
         let options = {
           js: [],
           css: [],
         };
 
@@ -487,47 +487,43 @@ extensions.registerSchemaAPI("tabs", nul
           options.matchesHost = extension.whiteListedHosts.serialize();
         }
 
         if (details.code !== null) {
           options[kind + "Code"] = details.code;
         }
         if (details.file !== null) {
           let url = context.uri.resolve(details.file);
-          if (extension.isExtensionURL(url)) {
-            // We should really set |lastError| here, and go straight to
-            // the callback, but we don't have |lastError| yet.
-            options[kind].push(url);
+          if (!extension.isExtensionURL(url)) {
+            return Promise.reject({ message: "Files to be injected must be within the extension" });
           }
+          options[kind].push(url);
         }
         if (details.allFrames) {
           options.all_frames = details.allFrames;
         }
         if (details.matchAboutBlank) {
           options.match_about_blank = details.matchAboutBlank;
         }
         if (details.runAt !== null) {
           options.run_at = details.runAt;
         }
 
-        // TODO: Set lastError.
-        context.sendMessage(mm, "Extension:Execute", { options }, recipient)
-          .then(result => {
-            if (callback) {
-              runSafe(context, callback, result);
-            }
-          });
+        return context.sendMessage(mm, "Extension:Execute", { options }, recipient)
+                      .then(value => [value]);
       },
 
       executeScript: function(tabId, details, callback) {
-        self.tabs._execute(tabId, details, "js", callback);
+        return context.wrapPromise(self.tabs._execute(tabId, details, "js"),
+                                   callback);
       },
 
       insertCSS: function(tabId, details, callback) {
-        self.tabs._execute(tabId, details, "css", callback);
+        return context.wrapPromise(self.tabs._execute(tabId, details, "css"),
+                                   callback);
       },
 
       connect: function(tabId, connectInfo) {
         let tab = TabManager.getTab(tabId);
         let mm = tab.linkedBrowser.messageManager;
 
         let name = "";
         if (connectInfo && connectInfo.name !== null) {
--- a/browser/components/extensions/test/browser/browser.ini
+++ b/browser/components/extensions/test/browser/browser.ini
@@ -15,16 +15,17 @@ support-files =
 [browser_ext_browserAction_context.js]
 [browser_ext_browserAction_disabled.js]
 [browser_ext_pageAction_context.js]
 [browser_ext_pageAction_popup.js]
 [browser_ext_browserAction_popup.js]
 [browser_ext_popup_api_injection.js]
 [browser_ext_contextMenus.js]
 [browser_ext_getViews.js]
+[browser_ext_lastError.js]
 [browser_ext_tabs_audio.js]
 [browser_ext_tabs_executeScript.js]
 [browser_ext_tabs_executeScript_good.js]
 [browser_ext_tabs_executeScript_bad.js]
 [browser_ext_tabs_insertCSS.js]
 [browser_ext_tabs_query.js]
 [browser_ext_tabs_getCurrent.js]
 [browser_ext_tabs_create.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/extensions/test/browser/browser_ext_lastError.js
@@ -0,0 +1,55 @@
+"use strict";
+
+function* sendMessage(options) {
+  function background(options) {
+    browser.runtime.sendMessage("invalid-extension-id", {}, {}, result => {
+      browser.test.assertEq(undefined, result, "Argument value");
+      if (options.checkLastError) {
+        let lastError = browser[options.checkLastError].lastError;
+        browser.test.assertEq("Invalid extension ID",
+                              lastError && lastError.message,
+                              "lastError value");
+      }
+      browser.test.sendMessage("done");
+    });
+  }
+
+  let extension = ExtensionTestUtils.loadExtension({
+    background: `(${background})(${JSON.stringify(options)})`,
+  });
+
+  yield extension.startup();
+
+  yield extension.awaitMessage("done");
+
+  yield extension.unload();
+}
+
+add_task(function* testLastError() {
+  // Not necessary in browser-chrome tests, but monitorConsole gripes
+  // if we don't call it.
+  SimpleTest.waitForExplicitFinish();
+
+  // Check that we have no unexpected console messages when lastError is
+  // checked.
+  for (let api of ["extension", "runtime"]) {
+    let waitForConsole = new Promise(resolve => {
+      SimpleTest.monitorConsole(resolve, [], true);
+    });
+
+    yield sendMessage({ checkLastError: api });
+
+    SimpleTest.endMonitorConsole();
+    yield waitForConsole;
+  }
+
+  // Check that we do have a console message when lastError is not checked.
+  let waitForConsole = new Promise(resolve => {
+    SimpleTest.monitorConsole(resolve, [/Unchecked lastError value: Invalid extension ID/], true);
+  });
+
+  yield sendMessage({});
+
+  SimpleTest.endMonitorConsole();
+  yield waitForConsole;
+});
--- a/browser/components/extensions/test/browser/browser_ext_tabs_executeScript_bad.js
+++ b/browser/components/extensions/test/browser/browser_ext_tabs_executeScript_bad.js
@@ -101,14 +101,83 @@ add_task(function* testBadPermissions() 
       });
     },
   });
 
   yield BrowserTestUtils.removeTab(tab2);
   yield BrowserTestUtils.removeTab(tab1);
 });
 
+add_task(function* testBadURL() {
+  function background() {
+    browser.tabs.query({ currentWindow: true }, tabs => {
+      let promises = [
+        new Promise(resolve => {
+          browser.tabs.executeScript({
+            file: "http://example.com/script.js",
+          }, result => {
+            browser.test.assertEq(undefined, result, "Result value");
+
+            browser.test.assertTrue(browser.extension.lastError instanceof Error,
+                                    "runtime.lastError is Error");
+
+            browser.test.assertTrue(browser.runtime.lastError instanceof Error,
+                                    "runtime.lastError is Error");
+
+            browser.test.assertEq(
+              "Files to be injected must be within the extension",
+              browser.extension.lastError && browser.extension.lastError.message,
+              "extension.lastError value");
+
+            browser.test.assertEq(
+              "Files to be injected must be within the extension",
+              browser.runtime.lastError && browser.runtime.lastError.message,
+              "runtime.lastError value");
+
+            resolve();
+          });
+        }),
+
+        browser.tabs.executeScript({
+          file: "http://example.com/script.js",
+        }).catch(error => {
+          browser.test.assertTrue(error instanceof Error, "Error is Error");
+
+          browser.test.assertEq(null, browser.extension.lastError,
+                                "extension.lastError value");
+
+          browser.test.assertEq(null, browser.runtime.lastError,
+                                "runtime.lastError value");
+
+          browser.test.assertEq(
+            "Files to be injected must be within the extension",
+            error && error.message,
+            "error value");
+        }),
+      ];
+
+      Promise.all(promises).then(() => {
+        browser.test.notifyPass("executeScript-lastError");
+      });
+    });
+  }
+
+  let extension = ExtensionTestUtils.loadExtension({
+    manifest: {
+      "permissions": ["<all_urls>"],
+    },
+
+    background,
+  });
+
+  yield extension.startup();
+
+  yield extension.awaitFinish("executeScript-lastError");
+
+  yield extension.unload();
+});
+
 // TODO: Test that |executeScript| fails if the tab has navigated to a
 // new page, and no longer matches our expected state. This involves
 // intentionally trying to trigger a race condition, and is probably not
 // even worth attempting until we have proper |executeScript| callbacks.
 
 add_task(forceGC);
--- a/toolkit/components/extensions/Extension.jsm
+++ b/toolkit/components/extensions/Extension.jsm
@@ -157,23 +157,24 @@ var Management = {
   // Mash together into a single object all the APIs registered by the
   // functions above. Return the merged object.
   generateAPIs(extension, context, apis) {
     let obj = {};
 
     // Recursively copy properties from source to dest.
     function copy(dest, source) {
       for (let prop in source) {
-        if (typeof(source[prop]) == "object") {
+        let desc = Object.getOwnPropertyDescriptor(source, prop);
+        if (typeof(desc.value) == "object") {
           if (!(prop in dest)) {
             dest[prop] = {};
           }
           copy(dest[prop], source[prop]);
         } else {
-          dest[prop] = source[prop];
+          Object.defineProperty(dest, prop, desc);
         }
       }
     }
 
     for (let api of apis) {
       if (api.permission) {
         if (!extension.hasPermission(api.permission)) {
           continue;
@@ -349,16 +350,24 @@ GlobalManager = {
       injectAPI(api, chromeObj);
 
       let schemaApi = Management.generateAPIs(extension, context, Management.schemaApis);
       let schemaWrapper = {
         callFunction(ns, name, args) {
           return schemaApi[ns][name].apply(null, args);
         },
 
+        getProperty(ns, name) {
+          return schemaApi[ns][name];
+        },
+
+        setProperty(ns, name, value) {
+          schemaApi[ns][name] = value;
+        },
+
         addListener(ns, name, listener, args) {
           return schemaApi[ns][name].addListener.call(null, listener, ...args);
         },
         removeListener(ns, name, listener) {
           return schemaApi[ns][name].removeListener.call(null, listener);
         },
         hasListener(ns, name, listener) {
           return schemaApi[ns][name].hasListener.call(null, listener);
--- a/toolkit/components/extensions/ExtensionContent.jsm
+++ b/toolkit/components/extensions/ExtensionContent.jsm
@@ -62,16 +62,20 @@ var api = context => {
           connectInfo = extensionId;
           extensionId = null;
         }
         let name = connectInfo && connectInfo.name || "";
         let recipient = extensionId ? {extensionId} : {extensionId: context.extensionId};
         return context.messenger.connect(context.messageManager, name, recipient);
       },
 
+      get lastError() {
+        return context.lastError;
+      },
+
       getManifest: function() {
         return Cu.cloneInto(context.extension.manifest, context.cloneScope);
       },
 
       getURL: function(url) {
         return context.extension.baseURI.resolve(url);
       },
 
@@ -95,16 +99,20 @@ var api = context => {
       },
     },
 
     extension: {
       getURL: function(url) {
         return context.extension.baseURI.resolve(url);
       },
 
+      get lastError() {
+        return context.lastError;
+      },
+
       inIncognitoContext: PrivateBrowsingUtils.isContentWindowPrivate(context.contentWindow),
     },
 
     i18n: {
       getMessage: function(messageName, substitutions) {
         return context.extension.localizeMessage(messageName, substitutions);
       },
     },
--- a/toolkit/components/extensions/ExtensionUtils.jsm
+++ b/toolkit/components/extensions/ExtensionUtils.jsm
@@ -110,16 +110,18 @@ DefaultWeakMap.prototype = {
       this.defaultValue = value;
     }
   },
 };
 
 class BaseContext {
   constructor() {
     this.onClose = new Set();
+    this.checkedLastError = false;
+    this._lastError = null;
   }
 
   get cloneScope() {
     throw new Error("Not implemented");
   }
 
   get principal() {
     throw new Error("Not implemented");
@@ -147,16 +149,97 @@ class BaseContext {
   callOnClose(obj) {
     this.onClose.add(obj);
   }
 
   forgetOnClose(obj) {
     this.onClose.delete(obj);
   }
 
+  get lastError() {
+    this.checkedLastError = true;
+    return this._lastError;
+  }
+
+  set lastError(val) {
+    this.checkedLastError = false;
+    this._lastError = val;
+  }
+
+  /**
+   * Sets the value of `.lastError` to `error`, calls the given
+   * callback, and reports an error if the value has not been checked
+   * when the callback returns.
+   *
+   * @param {object} error An object with a `message` property. May
+   *     optionally be an `Error` object belonging to the target scope.
+   * @param {function} callback The callback to call.
+   * @returns {*} The return value of callback.
+   */
+  withLastError(error, callback) {
+    if (!(error instanceof this.cloneScope.Error)) {
+      error = new this.cloneScope.Error(error.message);
+    }
+    this.lastError = error;
+    try {
+      return callback();
+    } finally {
+      if (!this.checkedLastError) {
+        Cu.reportError(`Unchecked lastError value: ${error}`);
+      }
+      this.lastError = null;
+    }
+  }
+
+  /**
+   * Wraps the given promise so it can be safely returned to extension
+   * code in this context.
+   *
+   * If `callback` is provided, however, it is used as a completion
+   * function for the promise, and no promise is returned. In this case,
+   * the callback is called when the promise resolves or rejects. In the
+   * latter case, `lastError` is set to the rejection value, and the
+   * callback funciton must check `browser.runtime.lastError` or
+   * `extension.runtime.lastError` in order to prevent it being reported
+   * to the console.
+   *
+   * @param {Promise} promise The promise with which to wrap the
+   *     callback. Must resolve to an array, or other iterable, each
+   *     element of which will be passed as an argument to the callback.
+   *
+   * @param {function} [callback] The callback function to wrap
+   *
+   * @returns {Promise|undefined} If callback is null, a promise object
+   *     belonging to the target scope. Otherwise, undefined.
+   */
+  wrapPromise(promise, callback = null) {
+    if (callback) {
+      promise.then(
+        args => {
+          runSafeSync(this, callback, ...args);
+        },
+        error => {
+          this.withLastError(error, () => {
+            runSafeSync(this, callback);
+          });
+        });
+    } else {
+      return new this.cloneScope.Promise((resolve, reject) => {
+        promise.then(
+          value => { runSafeSync(this, resolve, value); },
+          value => {
+            if (!(value instanceof this.cloneScope.Error)) {
+              value = new this.cloneScope.Error(value.message);
+            }
+            runSafeSyncWithoutClone(reject, value);
+          });
+      });
+    }
+  }
+
   unload() {
     for (let obj of this.onClose) {
       obj.close();
     }
   }
 }
 
 function LocaleData(data) {
@@ -488,24 +571,24 @@ function ignoreEvent(context, name) {
 // Copy an API object from |source| into the scope |dest|.
 function injectAPI(source, dest) {
   for (let prop in source) {
     // Skip names prefixed with '_'.
     if (prop[0] == "_") {
       continue;
     }
 
-    let value = source[prop];
-    if (typeof(value) == "function") {
-      Cu.exportFunction(value, dest, {defineAs: prop});
-    } else if (typeof(value) == "object") {
+    let desc = Object.getOwnPropertyDescriptor(source, prop);
+    if (typeof(desc.value) == "function") {
+      Cu.exportFunction(desc.value, dest, {defineAs: prop});
+    } else if (typeof(desc.value) == "object") {
       let obj = Cu.createObjectIn(dest, {defineAs: prop});
-      injectAPI(value, obj);
+      injectAPI(desc.value, obj);
     } else {
-      dest[prop] = value;
+      Object.defineProperty(dest, prop, desc);
     }
   }
 }
 
 /*
  * Messaging primitives.
  */
 
--- a/toolkit/components/extensions/MessageChannel.jsm
+++ b/toolkit/components/extensions/MessageChannel.jsm
@@ -479,23 +479,31 @@ this.MessageChannel = {
 
         target.sendAsyncMessage(MESSAGE_RESPONSE, response);
       },
       error => {
         let response = {
           result: this.RESULT_ERROR,
           messageName: data.channelId,
           recipient: {},
-          error,
+          error: {},
         };
 
         if (error && typeof(error) == "object") {
           if (error.result) {
             response.result = error.result;
           }
+          // Error objects are not structured-clonable, so just copy
+          // over the important properties.
+          for (let key of ["fileName", "filename", "lineNumber",
+                           "columnNumber", "message", "stack", "result"]) {
+            if (key in error) {
+              response.error[key] = error[key];
+            }
+          }
         }
 
         target.sendAsyncMessage(MESSAGE_RESPONSE, response);
       });
 
     this._addPendingResponse(deferred);
   },
 
@@ -508,17 +516,17 @@ this.MessageChannel = {
   _handleResponse({ handler, error }, data) {
     if (error) {
       // If we have an error at this point, we have handler to report it to,
       // so just log it.
       Cu.reportError(error.message);
     } else if (data.result === this.RESULT_SUCCESS) {
       handler.resolve(data.value);
     } else {
-      handler.reject(data);
+      handler.reject(data.error);
     }
   },
 
   /**
    * Adds a pending response to the the `pendingResponses` list.
    *
    * The response object must be a deferred promise with the following
    * properties:
--- a/toolkit/components/extensions/Schemas.jsm
+++ b/toolkit/components/extensions/Schemas.jsm
@@ -450,20 +450,59 @@ class ValueProperty extends Entry {
   inject(name, dest, wrapperFuncs) {
     dest[name] = this.value;
   }
 }
 
 // Represents a "property" defined in a schema namespace that is not a
 // constant.
 class TypeProperty extends Entry {
-  constructor(name, type) {
+  constructor(namespaceName, name, type, writable) {
     super();
+    this.namespaceName = namespaceName;
     this.name = name;
     this.type = type;
+    this.writable = writable;
+  }
+
+  throwError(global, msg) {
+    global = Cu.getGlobalForObject(global);
+    throw new global.Error(`${msg} for ${this.namespaceName}.${this.name}.`);
+  }
+
+  inject(name, dest, wrapperFuncs) {
+    if (this.unsupported) {
+      return;
+    }
+
+    let getStub = () => {
+      return wrapperFuncs.getProperty(this.namespaceName, name);
+    };
+
+    let desc = {
+      configurable: false,
+      enumerable: true,
+
+      get: Cu.exportFunction(getStub, dest),
+    };
+
+    if (this.writable) {
+      let setStub = (value) => {
+        let normalized = this.type.normalize(value);
+        if (normalized.error) {
+          this.throwError(dest, normalized.error);
+        }
+
+        wrapperFuncs.setProperty(this.namespaceName, name, normalized.value);
+      };
+
+      desc.set = Cu.exportFunction(setStub, dest);
+    }
+
+    Object.defineProperty(dest, name, desc);
   }
 }
 
 // This class is a base class for FunctionEntrys and Events. It takes
 // care of validating parameter lists (i.e., handling of optional
 // parameters and parameter type checking).
 class CallEntry extends Entry {
   constructor(namespaceName, name, parameters, allowAmbiguousOptionalArguments) {
@@ -742,18 +781,19 @@ this.Schemas = {
   },
 
   loadProperty(namespaceName, name, prop) {
     if ("value" in prop) {
       this.register(namespaceName, name, new ValueProperty(name, prop.value));
     } else {
       // We ignore the "optional" attribute on properties since we
       // don't inject anything here anyway.
-      let type = this.parseType(namespaceName, prop, ["optional"]);
-      this.register(namespaceName, name, new TypeProperty(name, type));
+      let type = this.parseType(namespaceName, prop, ["optional", "writable"]);
+      this.register(namespaceName, name, new TypeProperty(namespaceName, name, type),
+                    prop.writable);
     }
   },
 
   loadFunction(namespaceName, fun) {
     // We ignore this property for now.
     let returns = fun.returns;  // eslint-disable-line no-unused-vars
 
     let f = new FunctionEntry(namespaceName, fun.name,
--- a/toolkit/components/extensions/ext-extension.js
+++ b/toolkit/components/extensions/ext-extension.js
@@ -22,15 +22,19 @@ extensions.registerSchemaAPI("extension"
           }
 
           result.push(view.contentWindow);
         }
 
         return result;
       },
 
+      get lastError() {
+        return context.lastError;
+      },
+
       get inIncognitoContext() {
         return context.incognito;
       },
     },
   };
 });
 
--- a/toolkit/components/extensions/ext-runtime.js
+++ b/toolkit/components/extensions/ext-runtime.js
@@ -42,19 +42,28 @@ extensions.registerSchemaAPI("runtime", 
         if (args.length == 1) {
           message = args[0];
         } else if (args.length == 2) {
           [message, responseCallback] = args;
         } else {
           [extensionId, message, options, responseCallback] = args;
         }
         let recipient = {extensionId: extensionId ? extensionId : extension.id};
+
+        if (!GlobalManager.extensionMap.has(recipient.extensionId)) {
+          return context.wrapPromise(Promise.reject({ message: "Invalid extension ID" }),
+                                     responseCallback);
+        }
         return context.messenger.sendMessage(Services.cpmm, message, recipient, responseCallback);
       },
 
+      get lastError() {
+        return context.lastError;
+      },
+
       getManifest() {
         return Cu.cloneInto(extension.manifest, context.cloneScope);
       },
 
       id: extension.id,
 
       getURL: function(url) {
         return extension.baseURI.resolve(url);
--- a/toolkit/components/extensions/schemas/extension.json
+++ b/toolkit/components/extensions/schemas/extension.json
@@ -8,16 +8,19 @@
     "description": "The <code>browser.extension</code> API has utilities that can be used by any extension page. It includes support for exchanging messages between an extension and its content scripts or between extensions, as described in detail in $(topic:messaging)[Message Passing].",
     "properties": {
       "lastError": {
         "type": "object",
         "optional": true,
         "description": "Set for the lifetime of a callback if an ansychronous extension api has resulted in an error. If no error has occured lastError will be <var>undefined</var>.",
         "properties": {
           "message": { "type": "string", "description": "Description of the error that has taken place." }
+        },
+        "additionalProperties": {
+          "type": "any"
         }
       },
       "inIncognitoContext": {
         "type": "boolean",
         "optional": true,
         "description": "True for content scripts running inside incognito tabs, and for extension pages running inside an incognito process. The latter only applies to extensions with 'split' incognito_behavior."
       }
     },
--- a/toolkit/components/extensions/schemas/runtime.json
+++ b/toolkit/components/extensions/schemas/runtime.json
@@ -94,16 +94,19 @@
         "optional": true,
         "description": "This will be defined during an API method callback if there was an error",
         "properties": {
           "message": {
             "optional": true,
             "type": "string",
             "description": "Details about the error which occurred."
           }
+        },
+        "additionalProperties": {
+          "type": "any"
         }
       },
       "id": {
         "type": "string",
         "description": "The ID of the extension/app."
       }
     },
     "functions": [