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
--- 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]