Race in BrowserTestUtils.openNewBrowserWindow()

RESOLVED FIXED in Firefox 50

Status

RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: aswan, Assigned: mconley)

Tracking

Version 3
mozilla51
Points:
---

Firefox Tracking Flags

(firefox50 fixed, firefox51 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8777893 [details]
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.
(Reporter)

Updated

2 years ago
Blocks: 1213990
(Reporter)

Updated

2 years ago
See Also: → bug 1277318
(Assignee)

Comment 2

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

Comment 3

2 years ago
Still doing retriggers, but try looks compelling.

aswan, can you confirm this is going to work for your needs?
Flags: needinfo?(mconley) → needinfo?(aswan)
(Reporter)

Comment 4

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

Comment 5

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

Comment 6

2 years ago
Created attachment 8778380 [details]
Bug 1292239 - Add utility to BrowserTestUtils to wait for load of selected browser in a brand new window.

Review commit: https://reviewboard.mozilla.org/r/69704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69704/
Attachment #8778380 - Flags: review?(felipc)
(Assignee)

Comment 7

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

Comment 8

2 years ago
(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+

Comment 11

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

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/14b1172b12cb
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Assignee: nobody → mconley
Component: BrowserTest → Mochitest
Product: Testing → Testing
You need to log in before you can comment on or make changes to this bug.