Bug 1287007 - Require "async" in schemas to match name r=billm

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

--HG--
extra : rebase_source : 62d1cbc4843dd6c792318337158e4311f8df94a4

browser/components/extensions/schemas/page_action.json

@@ -56,19 +56,31 @@
       {
         "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": []
+          }
         ]
       },
       {

browser/components/extensions/schemas/tabs.json

@@ -255,7 +255,7 @@
         "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",

mobile/android/components/extensions/schemas/page_action.json

@@ -57,18 +57,30 @@
         "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": []
+          }
         ]
       },
       {
@@ -163,7 +175,7 @@
       {
         "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": [
           {
@@ -176,6 +188,12 @@
                 "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": []
           }
         ]
       },
@@ -183,7 +201,7 @@
         "name": "getPopup",
         "type": "function",
         "description": "Gets the html document set as the popup for this page action.",
-        "async": true,
+        "async": "callback",
         "parameters": [
           {
             "name": "details",
@@ -194,6 +212,12 @@
                 "description": "Specify the tab to get the popup from."
               }
             }
+          },
+          {
+            "type": "function",
+            "name": "callback",
+            "optional": true,
+            "parameters": []
           }
         ]
       }

toolkit/components/extensions/Schemas.jsm

@@ -1284,6 +1284,7 @@ class FunctionType extends Type {
     this.checkSchemaProperties(schema, path, extraProperties);
 
     let isAsync = !!schema.async;
+    let isExpectingCallback = isAsync;
     let parameters = null;
     if ("parameters" in schema) {
       parameters = [];
@@ -1291,6 +1292,9 @@ class FunctionType extends Type {
         // 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"]),
@@ -1300,6 +1304,9 @@ class FunctionType extends Type {
         });
       }
     }
+    if (isExpectingCallback) {
+      throw new Error(`Internal error: Expected a callback parameter with name ${schema.async}`);
+    }
 
     let hasAsyncCallback = false;
     if (isAsync) {

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");
+    }
+  }
+});

toolkit/components/extensions/test/xpcshell/xpcshell.ini

@@ -52,6 +52,7 @@ skip-if = release_or_beta
 [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]