Bug 1190680: Part 2 - Add initial support for lastError callbacks. r?billm draft
authorKris Maglione <maglione.k@gmail.com>
Fri, 22 Jan 2016 21:14:11 -0800
changeset 324494 76aa11d2c8e89cc99674357428214fd749601d4f
parent 324493 4b051e065f262155a8dec79c819b40c872a78b19
child 513401 16ccc852b34d28413e1e59fddef68cb254ae61bf
push id9927
push usermaglione.k@gmail.com
push dateSat, 23 Jan 2016 05:20:35 +0000
reviewersbillm
bugs1190680
milestone46.0a1
Bug 1190680: Part 2 - 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/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
@@ -436,17 +436,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: [],
 
           // We need to send the inner window ID to make sure we only
@@ -467,43 +467,44 @@ 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;
         }
         mm.sendAsyncMessage("Extension:Execute",
                             {extensionId: extension.id, options});
 
-        // TODO: Call the callback with the result (which is what???).
+        return Promise.resolve([]);
       },
 
       executeScript: function(tabId, details, callback) {
-        self.tabs._execute(tabId, details, "js", callback);
+        return context.wrapCallback(callback,
+                                    self.tabs._execute(tabId, details, "js"));
       },
 
       insertCss: function(tabId, details, callback) {
-        self.tabs._execute(tabId, details, "css", callback);
+        return context.wrapCallback(callback,
+                                    self.tabs._execute(tabId, details, "css"));
       },
 
       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_executeScript_good.js]
 [browser_ext_tabs_executeScript_bad.js]
 [browser_ext_tabs_query.js]
 [browser_ext_tabs_getCurrent.js]
 [browser_ext_tabs_create.js]
 [browser_ext_tabs_update.js]
 [browser_ext_tabs_onUpdated.js]
 [browser_ext_tabs_sendMessage.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
@@ -155,23 +155,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;
@@ -318,16 +319,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
@@ -58,16 +58,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);
       },
 
@@ -91,16 +95,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 = undefined;
   }
 
   get cloneScope() {
     throw new Error("Not implemented");
   }
 
   get principal() {
     throw new Error("Not implemented");
@@ -147,16 +149,90 @@ 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 callback with the given promise. When the promise
+   * resolves, the callback is called with its resolution value. When
+   * the promise rejects, the callback is called with no arguments, and
+   * `lastError` set to its rejection value.
+   *
+   * @param {function|null} callback The callback function to wrap
+   * @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.
+   *
+   * @returns {Promise|undefined} If callback is null, a promise object
+   *     belonging to the target scope. Otherwise, undefined.
+   */
+  wrapCallback(callback, promise) {
+    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 +564,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/Schemas.jsm
+++ b/toolkit/components/extensions/Schemas.jsm
@@ -69,16 +69,21 @@ function getValueBaseType(value) {
 class Entry {
   // Injects JS values for the entry into the extension API
   // namespace. The default implementation is to do
   // nothing. |wrapperFuncs| is used to call the actual implementation
   // of a given function or event. It's an object with properties
   // callFunction, addListener, removeListener, and hasListener.
   inject(name, dest, wrapperFuncs) {
   }
+
+  throwError(global, msg) {
+    global = Cu.getGlobalForObject(global);
+    throw new global.Error(`${msg} for ${this.namespaceName}.${this.name}.`);
+  }
 }
 
 // Corresponds either to a type declared in the "types" section of the
 // schema or else to any type object used throughout the schema.
 class Type extends Entry {
   // Takes a value, checks that it has the correct type, and returns a
   // "normalized" version of the value. The normalized version will
   // include "nulls" in place of omitted optional properties. The
@@ -450,40 +455,69 @@ 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;
+  }
+
+  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) {
     super();
     this.namespaceName = namespaceName;
     this.name = name;
     this.parameters = parameters;
     this.allowAmbiguousOptionalArguments = allowAmbiguousOptionalArguments;
   }
 
-  throwError(global, msg) {
-    global = Cu.getGlobalForObject(global);
-    throw new global.Error(`${msg} for ${this.namespaceName}.${this.name}.`);
-  }
-
   checkParameters(args, global) {
     let fixedArgs = [];
 
     // First we create a new array, fixedArgs, that is the same as
     // |args| but with null values in place of omitted optional
     // parameters.
     let check = (parameterIndex, argIndex) => {
       if (parameterIndex == this.parameters.length) {
@@ -742,18 +776,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.wrapCallback(responseCallback,
+                                      Promise.reject({ message: "Invalid extension ID" }));
+        }
         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": [