1277318 - Make BrowserTestUtils.openNewBrowserWindow wait for the initial browser load before resolving

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

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

Suppose we have the following actions occurring in a test:

add_task(function* my_test() {
  // ... setup...
  let win = yield BrowserTestUtils.openNewBrowserWindow();
  let browser = win.gBrowser.selectedBrowser;
  browser.loadURI("http://www.example.com");
  yield BrowserTestUtils.browserLoaded(browser);
});

openNewBrowserWindow only waits for the delayed startup notification to fire and does _not_ wait for the initial browser load event (the initial browser might load about:blank, or something else, depending on whether or not a message or command to load a page arrived in time to stop the initial about:blank load).

This means that it's possible for the browserLoaded Promise in the last part of my hypothetical test to resolve due to the _initial_ about:blank load instead of the example.com load that the test cares about.

This bug is about making openNewBrowserWindow not resolve until the initial browser has finished its load as well as the delayed startup notification having been fired.

Comment 1

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

It's possible there are some tests that actually depend upon openNewBrowserWindow resolving _before_ the initial browser load. I'm not sure what those tests will be. They're probably pretty fragile.

There might also be tests in the tree that attempt to work around the issue by waiting for the about:blank. It wouldn't surprise me if these cause intermittents in the case where the about:blank load fires before delayed startup is done.

I guess we'll see where the problems are when I push to try.

Comment 2

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

Attachment: MozReview Request: Bug 1277318 - Make openNewBrowserWindow wait for the initial browser load before resolving. r?Gijs

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

Comment 3

[:Gijs (he/him)]

Comment on attachment 8758822 [details]
MozReview Request: Bug 1277318 - Make openNewBrowserWindow wait for the initial browser load before resolving. r?Gijs

https://reviewboard.mozilla.org/r/56954/#review53918

Looks OK but see below.

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:404
(Diff revision 1)
>        features, argString);
>  
>      // Wait for browser-delayed-startup-finished notification, it indicates
>      // that the window has loaded completely and is ready to be used for
>      // testing.
> -    return TestUtils.topicObserved("browser-delayed-startup-finished",
> +    yield this.waitForEvent(win, "load");

Why "load" rather than "DOMContentLoaded" ?

Comment 4

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

https://reviewboard.mozilla.org/r/56954/#review53918

> Why "load" rather than "DOMContentLoaded" ?

I guess I'm trying to look ahead. I'm working on a patch that will flip the remoteness of the initial browser on DOMContentLoaded, and I want to make sure that we do that before we proceed with openNewBrowserWindow. I imagine it's deterministic, and that one DOMContentLoaded event handler will always fire before the other, but I didn't really bother to check which one it is, and opted for waiting for `load`. Gonna drop the issue and land this - let me know if you've got concerns. :)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13988ac3bc12
Make openNewBrowserWindow wait for the initial browser load before resolving. r=Gijs

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/13988ac3bc12