1208257 - Omitted optional arguments not handled correctly for all cases

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Will Bamberg [:wbamberg]]

Attachment: 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

[Kris Maglione [:kmag]]

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

[Kris Maglione [:kmag]]

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

[Kris Maglione [:kmag]]

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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 5

[Kris Maglione [:kmag]]

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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 7

[Kris Maglione [:kmag]]

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

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 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

[Kris Maglione [:kmag]]

Yup, I think I can work with that. Thanks

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: webnavigation schema support

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

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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 18

[Kris Maglione [:kmag]]

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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Extension.jsm support for schemas and cookie implementation, v2

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

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: windows.json

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 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

[Kris Maglione [:kmag]]

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

[Kris Maglione [:kmag]]

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

[Kris Maglione [:kmag]]

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

[Kris Maglione [:kmag]]

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

[Kris Maglione [:kmag]]

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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: bookmarks.json

Comment 29

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: idle.json

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

Comment 30

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: page_action.json

Comment 32

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: i18n.json

Comment 34

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: extension.json

Comment 35

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 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

[Kris Maglione [:kmag]]

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

[Kris Maglione [:kmag]]

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

[Kris Maglione [:kmag]]

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

[Kris Maglione [:kmag]]

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

[Kris Maglione [:kmag]]

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

[Kris Maglione [:kmag]]

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

[Kris Maglione [:kmag]]

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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This depends on some JS engine work.

Comment 44

[Pulsebot]

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

Comment 45

[Phil Ringnalda (:philor)]

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

[Pulsebot]

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

[Pulsebot]

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

Comment 48

[Carsten Book [:Tomcat]]

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

[Pulsebot]

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

[Ryan VanderMeulen [:RyanVM]]

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

Comment 51

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 52

[Pulsebot]

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

Comment 53

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d2b5f30e0b9c

Comment 55

[Vasilica Tamas, QA [:vtamas]]

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

[Kris Maglione [:kmag]]

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

Comment 57

[Vasilica Tamas, QA [:vtamas]]

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

[Kris Maglione [:kmag]]

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.