From 8a87af36f9e9452f88706e44f4c5d908472126b9 Mon Sep 17 00:00:00 2001 From: Rob Wu Date: Fri, 2 Sep 2016 03:37:55 -0700 Subject: [PATCH] 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 --- .../extensions/schemas/page_action.json | 20 +- .../components/extensions/schemas/tabs.json | 2 +- .../extensions/schemas/page_action.json | 36 ++- toolkit/components/extensions/Schemas.jsm | 7 + .../test/xpcshell/test_ext_schemas_async.js | 232 ++++++++++++++++++ .../extensions/test/xpcshell/xpcshell.ini | 1 + 6 files changed, 287 insertions(+), 11 deletions(-) create mode 100644 toolkit/components/extensions/test/xpcshell/test_ext_schemas_async.js diff --git a/browser/components/extensions/schemas/page_action.json b/browser/components/extensions/schemas/page_action.json index 38bf784691a8..c17f29687f4b 100644 --- a/browser/components/extensions/schemas/page_action.json +++ b/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": [] + } ] }, { diff --git a/browser/components/extensions/schemas/tabs.json b/browser/components/extensions/schemas/tabs.json index 0f94671bbe48..c1edc6c6402e 100644 --- a/browser/components/extensions/schemas/tabs.json +++ b/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", diff --git a/mobile/android/components/extensions/schemas/page_action.json b/mobile/android/components/extensions/schemas/page_action.json index 2e2c38a2015a..5e92809229d2 100644 --- a/mobile/android/components/extensions/schemas/page_action.json +++ b/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": [] } ] } diff --git a/toolkit/components/extensions/Schemas.jsm b/toolkit/components/extensions/Schemas.jsm index 44af5f64e192..71aa3bb39f35 100644 --- a/toolkit/components/extensions/Schemas.jsm +++ b/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) { diff --git a/toolkit/components/extensions/test/xpcshell/test_ext_schemas_async.js b/toolkit/components/extensions/test/xpcshell/test_ext_schemas_async.js new file mode 100644 index 000000000000..e757792a036b --- /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"); + } + } +}); diff --git a/toolkit/components/extensions/test/xpcshell/xpcshell.ini b/toolkit/components/extensions/test/xpcshell/xpcshell.ini index 3ab1871c22aa..760cca09ea03 100644 --- a/toolkit/components/extensions/test/xpcshell/xpcshell.ini +++ b/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]