Last Comment Bug 1208257 - Omitted optional arguments not handled correctly for all cases
: Omitted optional arguments not handled correctly for all cases
Status: VERIFIED FIXED
triaged
: leave-open
Product: Toolkit
Classification: Components
Component: WebExtensions: Untriaged (show other bugs)
: 44 Branch
: All All
P3 normal with 2 votes (vote)
: ---
Assigned To: Bill McCloskey (:billm)
:
: Andy McKay [:andym]
Mentors:
: 1190677 1223422 (view as bug list)
Depends on: 1229579
Blocks: webext-port-avira 1190677 webext1.0 1225215 1225715
  Show dependency treegraph
 
Reported: 2015-09-24 16:10 PDT by Will Bamberg [:wbamberg]
Modified: 2016-03-25 14:14 PDT (History)
8 users (show)
amckay: blocking‑webextensions+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: 45.3 - Dec 14
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
execute.xpi (2.15 KB, application/x-xpinstall)
2015-09-24 16:10 PDT, Will Bamberg [:wbamberg]
no flags Details
schema patch (144.54 KB, patch)
2015-11-16 17:49 PST, Bill McCloskey (:billm)
no flags Details | Diff | Splinter Review
schema parsing/checking (28.21 KB, patch)
2015-11-19 16:46 PST, Bill McCloskey (:billm)
kmaglione+bmo: review+
Details | Diff | Splinter Review
Extension.jsm support for schemas and cookie implementation (28.21 KB, patch)
2015-11-19 16:48 PST, Bill McCloskey (:billm)
no flags Details | Diff | Splinter Review
webrequest schema support (41.59 KB, patch)
2015-11-19 16:50 PST, Bill McCloskey (:billm)
kmaglione+bmo: review+
Details | Diff | Splinter Review
webnavigation schema support (21.56 KB, patch)
2015-11-19 16:53 PST, Bill McCloskey (:billm)
kmaglione+bmo: review+
Details | Diff | Splinter Review
Extension.jsm support for schemas and cookie implementation, v2 (29.61 KB, patch)
2015-11-20 20:38 PST, Bill McCloskey (:billm)
kmaglione+bmo: review+
Details | Diff | Splinter Review
windows.json (29.00 KB, patch)
2015-11-20 20:39 PST, Bill McCloskey (:billm)
kmaglione+bmo: review+
Details | Diff | Splinter Review
tabs.json (71.57 KB, patch)
2015-11-20 20:40 PST, Bill McCloskey (:billm)
kmaglione+bmo: review+
Details | Diff | Splinter Review
bookmarks.json (24.34 KB, patch)
2015-11-25 17:16 PST, Bill McCloskey (:billm)
kmaglione+bmo: review+
Details | Diff | Splinter Review
idle.json (5.43 KB, patch)
2015-11-25 17:16 PST, Bill McCloskey (:billm)
kmaglione+bmo: review+
Details | Diff | Splinter Review
browser_action.json (32.04 KB, patch)
2015-11-25 17:19 PST, Bill McCloskey (:billm)
kmaglione+bmo: review+
Details | Diff | Splinter Review
page_action.json (10.73 KB, patch)
2015-11-25 17:19 PST, Bill McCloskey (:billm)
kmaglione+bmo: review+
Details | Diff | Splinter Review
context_menu.json (26.73 KB, patch)
2015-11-25 17:21 PST, Bill McCloskey (:billm)
kmaglione+bmo: review+
Details | Diff | Splinter Review
i18n.json (7.88 KB, patch)
2015-11-25 17:21 PST, Bill McCloskey (:billm)
kmaglione+bmo: review+
Details | Diff | Splinter Review
extension.json (18.07 KB, patch)
2015-11-25 17:22 PST, Bill McCloskey (:billm)
kmaglione+bmo: review+
Details | Diff | Splinter Review
patch (29.54 KB, patch)
2015-11-25 17:23 PST, Bill McCloskey (:billm)
kmaglione+bmo: review+
Details | Diff | Splinter Review

Description User image Will Bamberg [:wbamberg] 2015-09-24 16:10:54 PDT
Created attachment 8665691 [details]
execute.xpi

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.
Comment 1 User image Kris Maglione [:kmag] 2015-09-29 18:20:18 PDT
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.
Comment 2 User image Kris Maglione [:kmag] 2015-10-01 14:47:07 PDT
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
Comment 3 User image Kris Maglione [:kmag] 2015-10-08 00:52:45 PDT
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.
Comment 4 User image Bill McCloskey (:billm) 2015-11-13 20:44:38 PST
Hey Kris, do you mind if I steal this from you? I need something like this to implement the OOP stuff.
Comment 5 User image Kris Maglione [:kmag] 2015-11-13 21:05:51 PST
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.
Comment 6 User image Bill McCloskey (:billm) 2015-11-13 21:49:57 PST
Sorry, I already started on it :-|. I think I'll have it done over the weekend.
Comment 7 User image Kris Maglione [:kmag] 2015-11-13 22:29:50 PST
It's fine. There are other things I can work on instead.
Comment 8 User image Bill McCloskey (:billm) 2015-11-14 18:12:54 PST
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.
Comment 9 User image Bill McCloskey (:billm) 2015-11-16 17:49:44 PST
Created attachment 8688192 [details] [diff] [review]
schema patch

Hey Kris, here's what I have so far. Hopefully it unblocks you. I'll try to finish it up tomorrow.
Comment 10 User image Kris Maglione [:kmag] 2015-11-16 22:29:09 PST
Yup, I think I can work with that. Thanks
Comment 11 User image Bill McCloskey (:billm) 2015-11-19 16:46:59 PST
Created attachment 8689830 [details] [diff] [review]
schema parsing/checking

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.
Comment 12 User image Bill McCloskey (:billm) 2015-11-19 16:48:56 PST
Created attachment 8689831 [details] [diff] [review]
Extension.jsm support for schemas and cookie implementation

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.
Comment 13 User image Bill McCloskey (:billm) 2015-11-19 16:50:04 PST
Created attachment 8689832 [details] [diff] [review]
webrequest schema support

This patch switches webrequest to use schema validation. In doing so, I found that our test actually passed the filter parameter incorrectly :-).
Comment 14 User image Bill McCloskey (:billm) 2015-11-19 16:53:24 PST
Created attachment 8689836 [details] [diff] [review]
webnavigation schema support

Same thing for webNavigation. Bug 1190329 has a test to make sure this isn't totally broken.
Comment 15 User image Bill McCloskey (:billm) 2015-11-19 16:54:23 PST
I'll keep working on more APIs. I don't think we need to land these all at once though.
Comment 16 User image Bill McCloskey (:billm) 2015-11-19 20:24:40 PST
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 17 User image Kris Maglione [:kmag] 2015-11-20 16:09:09 PST
*** Bug 1223422 has been marked as a duplicate of this bug. ***
Comment 18 User image Kris Maglione [:kmag] 2015-11-20 18:12:23 PST
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?
Comment 19 User image Bill McCloskey (:billm) 2015-11-20 20:38:01 PST
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.
Comment 20 User image Bill McCloskey (:billm) 2015-11-20 20:39:46 PST
Created attachment 8690351 [details] [diff] [review]
windows.json
Comment 21 User image Bill McCloskey (:billm) 2015-11-20 20:40:59 PST
Created attachment 8690352 [details] [diff] [review]
tabs.json

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 22 User image Kris Maglione [:kmag] 2015-11-21 14:49:38 PST
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 23 User image Kris Maglione [:kmag] 2015-11-21 15:03:35 PST
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 24 User image Kris Maglione [:kmag] 2015-11-21 15:27:53 PST
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 25 User image Kris Maglione [:kmag] 2015-11-21 15:42:08 PST
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 26 User image Kris Maglione [:kmag] 2015-11-21 16:30:47 PST
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.
Comment 27 User image Bill McCloskey (:billm) 2015-11-25 17:15:16 PST
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.
Comment 28 User image Bill McCloskey (:billm) 2015-11-25 17:16:03 PST
Created attachment 8692255 [details] [diff] [review]
bookmarks.json
Comment 29 User image Bill McCloskey (:billm) 2015-11-25 17:16:56 PST
Created attachment 8692256 [details] [diff] [review]
idle.json

We don't have tests for this API, but our "implementation" is basically empty, so it's probably fine.
Comment 30 User image Bill McCloskey (:billm) 2015-11-25 17:19:13 PST
Created attachment 8692257 [details] [diff] [review]
browser_action.json

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.
Comment 31 User image Bill McCloskey (:billm) 2015-11-25 17:19:48 PST
Created attachment 8692258 [details] [diff] [review]
page_action.json
Comment 32 User image Bill McCloskey (:billm) 2015-11-25 17:21:06 PST
Created attachment 8692259 [details] [diff] [review]
context_menu.json

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.
Comment 33 User image Bill McCloskey (:billm) 2015-11-25 17:21:35 PST
Created attachment 8692260 [details] [diff] [review]
i18n.json
Comment 34 User image Bill McCloskey (:billm) 2015-11-25 17:22:22 PST
Created attachment 8692261 [details] [diff] [review]
extension.json
Comment 35 User image Bill McCloskey (:billm) 2015-11-25 17:23:30 PST
Created attachment 8692262 [details] [diff] [review]
patch

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 36 User image Kris Maglione [:kmag] 2015-11-27 21:00:20 PST
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 37 User image Kris Maglione [:kmag] 2015-11-27 21:02:34 PST
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 38 User image Kris Maglione [:kmag] 2015-11-27 21:15:58 PST
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 39 User image Kris Maglione [:kmag] 2015-11-27 21:17:54 PST
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 40 User image Kris Maglione [:kmag] 2015-11-27 21:21:59 PST
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 41 User image Kris Maglione [:kmag] 2015-11-27 21:31:19 PST
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 42 User image Kris Maglione [:kmag] 2015-11-27 22:14:50 PST
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.
Comment 43 User image Bill McCloskey (:billm) 2015-12-01 15:02:45 PST
This depends on some JS engine work.
Comment 45 User image Phil Ringnalda (:philor) 2015-12-02 22:23:38 PST
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.
Comment 51 User image Bill McCloskey (:billm) 2015-12-29 15:49:37 PST
Checking in a small fix where onHistoryStateUpdated wasn't marked as unsupported.
Comment 53 User image Carsten Book [:Tomcat] 2015-12-30 03:07:11 PST
https://hg.mozilla.org/mozilla-central/rev/d2b5f30e0b9c
Comment 54 User image Kris Maglione [:kmag] 2016-01-08 19:43:14 PST
*** Bug 1190677 has been marked as a duplicate of this bug. ***
Comment 55 User image Vasilica Mihasca, QA [:vasilica_mihasca] 2016-03-24 06:21:28 PDT
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 ?
Comment 56 User image Kris Maglione [:kmag] 2016-03-24 15:23:59 PDT
I don't think so. Hello does not these APIs, and none of those changes should have had any external effects.
Comment 57 User image Vasilica Mihasca, QA [:vasilica_mihasca] 2016-03-25 06:42:20 PDT
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.
Comment 58 User image Kris Maglione [:kmag] 2016-03-25 14:14:16 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.