Bug 1287007 - Require "async" in schemas to match name draft
authorRob Wu <rob@robwu.nl>
Fri, 02 Sep 2016 03:37:55 -0700
changeset 428419 59488359cf7c3b141e4550cf6d11e86199c15dc6
parent 428418 0dc7324462f87f001e99dba0ad1d869ee3692e89
child 428420 82e419f1caa796a9b13a8163d2ee5fc0aa6f3529
push id33305
push userbmo:rob@robwu.nl
push dateSun, 23 Oct 2016 20:56:25 +0000
bugs1287007
milestone52.0a1
Bug 1287007 - Require "async" in schemas to match name In the pageAction and browserAction schemas, several methods are declared with `"async": true` but without a specified callback in the `"parameters"` object, so callbacks are not allowed. However, when a callback is proxied, the `ParentAPIManager` will mirror the call by passing in an extra callback to the proxied API - and break. This patch fixes the issue by removing uses of async:true. Also for consistency between the browserAction and pageAction methods, the methods that were not declared as async have also been marked as async. MozReview-Commit-ID: JQqzmTUAotB
browser/components/extensions/schemas/page_action.json
browser/components/extensions/schemas/tabs.json
mobile/android/components/extensions/schemas/page_action.json
toolkit/components/extensions/Schemas.jsm
toolkit/components/extensions/test/xpcshell/test_ext_schemas_async.js
toolkit/components/extensions/test/xpcshell/xpcshell.ini
--- a/browser/components/extensions/schemas/page_action.json
+++ b/browser/components/extensions/schemas/page_action.json
@@ -51,29 +51,41 @@
         "additionalProperties": { "type": "any" },
         "description": "Pixel data for an image. Must be an ImageData object (for example, from a <code>canvas</code> element)."
       }
     ],
     "functions": [
       {
         "name": "show",
         "type": "function",
-        "async": true,
+        "async": "callback",
         "description": "Shows the page action. The page action is shown whenever the tab is selected.",
         "parameters": [
-          {"type": "integer", "name": "tabId", "minimum": 0, "description": "The id of the tab for which you want to modify the page action."}
+          {"type": "integer", "name": "tabId", "minimum": 0, "description": "The id of the tab for which you want to modify the page action."},
+          {
+            "type": "function",
+            "name": "callback",
+            "optional": true,
+            "parameters": []
+          }
         ]
       },
       {
         "name": "hide",
         "type": "function",
-        "async": true,
+        "async": "callback",
         "description": "Hides the page action.",
         "parameters": [
-          {"type": "integer", "name": "tabId", "minimum": 0, "description": "The id of the tab for which you want to modify the page action."}
+          {"type": "integer", "name": "tabId", "minimum": 0, "description": "The id of the tab for which you want to modify the page action."},
+          {
+            "type": "function",
+            "name": "callback",
+            "optional": true,
+            "parameters": []
+          }
         ]
       },
       {
         "name": "setTitle",
         "type": "function",
         "description": "Sets the title of the page action. This is displayed in a tooltip over the page action.",
         "parameters": [
           {
--- a/browser/components/extensions/schemas/tabs.json
+++ b/browser/components/extensions/schemas/tabs.json
@@ -250,17 +250,17 @@
             ]
           }
         ]
       },
       {
         "name": "sendMessage",
         "type": "function",
         "description": "Sends a single message to the content script(s) in the specified tab, with an optional callback to run when a response is sent back.  The $(ref:runtime.onMessage) event is fired in each content script running in the specified tab for the current extension.",
-        "async": "sendResponse",
+        "async": "responseCallback",
         "parameters": [
           {
             "type": "integer",
             "name": "tabId",
             "minimum": 0
           },
           {
             "type": "any",
--- a/mobile/android/components/extensions/schemas/page_action.json
+++ b/mobile/android/components/extensions/schemas/page_action.json
@@ -52,28 +52,40 @@
         "description": "Pixel data for an image. Must be an ImageData object (for example, from a <code>canvas</code> element)."
       }
     ],
     "functions": [
       {
         "name": "show",
         "type": "function",
         "description": "Shows the page action. The page action is shown whenever the tab is selected.",
-        "async": true,
+        "async": "callback",
         "parameters": [
-          {"type": "integer", "name": "tabId", "minimum": 0, "description": "The id of the tab for which you want to modify the page action."}
+          {"type": "integer", "name": "tabId", "minimum": 0, "description": "The id of the tab for which you want to modify the page action."},
+          {
+            "type": "function",
+            "name": "callback",
+            "optional": true,
+            "parameters": []
+          }
         ]
       },
       {
         "name": "hide",
         "type": "function",
         "description": "Hides the page action.",
-        "async": true,
+        "async": "callback",
         "parameters": [
-          {"type": "integer", "name": "tabId", "minimum": 0, "description": "The id of the tab for which you want to modify the page action."}
+          {"type": "integer", "name": "tabId", "minimum": 0, "description": "The id of the tab for which you want to modify the page action."},
+          {
+            "type": "function",
+            "name": "callback",
+            "optional": true,
+            "parameters": []
+          }
         ]
       },
       {
         "name": "setTitle",
         "unsupported": true,
         "type": "function",
         "description": "Sets the title of the page action. This is displayed in a tooltip over the page action.",
         "parameters": [
@@ -158,47 +170,59 @@
             "optional": true,
             "parameters": []
           }
         ]
       },
       {
         "name": "setPopup",
         "type": "function",
-        "async": true,
+        "async": "callback",
         "description": "Sets the html document to be opened as a popup when the user clicks on the page action's icon.",
         "parameters": [
           {
             "name": "details",
             "type": "object",
             "properties": {
               "tabId": {"type": "integer", "minimum": 0, "description": "The id of the tab for which you want to modify the page action."},
               "popup": {
                 "type": "string",
                 "description": "The html file to show in a popup.  If set to the empty string (''), no popup is shown."
               }
             }
+          },
+          {
+            "type": "function",
+            "name": "callback",
+            "optional": true,
+            "parameters": []
           }
         ]
       },
       {
         "name": "getPopup",
         "type": "function",
         "description": "Gets the html document set as the popup for this page action.",
-        "async": true,
+        "async": "callback",
         "parameters": [
           {
             "name": "details",
             "type": "object",
             "properties": {
               "tabId": {
                 "type": "integer",
                 "description": "Specify the tab to get the popup from."
               }
             }
+          },
+          {
+            "type": "function",
+            "name": "callback",
+            "optional": true,
+            "parameters": []
           }
         ]
       }
     ],
     "events": [
       {
         "name": "onClicked",
         "type": "function",
--- a/toolkit/components/extensions/Schemas.jsm
+++ b/toolkit/components/extensions/Schemas.jsm
@@ -1279,32 +1279,39 @@ class FunctionType extends Type {
   static get EXTRA_PROPERTIES() {
     return ["parameters", "async", "returns", ...super.EXTRA_PROPERTIES];
   }
 
   static parseSchema(schema, path, extraProperties = []) {
     this.checkSchemaProperties(schema, path, extraProperties);
 
     let isAsync = !!schema.async;
+    let isExpectingCallback = isAsync;
     let parameters = null;
     if ("parameters" in schema) {
       parameters = [];
       for (let param of schema.parameters) {
         // Callbacks default to optional for now, because of promise
         // handling.
         let isCallback = isAsync && param.name == schema.async;
+        if (isCallback) {
+          isExpectingCallback = false;
+        }
 
         parameters.push({
           type: Schemas.parseSchema(param, path, ["name", "optional", "default"]),
           name: param.name,
           optional: param.optional == null ? isCallback : param.optional,
           default: param.default == undefined ? null : param.default,
         });
       }
     }
+    if (isExpectingCallback) {
+      throw new Error(`Internal error: Expected a callback parameter with name ${schema.async}`);
+    }
 
     let hasAsyncCallback = false;
     if (isAsync) {
       if (parameters && parameters.length && parameters[parameters.length - 1].name == schema.async) {
         hasAsyncCallback = true;
       }
       if (schema.returns) {
         throw new Error("Internal error: Async functions must not have return values.");
new file mode 100644
--- /dev/null
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_schemas_async.js
@@ -0,0 +1,232 @@
+"use strict";
+
+Components.utils.import("resource://gre/modules/ExtensionUtils.jsm");
+Components.utils.import("resource://gre/modules/Schemas.jsm");
+
+let {BaseContext, LocalAPIImplementation} = ExtensionUtils;
+
+let schemaJson = [
+  {
+    namespace: "testnamespace",
+    functions: [{
+      name: "one_required",
+      type: "function",
+      parameters: [{
+        name: "first",
+        type: "function",
+        parameters: [],
+      }],
+    }, {
+      name: "one_optional",
+      type: "function",
+      parameters: [{
+        name: "first",
+        type: "function",
+        parameters: [],
+        optional: true,
+      }],
+    }, {
+      name: "async_required",
+      type: "function",
+      async: "first",
+      parameters: [{
+        name: "first",
+        type: "function",
+        parameters: [],
+      }],
+    }, {
+      name: "async_optional",
+      type: "function",
+      async: "first",
+      parameters: [{
+        name: "first",
+        type: "function",
+        parameters: [],
+        optional: true,
+      }],
+    }],
+  },
+];
+
+const global = this;
+class StubContext extends BaseContext {
+  constructor() {
+    let fakeExtension = {id: "test@web.extension"};
+    super("testEnv", fakeExtension);
+    this.sandbox = Cu.Sandbox(global);
+  }
+
+  get cloneScope() {
+    return this.sandbox;
+  }
+
+  get principal() {
+    return Cu.getObjectPrincipal(this.sandbox);
+  }
+}
+
+let context;
+
+function generateAPIs(extraWrapper, apiObj) {
+  context = new StubContext();
+  let localWrapper = {
+    shouldInject() {
+      return true;
+    },
+    getImplementation(namespace, name) {
+      return new LocalAPIImplementation(apiObj, name, context);
+    },
+  };
+  Object.assign(localWrapper, extraWrapper);
+
+  let root = {};
+  Schemas.inject(root, localWrapper);
+  return root.testnamespace;
+}
+
+add_task(function* testParameterValidation() {
+  yield Schemas.load("data:," + JSON.stringify(schemaJson));
+
+  let testnamespace;
+  function assertThrows(name, ...args) {
+    Assert.throws(() => testnamespace[name](...args),
+        /Incorrect argument types/,
+        `Expected testnamespace.${name}(${args.map(String).join(", ")}) to throw.`);
+  }
+  function assertNoThrows(name, ...args) {
+    try {
+      testnamespace[name](...args);
+    } catch (e) {
+      do_print(`testnamespace.${name}(${args.map(String).join(", ")}) unexpectedly threw.`);
+      throw new Error(e);
+    }
+  }
+  let cb = () => {};
+
+  for (let isChromeCompat of [true, false]) {
+    do_print(`Testing API validation with isChromeCompat=${isChromeCompat}`);
+    testnamespace = generateAPIs({
+      isChromeCompat,
+    }, {
+      one_required() {},
+      one_optional() {},
+      async_required() {},
+      async_optional() {},
+    });
+
+    assertThrows("one_required");
+    assertThrows("one_required", null);
+    assertNoThrows("one_required", cb);
+    assertThrows("one_required", cb, null);
+    assertThrows("one_required", cb, cb);
+
+    assertNoThrows("one_optional");
+    assertNoThrows("one_optional", null);
+    assertNoThrows("one_optional", cb);
+    assertThrows("one_optional", cb, null);
+    assertThrows("one_optional", cb, cb);
+
+    // Schema-based validation happens before an async method is called, so
+    // errors should be thrown synchronously.
+
+    // The parameter was declared as required, but there was also an "async"
+    // attribute with the same value as the parameter name, so the callback
+    // parameter is actually optional.
+    assertNoThrows("async_required");
+    assertNoThrows("async_required", null);
+    assertNoThrows("async_required", cb);
+    assertThrows("async_required", cb, null);
+    assertThrows("async_required", cb, cb);
+
+    assertNoThrows("async_optional");
+    assertNoThrows("async_optional", null);
+    assertNoThrows("async_optional", cb);
+    assertThrows("async_optional", cb, null);
+    assertThrows("async_optional", cb, cb);
+  }
+});
+
+add_task(function* testAsyncResults() {
+  yield Schemas.load("data:," + JSON.stringify(schemaJson));
+  function* runWithCallback(func) {
+    do_print(`Calling testnamespace.${func.name}, expecting callback with result`);
+    return yield new Promise(resolve => {
+      let result = "uninitialized value";
+      let returnValue = func(reply => {
+        result = reply;
+        resolve(result);
+      });
+      // When a callback is given, the return value must be missing.
+      do_check_eq(returnValue, undefined);
+      // Callback must be called asynchronously.
+      do_check_eq(result, "uninitialized value");
+    });
+  }
+
+  function* runFailCallback(func) {
+    do_print(`Calling testnamespace.${func.name}, expecting callback with error`);
+    return yield new Promise(resolve => {
+      func(reply => {
+        do_check_eq(reply, undefined);
+        resolve(context.lastError.message); // eslint-disable-line no-undef
+      });
+    });
+  }
+
+  for (let isChromeCompat of [true, false]) {
+    do_print(`Testing API invocation with isChromeCompat=${isChromeCompat}`);
+    let testnamespace = generateAPIs({
+      isChromeCompat,
+    }, {
+      async_required(cb) {
+        do_check_eq(cb, undefined);
+        return Promise.resolve(1);
+      },
+      async_optional(cb) {
+        do_check_eq(cb, undefined);
+        return Promise.resolve(2);
+      },
+    });
+    if (!isChromeCompat) {  // No promises for chrome.
+      do_print("testnamespace.async_required should be a Promise");
+      let promise = testnamespace.async_required();
+      do_check_true(promise instanceof context.cloneScope.Promise);
+      do_check_eq(yield promise, 1);
+
+      do_print("testnamespace.async_optional should be a Promise");
+      promise = testnamespace.async_optional();
+      do_check_true(promise instanceof context.cloneScope.Promise);
+      do_check_eq(yield promise, 2);
+    }
+
+    do_check_eq(yield* runWithCallback(testnamespace.async_required), 1);
+    do_check_eq(yield* runWithCallback(testnamespace.async_optional), 2);
+
+    let otherSandbox = Cu.Sandbox(null, {});
+    let errorFactories = [
+      msg => { throw new context.cloneScope.Error(msg); },
+      msg => context.cloneScope.Promise.reject({message: msg}),
+      msg => Cu.evalInSandbox(`throw new Error("${msg}")`, otherSandbox),
+      msg => Cu.evalInSandbox(`Promise.reject({message: "${msg}"})`, otherSandbox),
+    ];
+    for (let makeError of errorFactories) {
+      do_print(`Testing callback/promise with error caused by: ${makeError}`);
+      testnamespace = generateAPIs({
+        isChromeCompat,
+      }, {
+        async_required() { return makeError("ONE"); },
+        async_optional() { return makeError("TWO"); },
+      });
+
+      if (!isChromeCompat) {  // No promises for chrome.
+        yield Assert.rejects(testnamespace.async_required(), /ONE/,
+            "should reject testnamespace.async_required()").catch(() => {});
+        yield Assert.rejects(testnamespace.async_optional(), /TWO/,
+            "should reject testnamespace.async_optional()").catch(() => {});
+      }
+
+      do_check_eq(yield* runFailCallback(testnamespace.async_required), "ONE");
+      do_check_eq(yield* runFailCallback(testnamespace.async_optional), "TWO");
+    }
+  }
+});
--- a/toolkit/components/extensions/test/xpcshell/xpcshell.ini
+++ b/toolkit/components/extensions/test/xpcshell/xpcshell.ini
@@ -47,16 +47,17 @@ skip-if = release_or_beta
 [test_ext_runtime_getBrowserInfo.js]
 [test_ext_runtime_getPlatformInfo.js]
 [test_ext_runtime_sendMessage.js]
 [test_ext_runtime_sendMessage_errors.js]
 [test_ext_runtime_sendMessage_no_receiver.js]
 [test_ext_runtime_sendMessage_self.js]
 [test_ext_schemas.js]
 [test_ext_schemas_api_injection.js]
+[test_ext_schemas_async.js]
 [test_ext_schemas_allowed_contexts.js]
 [test_ext_simple.js]
 [test_ext_storage.js]
 [test_ext_topSites.js]
 skip-if = os == "android"
 [test_getAPILevelForWindow.js]
 [test_ext_legacy_extension_context.js]
 [test_ext_legacy_extension_embedding.js]