Andrew Swan [:aswan] Reporter
	Description • 8 years ago

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.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 1 • 8 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 2 • 8 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.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 3 • 8 years ago

Still doing retriggers, but try looks compelling.

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

	Andrew Swan [:aswan] Reporter
	Comment 4 • 8 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.

	Andrew Swan [:aswan] Reporter
	Comment 5 • 8 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)

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 6 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/69704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69704/

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 7 • 8 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?

	Andrew Swan [:aswan] Reporter
	Comment 8 • 8 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!

	Kris Maglione [:kmag]
	Comment 9 • 8 years ago

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.

	:Felipe Gomes (needinfo for replies!)
	Comment 10 • 8 years ago

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

	Pulsebot
	Comment 11 • 8 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

	Phil Ringnalda (:philor)
	Comment 12 • 8 years ago
bugherder

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

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 8 years ago
bugherder uplift

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