Omitted optional arguments not handled correctly for all cases

VERIFIED FIXED

Status

()

Toolkit
WebExtensions: Untriaged
P3
normal
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: wbamberg, Assigned: billm)

Tracking

({leave-open})

44 Branch
leave-open
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox48 verified)

Details

(Whiteboard: triaged)

Attachments

(15 attachments, 2 obsolete attachments)

2.15 KB, application/x-xpinstall
Details
28.21 KB, patch
kmag
: review+
Details | Diff | Splinter Review
41.59 KB, patch
kmag
: review+
Details | Diff | Splinter Review
21.56 KB, patch
kmag
: review+
Details | Diff | Splinter Review
29.61 KB, patch
kmag
: review+
Details | Diff | Splinter Review
29.00 KB, patch
kmag
: review+
Details | Diff | Splinter Review
71.57 KB, patch
kmag
: review+
Details | Diff | Splinter Review
24.34 KB, patch
kmag
: review+
Details | Diff | Splinter Review
5.43 KB, patch
kmag
: review+
Details | Diff | Splinter Review
32.04 KB, patch
kmag
: review+
Details | Diff | Splinter Review
10.73 KB, patch
kmag
: review+
Details | Diff | Splinter Review
26.73 KB, patch
kmag
: review+
Details | Diff | Splinter Review
7.88 KB, patch
kmag
: review+
Details | Diff | Splinter Review
18.07 KB, patch
kmag
: review+
Details | Diff | Splinter Review
29.54 KB, patch
kmag
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
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.

Updated

2 years ago
Assignee: nobody → kmaglione+bmo
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.
Depends on: 1190677
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
Summary: tabs.executeScript does not work if you omit tabID but include callback → Omitted optional arguments not handled correctly for all cases
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.
Blocks: 1190677
No longer depends on: 1190677
(Assignee)

Updated

2 years ago
Blocks: 1214433
(Assignee)

Updated

2 years ago
Priority: -- → P3

Updated

2 years ago
Flags: blocking-webextensions+
(Assignee)

Comment 4

2 years ago
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.
(Assignee)

Comment 6

2 years ago
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.
Assignee: kmaglione+bmo → wmccloskey
(Assignee)

Comment 8

2 years ago
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.

Updated

2 years ago
Blocks: 1223422
(Assignee)

Comment 9

2 years ago
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.
Yup, I think I can work with that. Thanks
Blocks: 1225715
(Assignee)

Comment 11

a year ago
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.
Attachment #8688192 - Attachment is obsolete: true
Attachment #8689830 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 12

a year ago
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.
Attachment #8689831 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 13

a year ago
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 :-).
Attachment #8689832 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 14

a year ago
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.
Attachment #8689836 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 15

a year ago
I'll keep working on more APIs. I don't think we need to land these all at once though.
(Assignee)

Comment 16

a year ago
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.
Attachment #8689831 - Flags: review?(kmaglione+bmo)
OS: Unspecified → All
Hardware: Unspecified → All
Duplicate of this bug: 1223422
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?
Attachment #8689830 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 19

a year ago
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.
Attachment #8689831 - Attachment is obsolete: true
Attachment #8690350 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 20

a year ago
Created attachment 8690351 [details] [diff] [review]
windows.json
Attachment #8690351 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

a year ago
Attachment #8690351 - Attachment is patch: true
(Assignee)

Comment 21

a year ago
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.
Attachment #8690352 - Flags: review?(kmaglione+bmo)
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.
Attachment #8690350 - Flags: review?(kmaglione+bmo) → review+
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.
Attachment #8689832 - Flags: review?(kmaglione+bmo) → review+
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)
Attachment #8689836 - Flags: review?(kmaglione+bmo) → review+
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?
Attachment #8690351 - Flags: review?(kmaglione+bmo) → review+
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.
Attachment #8690352 - Flags: review?(kmaglione+bmo) → review+
Blocks: 1225215
Blocks: 1066890
No longer blocks: 1223422
Version: unspecified → 44 Branch
(Assignee)

Comment 27

a year ago
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.
(Assignee)

Comment 28

a year ago
Created attachment 8692255 [details] [diff] [review]
bookmarks.json
Attachment #8692255 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 29

a year ago
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.
Attachment #8692256 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 30

a year ago
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.
Attachment #8692257 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 31

a year ago
Created attachment 8692258 [details] [diff] [review]
page_action.json
Attachment #8692258 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 32

a year ago
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.
Attachment #8692259 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 33

a year ago
Created attachment 8692260 [details] [diff] [review]
i18n.json
Attachment #8692260 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 34

a year ago
Created attachment 8692261 [details] [diff] [review]
extension.json
Attachment #8692261 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 35

a year ago
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.
Attachment #8692262 - Flags: review?(kmaglione+bmo)
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.
Attachment #8692257 - Flags: review?(kmaglione+bmo) → review+
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.
Attachment #8692259 - Flags: review?(kmaglione+bmo) → review+
Attachment #8692255 - Flags: review?(kmaglione+bmo) → review+
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.
Attachment #8692256 - Flags: review?(kmaglione+bmo) → review+
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".
Attachment #8692260 - Flags: review?(kmaglione+bmo) → review+
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.
Attachment #8692258 - Flags: review?(kmaglione+bmo) → review+
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.
Attachment #8692261 - Flags: review?(kmaglione+bmo) → review+
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.
Attachment #8692262 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Updated

a year ago
Iteration: --- → 44.3 - Nov 2
(Assignee)

Updated

a year ago
Iteration: 44.3 - Nov 2 → 45.3 - Dec 14
(Assignee)

Comment 43

a year ago
This depends on some JS engine work.
Depends on: 1229579
(Assignee)

Updated

a year ago
Keywords: leave-open

Comment 44

a year ago
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.

Comment 46

a year ago
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

Comment 47

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/76abfb865731

Comment 48

a year ago
bugherder
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

Comment 49

a year ago
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

Comment 50

a year ago
bugherder
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
(Assignee)

Comment 51

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

Comment 52

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2b5f30e0b9c

Comment 53

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d2b5f30e0b9c
Duplicate of this bug: 1190677

Updated

a year ago
Whiteboard: triaged
(Assignee)

Updated

a year ago
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
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 ?
Flags: needinfo?(kmaglione+bmo)
I don't think so. Hello does not these APIs, and none of those changes should have had any external effects.
Flags: needinfo?(kmaglione+bmo)
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.
Status: RESOLVED → VERIFIED
status-firefox48: --- → verified
Flags: needinfo?(kmaglione+bmo)
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.
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.