I've attached a WebExtension that adds a button: when you click the button, the background script executes a script in the active tab, and passes a callback to run when the script has finished: chrome.browserAction.onClicked.addListener(executeScript); function executeScript() { chrome.tabs.executeScript({ file: "content_scripts/hello.js" }, runCallback); } function runCallback() { alert("I'm in the callback"); } This fails, and "tab is null" gets logged to the console. The problem seems to be here: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-tabs.js?offset=500#466 - if you omit the tabId (to ask for the active tab) but include a callback, then the code passes arg[0] to _execute. _execute expects this to be the tabID, but since you've omitted the tabId, arg[0] is the details object, so tab lookup fails. If you pass undefined as tabId, it works. I'm not sure if the caller is expected to do this, but omitting tabId works in Chrome, so I guess it ought to work here, too.

Chrome allows optional arguments to be omitted, even if other arguments (optional or mandatory) follow them. I haven't been able to find documentation for this anywhere, but I did find the patch that implemented the behavior, and it does the following to resolve the meaning of arguments: 1) Generate a list of all possible call signatures, for any combination of included or omitted optional arguments. 2) Check that no two signatures can match the same set of arguments. 3) At call time, loop over the list of signatures, and accept the first one which matches the given arguments. 4) Normalize the arguments list, so that omitted arguments are replaced with nulls. So, ideally, this should wait for bug 1190677, and we should normalize arguments in roughly the same manner that Chrome does. If this is breaking any existing extensions, though, we may want to special case this function in the mean time.

Forking this into two bugs, since, as discussed on IRC, the callback isn't handled in either case, but omitting only the first argument results in an error. In ext-tabs.js, this also affects: executeScript, insertCSS, update

Reversing the dependency order. I'm going to write a prototype argument checker for handling optional arguments, and we can use that as a basis for full scale argument checking later.

Hey Kris, do you mind if I steal this from you? I need something like this to implement the OOP stuff.

If you need it before next week, sure. I was planning to do it over the weekend, since I need the type checking for the manifest processing bugs. I was hoping to have it done earlier in the week, but things caught fire.

Sorry, I already started on it :-|. I think I'll have it done over the weekend.

It's fine. There are other things I can work on instead.

The code Kris mentioned in comment 1 is in src/extensions/renderer/resources in the Chromium tree. Mostly in binding.js and json_schema.js.

Hey Kris, here's what I have so far. Hopefully it unblocks you. I'll try to finish it up tomorrow.

Yup, I think I can work with that. Thanks

This is essentially the same code as before. I added some more tests, an "unsupported" property to functions, events, and object properties (so we don't have to delete things from the .json file even though we plan to support them eventually), and some rudimentary validation of the schemas themselves. Ideally we would have a schema for schemas, but I just did some simple property name checking, which covers most of the problems I'm concerned about.

This extends Extension.jsm to allow APIs to be registered using schemas. I also switched the cookies API to use the schema validation rather than the ad hoc stuff we're doing now.

This patch switches webrequest to use schema validation. In doing so, I found that our test actually passed the filter parameter incorrectly :-).

Same thing for webNavigation. Bug 1190329 has a test to make sure this isn't totally broken.

I'll keep working on more APIs. I don't think we need to land these all at once though.

Comment on attachment 8689831 [details] [diff] [review] Extension.jsm support for schemas and cookie implementation I just noticed a problem here. I'll try to fix it tomorrow.

Comment on attachment 8689830 [details] [diff] [review] schema parsing/checking Review of attachment 8689830 [details] [diff] [review]: ----------------------------------------------------------------- This is very nice. ::: toolkit/components/extensions/Schemas.jsm @@ +58,5 @@ > + return t; > +} > + > +class Entry { > + inject(name, dest, wrapperFuncs) { Please add comments describing the base classes and their methods. @@ +110,5 @@ > + return this.choices.some(t => t.checkBaseType(baseType)); > + } > +}; > + > +class RefType extends Type { Please add a comment describing this type, what the namespaces are, and how references are used. @@ +144,5 @@ > + } > + > + normalize(value) { > + if (this.enumeration) { > + if (this.enumeration.indexOf(value) != -1) { |if (this.enumeration.includes(value))| @@ +150,5 @@ > + } > + return {error: `Invalid enumeration value ${JSON.stringify(value)}`}; > + } > + > + // We don't bother checking min/max length. Why not? @@ +192,5 @@ > + return baseType == "object"; > + } > + > + normalize(value) { > + if (getValueBaseType(value) != "object") { Why not |this.normalizeBase|? Same for ArrayType. @@ +220,5 @@ > + result[prop] = null; > + } > + } > + > + for (let prop in value) { We should probably use |Object.keys| here, if not above too. |__iterator__| properties and strange prototypes will come into play here. @@ +247,5 @@ > + } > + > + if (isNaN(value) || > + value == Number.POSITIVE_INFINITY || > + value == Number.NEGATIVE_INFINITY) { |if (!Number.isFinite(value))| @@ +251,5 @@ > + value == Number.NEGATIVE_INFINITY) { > + return {error: "NaN or infinity are not valid"}; > + } > + > + // Don't bother checking min/max values. Why not? Especially in the case of negative values, I could see this being important. @@ +269,5 @@ > + return r; > + } > + > + // Ensure it's between -2**31 and 2**31-1 > + if ((value | 0) !== value) { We should probably use the same check for the "integer" base type. @@ +374,5 @@ > + const ABSENT = 0; > + const UNDEF = 1; > + const PRESENT = 2; > + > + function check(accum, parameters, args) { It probably doesn't make a huge difference, but if you passed an index rather than an array for |args| and |parameters|, you'd avoid generating a lot of array garbage without really making anything less readable (I think I'd actually find it slightly easier to follow). You could also get away with using a single array for |accum|, and writing to the same index as the parameter index that you're reading. @@ +415,5 @@ > + } > + > + let fixedArgs = []; > + let j = 0; > + for (let i = 0; i < this.parameters.length; i++) { This might be more understandable as |let parameters = check(...); let fixedArgs = parameters.map(present => { ... })| Or even |let fixedArgs = this.parameters.map((type, i) => { if (present[i]) ... })|. It takes a lot of reading to figure out that each iteration of the loop adds one arg to the result. Also, maybe something like |currentArg| for |j|, or just use |args.shift()| instead. @@ +434,5 @@ > + return fixedArgs; > + } > +}; > + > +class Function extends CallEntry { Please give this a different name, like |FunctionEntry|. I get an uncomfortable gut reaction whenever I see |new Function()|. @@ +497,5 @@ > + } > +}; > + > +let Schemas = { > + namespaces: new Map(), A comment explaining this would be helpful. @@ +509,5 @@ > + ns.set(symbol, value); > + }, > + > + parseType(namespaceName, type, extraProperties = []) { > + let allowedProperties = extraProperties; Should make a copy, since you modify below. @@ +513,5 @@ > + let allowedProperties = extraProperties; > + > + // Do some simple validation of our own schemas. > + function checkTypeProperties(...extra) { > + let allowedSet = new Set(allowedProperties); |new Set([...allowedProperties, ...extra, "description"])| ::: toolkit/components/extensions/test/xpcshell/test_ext_schemas.js @@ +206,5 @@ > + > + try { > + root.testing.bar(11); > + } catch (e) { > + threw = true; You can use |Assert.throws| for these. @@ +253,5 @@ > + } catch (e) { > + threw = true; > + } > + do_check_eq(threw, true, "should throw with wrong type"); > + threw = false; Can you test that extra arguments also throw?

This actually waits for schemas to load before trying to use them.

This one needed some Schema.jsm changes. I put them in here so you don't have to review the Schema.jsm patch again.

Comment on attachment 8690350 [details] [diff] [review] Extension.jsm support for schemas and cookie implementation, v2 Review of attachment 8690350 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Bill McCloskey (:billm) from comment #19) > Created attachment 8690350 [details] [diff] [review] > Extension.jsm support for schemas and cookie implementation, v2 > > This actually waits for schemas to load before trying to use them. Good, I'd already had a comment about that :) Sorry I didn't finish reviewing this yesterday. I needed a break after the first patch. ::: toolkit/components/extensions/ext-cookies.js @@ +30,5 @@ > > function* query(allDetails, allowed) { > let details = {}; > allowed.map(property => { > + if (allDetails[property] !== null) { Is |allowed| necessary anymore? It looks like it's already enforced by the schema everywhere it matters.

Comment on attachment 8689832 [details] [diff] [review] webrequest schema support Review of attachment 8689832 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest.html @@ +175,5 @@ > + browser.webRequest.onBeforeRequest.addListener(onBeforeRequest, {urls: ["<all_urls>"]}, ["blocking"]); > + browser.webRequest.onBeforeSendHeaders.addListener(onBeforeSendHeaders, {urls: ["<all_urls>"]}, ["blocking"]); > + browser.webRequest.onSendHeaders.addListener(onRecord.bind(null, "sendHeaders"), {urls: ["<all_urls>"]}); > + browser.webRequest.onResponseStarted.addListener(onRecord.bind(null, "responseStarted"), {urls: ["<all_urls>"]}); > + browser.webRequest.onCompleted.addListener(onRecord.bind(null, "completed"), {urls: ["<all_urls>"]}); I wonder if this is going to break any extensions... but I guess it's too early to start worrying about that.

Comment on attachment 8689836 [details] [diff] [review] webnavigation schema support Review of attachment 8689836 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/extensions/schemas/web_navigation.json @@ +164,5 @@ > + "processId": {"type": "integer", "description": "The ID of the process runs the renderer for this tab."}, > + "frameId": {"type": "integer", "description": "0 indicates the navigation happens in the tab content window; a positive value indicates navigation in a subframe. Frame IDs are unique within a tab."}, > + "transitionType": {"$ref": "TransitionType", "description": "Cause of the navigation."}, > + "transitionQualifiers": {"type": "array", "description": "A list of transition qualifiers.", "items": {"$ref": "TransitionQualifier"}}, > + "timeStamp": {"type": "number", "description": "The time when the navigation was committed, in milliseconds since the epoch."} I guess most of these aren't actually implemented. Can you add an "unsupported" flag to the ones that aren't? (Same for the other events)

Comment on attachment 8690351 [details] [diff] [review] windows.json Review of attachment 8690351 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/extensions/schemas/windows.json @@ +41,5 @@ > + "optional": true, > + "description": "The state of this browser window." > + }, > + "alwaysOnTop": {"type": "boolean", "description": "Whether the window is set to be always on top."}, > + "sessionId": {"type": "string", "optional": true, "description": "The session ID used to uniquely identify a Window obtained from the $(ref:sessions) API."} Can you add "unsupported" to these last two?

Comment on attachment 8690352 [details] [diff] [review] tabs.json Review of attachment 8690352 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/extensions/ext-tabs.js @@ +382,5 @@ > queryInfo = {}; > } > > let pattern = null; > + if (queryInfo.url !== null) { This isn't going to work if you replace a null |queryInfo| with an empty object. That argument isn't optional, though, so you can just remove that check. ::: browser/components/extensions/schemas/tabs.json @@ +58,5 @@ > + "incognito": {"type": "boolean", "description": "Whether the tab is in an incognito window."}, > + "width": {"type": "integer", "optional": true, "description": "The width of the tab in pixels."}, > + "height": {"type": "integer", "optional": true, "description": "The height of the tab in pixels."}, > + "sessionId": {"type": "string", "optional": true, "description": "The session ID used to uniquely identify a Tab obtained from the $(ref:sessions) API."} > + } Can you add "unsupported" to "openerTabId", "audible", "mutedInfo", and "sessionId"? @@ +366,5 @@ > + "openerTabId": { > + "type": "integer", > + "minimum": 0, > + "optional": true, > + "description": "The ID of the tab that opened this tab. If specified, the opener tab must be in the same window as the newly created tab." Can you add unsupported to this? @@ +488,5 @@ > + "type": "integer", > + "optional": true, > + "minimum": 0, > + "description": "The position of the tabs within their windows." > + } Can you add unsupported to "audible", "muted", and "windowType"? ::: toolkit/components/extensions/Schemas.jsm @@ +114,5 @@ > class RefType extends Type { > constructor(namespaceName, reference) { > super(); > + if (reference.includes('.')) { > + [namespaceName, reference] = reference.split('.', 2); The limit parameter to |String.split| is basically useless. It splits at every '.', but truncates the result array. If that matters, use a regexp or a slice with |indexOf|, otherwise I'd leave the limit out. It's basically a lie. @@ +523,5 @@ > } > allowedSet.add("description"); > for (let prop in type) { > if (!allowedSet.has(prop)) { > + throw new Error(`Internal error: Namespace ${namespaceName} has invalid type property "${prop}" in type "${type.name}"`); You should probably use |InternalError| for these.

I made all the changes you requested except where noted below. > > + // Ensure it's between -2**31 and 2**31-1 > > + if ((value | 0) !== value) { > > We should probably use the same check for the "integer" base type. That code emulates what Chrome does, so I'd rather not change it. > > function* query(allDetails, allowed) { > > let details = {}; > > allowed.map(property => { > > + if (allDetails[property] !== null) { > > Is |allowed| necessary anymore? It looks like it's already enforced by the schema > everywhere it matters. The different callers of query() allow different properties in the details list. So in some cases a missing property on details will be null and in other cases it will be undefined. I commented about this. > Can you add unsupported to "audible", "muted", and "windowType"? I think "windowType" is supported. > You should probably use |InternalError| for these. It looks like InternalError is for stuff internal to the JS engine (OOMs and such). I'd rather not mix schema stuff up with that.

We don't have tests for this API, but our "implementation" is basically empty, so it's probably fine.

It turns out that Chrome has no such thing as an "unrestricted" object type. If you want that, you need to specify "additionalProperties": {"type": "any"}. The isInstanceOf thing here is a bit gross, but I think that's just how it has to be.

I can pass this on to Gabor if you want. It's a bit complicated because of the interaction between Xrays and the onclick handler that's passed in.

Don't feel rushed on this stuff. I understand if you can't get to it until next week. Just posting it so my plate is clear.

Comment on attachment 8692257 [details] [diff] [review] browser_action.json Review of attachment 8692257 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/extensions/ext-utils.js @@ +34,5 @@ > + // an ImageData property. But Schema.jsm doesn't normalize > + // actual ImageData objects, so they will come from a global > + // with the right property. > + let global = Cu.getGlobalForObject(imageData); > + if (global.ImageData && imageData instanceof global.ImageData) { We should probably just use |instanceOf(imageData, "ImageData")| now, since we're doing that in Schemas.jsm anyway. ::: browser/components/extensions/schemas/browser_action.json @@ +92,5 @@ > + { > + "type": "object", > + "properties": { > + "19": {"$ref": "ImageDataType", "optional": true}, > + "38": {"$ref": "ImageDataType", "optional": true} We accept any integer key for this since bug 1200674. Same for path.

Comment on attachment 8692259 [details] [diff] [review] context_menu.json Review of attachment 8692259 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/extensions/Schemas.jsm @@ +258,5 @@ > + // object using a waiver wrapper. > + > + let klass = Cu.getClassName(value, true); > + if (klass != "Object") { > + return {error: `Expected a plain JavaScript object, got a ${klass}`}; Please add a test that we get this error for Proxy objects and objects with Symbol.toStringTag properties (not yet implemented), in case the behavior of getClassName changes. @@ +261,5 @@ > + if (klass != "Object") { > + return {error: `Expected a plain JavaScript object, got a ${klass}`}; > + } > + > + let properties = {}; Should probably use |Object.create(null)| here. @@ +272,5 @@ > + if (desc.get || desc.set) { > + return {error: "Objects cannot have getters or setters on properties"}; > + } > + if (!desc.enumerable) { > + return {error: "Objects cannot have non-enumerable own properties"}; If we're going to make non-enumerable properties an error, we should probably do the same for symbol properties and unexpected prototypes. @@ +274,5 @@ > + } > + if (!desc.enumerable) { > + return {error: "Objects cannot have non-enumerable own properties"}; > + } > + properties[prop] = desc.value; You should |unwaiveXrays| this.

Comment on attachment 8692256 [details] [diff] [review] idle.json Review of attachment 8692256 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/extensions/schemas/idle.json @@ +15,5 @@ > + ], > + "functions": [ > + { > + "name": "queryState", > + "unsupported": true, Right now, we have a stub function for this. If we add "unsupported" here, that stub should stop working. So we should either remove the stub, or remove the unsupported flag.

Comment on attachment 8692260 [details] [diff] [review] i18n.json Review of attachment 8692260 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/extensions/schemas/i18n.json @@ +42,5 @@ > + { > + "type": "any", > + "name": "substitutions", > + "optional": true, > + "description": "Up to 9 substitution strings, if the message requires any." We don't actually enforce the "Up to 9".

Comment on attachment 8692258 [details] [diff] [review] page_action.json Review of attachment 8692258 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/extensions/schemas/page_action.json @@ +90,5 @@ > + { > + "type": "object", > + "properties": { > + "19": {"$ref": "ImageDataType", "optional": true}, > + "38": {"$ref": "ImageDataType", "optional": true} We accept any integer key for these too, now (same bug as browser action). Same for path. @@ +116,5 @@ > + "type": "integer", > + "minimum": 0, > + "description": "<b>Deprecated.</b> This argument is ignored.", > + "optional": true > + } Let's get rid of this.

Comment on attachment 8692261 [details] [diff] [review] extension.json Review of attachment 8692261 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/extensions/schemas/extension.json @@ +32,5 @@ > + "functions": [ > + { > + "name": "sendRequest", > + "unsupported": true, > + "deprecated": "Please use $(ref:runtime.sendMessage).", Deprecated and unsupported, we should probably just remove it. Same for |getExtensionTabs|. @@ +104,5 @@ > + } > + } > + }, > + { > + "name": "getBackgroundPage", Unsupported.

Comment on attachment 8692262 [details] [diff] [review] patch Review of attachment 8692262 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/extensions/ext-runtime.js @@ -48,5 @@ > > onConnect: context.messenger.onConnect("runtime.onConnect"), > > - connect: function(...args) { > - let { extensionId, connectInfo } = processRuntimeConnectParams(context.contentWindow, ...args); |processRuntimeConnectParams| is unused now. ::: toolkit/components/extensions/schemas/runtime.json @@ +49,5 @@ > + "enum": ["arm", "x86-32", "x86-64"], > + "description": "The machine's processor architecture." > + }, > + { > + "id": "PlatformNaclArch", Unsupported. @@ +113,5 @@ > + } > + }, > + "functions": [ > + { > + "name": "getBackgroundPage", Unsupported.

This depends on some JS engine work.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a23147905fb3 https://hg.mozilla.org/integration/mozilla-inbound/rev/53004d642d8d https://hg.mozilla.org/integration/mozilla-inbound/rev/a1c13811fea4 https://hg.mozilla.org/integration/mozilla-inbound/rev/5af1998a1366 https://hg.mozilla.org/integration/mozilla-inbound/rev/256b993d4ffc https://hg.mozilla.org/integration/mozilla-inbound/rev/b0110c958530 https://hg.mozilla.org/integration/mozilla-inbound/rev/c074d2c82fd1

Backed out in fc87f6186253 - depending on what flavor you like, b2g emulator mochitest: https://treeherder.mozilla.org/logviewer.html#?job_id=18199823&repo=mozilla-inbound b2g emulator reftest: https://treeherder.mozilla.org/logviewer.html#?job_id=18199838&repo=mozilla-inbound b2g desktop gu: https://treeherder.mozilla.org/logviewer.html#?job_id=18201559&repo=mozilla-inbound lots of sorts of b2g didn't really feel like starting up with you.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c2556808669 https://hg.mozilla.org/integration/mozilla-inbound/rev/d648b84b5aa7 https://hg.mozilla.org/integration/mozilla-inbound/rev/8a1b13c29515 https://hg.mozilla.org/integration/mozilla-inbound/rev/c2fde5cdfaec https://hg.mozilla.org/integration/mozilla-inbound/rev/66b1cfe0d743 https://hg.mozilla.org/integration/mozilla-inbound/rev/4d4e7113daa2 https://hg.mozilla.org/integration/mozilla-inbound/rev/7b0f6544f9b6

https://hg.mozilla.org/mozilla-central/rev/5c2556808669 https://hg.mozilla.org/mozilla-central/rev/d648b84b5aa7 https://hg.mozilla.org/mozilla-central/rev/8a1b13c29515 https://hg.mozilla.org/mozilla-central/rev/c2fde5cdfaec https://hg.mozilla.org/mozilla-central/rev/66b1cfe0d743 https://hg.mozilla.org/mozilla-central/rev/4d4e7113daa2 https://hg.mozilla.org/mozilla-central/rev/7b0f6544f9b6 https://hg.mozilla.org/mozilla-central/rev/76abfb865731

https://hg.mozilla.org/integration/mozilla-inbound/rev/8998af1a9ba8 https://hg.mozilla.org/integration/mozilla-inbound/rev/8600dcc0ae75 https://hg.mozilla.org/integration/mozilla-inbound/rev/6424eb119637 https://hg.mozilla.org/integration/mozilla-inbound/rev/80d23d13059f https://hg.mozilla.org/integration/mozilla-inbound/rev/760f3c2165e0 https://hg.mozilla.org/integration/mozilla-inbound/rev/b2ba92b17aab https://hg.mozilla.org/integration/mozilla-inbound/rev/d570db94c960 https://hg.mozilla.org/integration/mozilla-inbound/rev/65f6e081ded8

https://hg.mozilla.org/mozilla-central/rev/8998af1a9ba8 https://hg.mozilla.org/mozilla-central/rev/8600dcc0ae75 https://hg.mozilla.org/mozilla-central/rev/6424eb119637 https://hg.mozilla.org/mozilla-central/rev/80d23d13059f https://hg.mozilla.org/mozilla-central/rev/760f3c2165e0 https://hg.mozilla.org/mozilla-central/rev/b2ba92b17aab https://hg.mozilla.org/mozilla-central/rev/d570db94c960 https://hg.mozilla.org/mozilla-central/rev/65f6e081ded8

Checking in a small fix where onHistoryStateUpdated wasn't marked as unsupported.

Performed Exploratory testing around this bug on Latest Firefox 48.0a1 (2016-03-23) and noticed the following: - Some errors are thrown in Browser Console after clicking on “OK” button from “hello” dialog: NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIDOMWindowUtils.isParentWindowMainWidgetVisible] nsPrompter.js:346 [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIDOMWindowUtils.isParentWindowMainWidgetVisible]" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)" location: "JS frame :: resource://gre/components/nsPrompter.js :: openModalWindow :: line 346" data: no] (unknown) - This seems to be a regression since this issue is not reproducible on Firefox 46.0a1 (2015-12-30) Last good revision: 211a4c710fb6af2cad10102c4cabc7cb525998b8 First bad revision: 0ecd7d72f232304da046b352c457e039e35ceed7 Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=211a4c710fb6af2cad10102c4cabc7cb525998b8&tochange=0ecd7d72f232304da046b352c457e039e35ceed7 Kris, any thoughts about this? Could this errors be regressed by Bug 1210583 ?

I don't think so. Hello does not these APIs, and none of those changes should have had any external effects.

Are these errors tracked in any bug? Or should I file one? Based on Comment 55 and Comment 56 I am marking this bug as Verified.

I don't know. I'm not involved in that project, but I did a few searches and I don't see any relevant bugs.