Closed Bug 1157714 Opened 9 years ago Closed 9 years ago

browser_monitorUncaught.js is going to permafail when Gecko 40 merges to Aurora

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla41
Tracking Status
firefox40 --- verified
firefox41 --- verified

People

(Reporter: RyanVM, Assigned: Yoric)

References

Details

Attachments

(1 file, 1 obsolete file)

https://treeherder.mozilla.org/logviewer.html#?job_id=6870722&repo=try

23:18:02 INFO - 292 INFO Entering test test_globals
23:18:02 INFO - 293 INFO TEST-UNEXPECTED-FAIL | dom/promise/tests/browser_monitorUncaught.js | We are testing DOM Promise. - "function ()\n{\n\"use strict\";\n\n return new Deferred();\n}" == "undefined" - JS frame :: chrome://mochitests/content/browser/dom/promise/tests/browser_monitorUncaught.js :: test_globals :: line 10
23:18:02 INFO - Stack trace:
23:18:02 INFO - chrome://mochitests/content/browser/dom/promise/tests/browser_monitorUncaught.js:test_globals:10
23:18:02 INFO - self-hosted:next:624
23:18:02 INFO - Tester_execTest@chrome://mochikit/content/browser-test.js:742:9
23:18:02 INFO - Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:665:7
23:18:02 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:744:59
23:18:02 INFO - 294 INFO TEST-PASS | dom/promise/tests/browser_monitorUncaught.js | PromiseDebugging is available. - "function PromiseDebugging() {\n [native code]\n}" != "undefined" 

etc...
Flags: needinfo?(dteller)
Weird. There's nothing in that directory that should be related to the channel.
Wait, what?
This log means that Promise.jsm somehow overwrites DOM Promise in browser tests, at least in this test. That's bad, but that doesn't seem to be caused by DOM Promise. I'll need to investigate some more.
Flags: needinfo?(dteller)
If Yoric is gone through 6-May, that leaves very little time to investigate and fix this prior to the next uplift. Vladan, is there someone else on your team that can look in the mean time?
Flags: needinfo?(vdjeric)
Roberto, could you take a look at this issue while I'm away?
Flags: needinfo?(vdjeric) → needinfo?(rvitillo)
OK, I will have a look at it.
Flags: needinfo?(rvitillo)
I am having an hard time reproducing this on a build with a similar configuration.
It seems that the following happens with Nightly when compiled with the Aurora build configuration:

1. contentAreautils.js imports lazily Promise.jsm in the chrome window [1]
2. when the Promise object is first accessed in browser_monitorUncaught.js [2], Promise.jsm is imported in the window object

In the Nightly build, step 2 never happens as the native Promise implementation is used. The only way there to force the use of Promise.jsm is to access window.Promise. When Nightly is compiled as Aurora though, it seems as if the native Promise implementation is missing.

Vladan, Yoric, do you have any idea what might be causing the behaviour?

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/content/contentAreaUtils.js?from=contentAreaUtils.js&case=true#20
[2] https://dxr.mozilla.org/mozilla-central/source/dom/promise/tests/browser_monitorUncaught.js?from=browser_monitorUncaught.js&case=true#10
Flags: needinfo?(vdjeric)
Flags: needinfo?(dteller)
Are you sure that the native implementation is missing? It was my impression that it was somehow overwritten with Promise.jsm.
Flags: needinfo?(dteller)
Deferring to Yoric on this one
Yoric, you can instrument defineLazyModuleGetter and verify that comment 7 outlines precisely what's happening.

After a conversation with paolo, I came to realize that what happens in Aurora is likely the desired behaviour as Promise.jsm is imported lazily in the global object before the test is executed. It's not clear to me why that very same lazy import has no effect in Nightly though.
Flags: needinfo?(vdjeric)
Overshadowing DOM Promise with Promise.jsm strikes me as odd. Paolo, what makes you think that this is the right behavior? Also, bz, any idea how we could force DOM Promise to take back its place if it is masked?
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(bzbarsky)
There's basically nothing magic about DOM Promise: it's just a property on the global object.  The property is writable, configurable, etc, so people can override it, like any other built-in constructor.  That's what Promise.jsm does.

We could certainly add something that would redefine the global's .Promise to DOM Promise (e.g. on windowutils or some equivalent).  But who would use it and for what purpose?
Flags: needinfo?(bzbarsky)
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #11)
> Overshadowing DOM Promise with Promise.jsm strikes me as odd. Paolo, what
> makes you think that this is the right behavior?

Browser-chrome tests are supposed to be run in the environment of the browser XUL window, they are not supposed to see an environment that is substantially different from the one we have in production.

In the browser XUL window, we load Promise.jsm, so tests would need to see Promise.jsm as well. We use Promise.jsm since it is still ahead of DOM Promise with regard to handling of uncaught rejections.

Promise.jsm is loaded by contentAreaUtils and maybe other scripts. This is done using a <script> tag in a XUL overlay loaded over "browser.xul". The testing infrastructure is also loaded as an overlay of "browser.xul".

I haven't investigated this in detail, but the difference when compiling with the Aurora flags might be caused by some kind of Nightly-only overlay isolation feature in the JavaScript environment?

That said, I've taken a look and the test doesn't seem to depend on having the browser window environment available, or to be testing the browser-chrome framework itself (it's only testing the debugging API and not its use in the testing framework).

Converting this to an xpcshell test might simplify things. On the other hand, I guess there was a reason in the first place why this wasn't an xpcshell test to begin with.
Flags: needinfo?(paolo.mozmail)
> might be caused by some kind of Nightly-only overlay isolation feature

Ah, yes.  dom.compartment_per_addon is only set to true if E10S_TESTING_ONLY is defined, and that's only defined if NIGHTLY_BUILD in configure.in.  I bet that's the issue here, if the testing infra overlay comes from an addon.  If so, it would get its own global scope on nightly, but share one with contentAreaUtils on aurora.
(In reply to Not doing reviews right now from comment #14)
> Ah, yes.  dom.compartment_per_addon is only set to true if E10S_TESTING_ONLY
> is defined, and that's only defined if NIGHTLY_BUILD in configure.in.  I bet
> that's the issue here, if the testing infra overlay comes from an addon.  If
> so, it would get its own global scope on nightly, but share one with
> contentAreaUtils on aurora.

Hm, so if this is the case, maybe a simple solution to get consistency would be for the testing framework, instead of calling loadSubScript directly on the test and head files, to call a helper function in "browser.js" that in turn calls loadSubScript? If I understand correctly, the script would get the global active at the time loadSubScript is called, which would be the one for "browser.js".

The browser_monitorUncaught test would need to be modified to get a reference to DOM Promise from somewhere else than the browser's global scope (or converted to xpcshell).
I'll try and see if it can be converted to xpcshell.
Attached file MozReview Request: bz://1157714/Yoric (obsolete) —
/r/9025 - Bug 1157714 - Moving browser_monitorUncaught to a xpcshell test;r=self

Pull down this commit:

hg pull -r 7a5d3de1959c831ac8980729145437fc073f6a31 https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8607465 [details]
MozReview Request: bz://1157714/Yoric

/r/9025 - Bug 1157714 - Moving browser_monitorUncaught to a xpcshell test;r=self

Pull down this commit:

hg pull -r 6a62bcd962f7302b6068aeac138784be47fabe23 https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8607465 [details]
MozReview Request: bz://1157714/Yoric

Assuming you've published this after testing locally, it looks good to me, if you'd like a second opinion. Thanks for doing this!

Glad to see how using Assert.jsm assertions made the conversion easy.
https://hg.mozilla.org/mozilla-central/rev/c4d20e902d26
Assignee: nobody → dteller
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Whoops, forgot to uplift this.
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
Attachment #8607465 - Attachment is obsolete: true
Status: RESOLVED → VERIFIED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.