1292239 - Race in BrowserTestUtils.openNewBrowserWindow()

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Andrew Swan [:aswan]]

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

Comment 1

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac27c63f24a0

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Still doing retriggers, but try looks compelling.

aswan, can you confirm this is going to work for your needs?

Comment 4

[Andrew Swan [: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.

Comment 5

[Andrew Swan [: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)

Comment 6

[Mike Conley (:mconley) (:⚙️)]

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

Comment 7

[Mike Conley (: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?

Comment 8

[Andrew Swan [: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!

Comment 9

[Kris Maglione [:kmag]]

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 10

[:Felipe Gomes (needinfo for replies!)]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/14b1172b12cb

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3a73ccd4e90e