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

RESOLVED FIXED in Firefox 50

Status

Testing
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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.
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.
Created attachment 8758822 [details]
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/
Attachment #8758822 - Flags: review?(gijskruitbosch+bugs)
Attachment #8758822 - Flags: review?(gijskruitbosch+bugs) → review+
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" ?
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

2 years ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13988ac3bc12
Make openNewBrowserWindow wait for the initial browser load before resolving. r=Gijs

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/13988ac3bc12
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

Updated

2 years ago
See Also: → bug 1292239
Assignee: nobody → mconley
You need to log in before you can comment on or make changes to this bug.