Closed Bug 1292239 Opened 5 years ago Closed 5 years ago

Race in BrowserTestUtils.openNewBrowserWindow()

Categories

(Testing :: Mochitest, defect)

Version 3
defect
Not set
normal

Tracking

(firefox50 fixed, firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: aswan, Assigned: mconley)

References

Details

Attachments

(2 files)

Attached file browser_window_race.js
The code in BrowserTestUtils that opens a new window and waits for the various events that indicate the window is open and fully initialized has a race condition:
https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#433-443

loadPromise resolves when a browser-test-utils specific message is sent on the browser-specific message manager, but the listener for that message is not created until after the window "load" event has been received.  As I understand it, it isn't practical to create the listener earlier (since the message manager is reached through the window's gBrowser property which isn't valid until after the load event) but depending on the timing, the browser-test-utils message may come arrive before the code that sets up the listener gets an opportunity to run.

Attached is a simple test case that reliably triggers this race, the "startupcache-invalidate" event is observed by the xul prototype cache (https://dxr.mozilla.org/mozilla-central/source/dom/xul/nsXULPrototypeCache.cpp#128-130) and it clears, among other things, its css cache (https://dxr.mozilla.org/mozilla-central/source/dom/xul/nsXULPrototypeCache.cpp#284).  This apparently permutes the timing enough to trigger the race condition described above.
Blocks: 1213990
See Also: → 1277318
This seems to pass your test case. I've added a utility function that uses the window message manager to wait for the selected browser to announce that it has loaded, and I'm having openNewBrowserWindow use that instead.

I've pushed to try - let's see what CI thinks.
Flags: needinfo?(mconley)
Still doing retriggers, but try looks compelling.

aswan, can you confirm this is going to work for your needs?
Flags: needinfo?(mconley) → needinfo?(aswan)
(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #3)
> aswan, can you confirm this is going to work for your needs?

From a quick test, yes it does.  Thanks!
I'll run the full set of tests that I eventually whittled down to this, but this definitely looks promising.
Flags: needinfo?(aswan)
The original test that uncovered this issue is now passing, I would love to see it land (and if there's anything I can to do help, I'm happy to do so)
Flags: needinfo?(mconley)
(In reply to Andrew Swan [:aswan] from comment #5)
> The original test that uncovered this issue is now passing, I would love to
> see it land (and if there's anything I can to do help, I'm happy to do so)

Is central okay, or do you need this uplifted for something?
Flags: needinfo?(mconley) → needinfo?(aswan)
(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #7)
> Is central okay, or do you need this uplifted for something?

central is good.  I don't know the typical processes here but if I end up asking uplift for bug 1213990 (which depends on this) I can test it and request uplift myself if that's okay?
Thanks!
Flags: needinfo?(aswan)
Would be nice to uplift to Aurora. I wouldn't be surprised if we need to uplift changes that depend on this behavior in the next few weeks.
Comment on attachment 8778380 [details]
Bug 1292239 - Add utility to BrowserTestUtils to wait for load of selected browser in a brand new window.

https://reviewboard.mozilla.org/r/69704/#review66910
Attachment #8778380 - Flags: review?(felipc) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/14b1172b12cb
Add utility to BrowserTestUtils to wait for load of selected browser in a brand new window. r=Felipe
https://hg.mozilla.org/mozilla-central/rev/14b1172b12cb
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Assignee: nobody → mconley
Component: BrowserTest → Mochitest
You need to log in before you can comment on or make changes to this bug.