Closed Bug 1276738 Opened 4 years ago Closed 4 years ago

Add tests for nsWindowWatcher's window opening behaviours

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(9 files, 1 obsolete file)

58 bytes, text/x-review-board-request
mconley
: review+
Details
58 bytes, text/x-review-board-request
gkrizsanits
: review+
Details
58 bytes, text/x-review-board-request
gkrizsanits
: review+
Details
58 bytes, text/x-review-board-request
Gijs
: review+
Details
58 bytes, text/x-review-board-request
gkrizsanits
: review+
Details
58 bytes, text/x-review-board-request
Gijs
: review+
Details
58 bytes, text/x-review-board-request
gkrizsanits
: review+
Details
58 bytes, text/x-review-board-request
Gijs
: review+
Details
58 bytes, text/x-review-board-request
Gijs
: review+
Details
I'm planning on refactoring nsWindowWatcher a bit, and so the first step is to write a bunch of tests.

I initially had this work in bug 1267720, but I'm splitting this out because I think I can land it sooner.
There are a series of tests strewn about the tree that seem to exercise window
opening in one form or another, so I thought I'd put them under a tag.

Review commit: https://reviewboard.mozilla.org/r/56412/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56412/
Attachment #8758299 - Flags: review?(mconley)
Attachment #8758300 - Flags: review?(gkrizsanits)
Attachment #8758301 - Flags: review?(gkrizsanits)
Attachment #8758302 - Flags: review?(gijskruitbosch+bugs)
Attachment #8758303 - Flags: review?(ehsan)
Attachment #8758304 - Flags: review?(gkrizsanits)
Attachment #8758305 - Flags: review?(gijskruitbosch+bugs)
Attachment #8758306 - Flags: review?(gkrizsanits)
Attachment #8758307 - Flags: review?(gijskruitbosch+bugs)
Attachment #8758308 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8758299 [details]
MozReview Request: Bug 1276738 - Tag a bunch of tests that exercise opening windows with openwindow. r=me

https://reviewboard.mozilla.org/r/56412/#review53124
Attachment #8758299 - Flags: review?(mconley) → review+
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Comment on attachment 8758302 [details]
MozReview Request: Bug 1276738 - Test _blank name usage in new windows. r?Gijs

https://reviewboard.mozilla.org/r/56418/#review53184

::: embedding/components/windowwatcher/test/test_blank_named_window.html:27
(Diff revision 1)
> +      // of a new tab.
> +      yield SpecialPowers.pushPrefEnv({"set": [
> +        ["browser.link.open_newwindow", 2],
> +      ]});
> +
> +      let win1 = window.open("about:blank", "_blank");

In order to make these easier to identify if they leak or come up as 'leftover' windows when the test is done, I would prefer if we used something like this for the URI: data:text/html,<p>This is window 1 for test_blank_named_window.html</p>

::: embedding/components/windowwatcher/test/test_blank_named_window.html:40
(Diff revision 1)
> +      win1.close();
> +      win2.close();

Do we need to wait for this to finish in some way to avoid "this test left behind window X" ? or whatever?
Attachment #8758302 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8758305 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8758305 [details]
MozReview Request: Bug 1276738 - Ensure that .open() on web content called with chrome privileges results in a new window with the appropriate principal. r?Gijs

https://reviewboard.mozilla.org/r/56424/#review53186

(almost there, but no r+ because of the race)

::: embedding/components/windowwatcher/test/browser_new_content_window_from_chrome_principal.js:3
(Diff revision 1)
> +// This magic value of 2 means that by default, when content tries
> +// to open a new window, it'll actually open in a new window instead
> +// of a new tab.
> +Services.prefs.setIntPref("browser.link.open_newwindow", 2);
> +registerCleanupFunction(() => {
> +  Services.prefs.clearUserPref("browser.link.open_newwindow");
> +});

You can use yield SpecialPowers.pushPrefEnv even from browser tests.

::: embedding/components/windowwatcher/test/browser_new_content_window_from_chrome_principal.js:25
(Diff revision 1)
> +    content.open("http://example.com", "_blank");
> +  });
> +
> +  let win = yield newWinPromise;
> +  let browser = win.gBrowser.selectedBrowser;
> +  yield BrowserTestUtils.browserLoaded(browser);

This is going to race, IME. There's no guarantee that the load event for example.com will happen before or after delayed-startup happens for the parent window.

I don't have a great idea for how to fix that beyond using a content task to check if the browser has loaded or not, and jiggling that about with waiting for the content message that indicates the load has happened.

::: embedding/components/windowwatcher/test/browser_new_content_window_from_chrome_principal.js:33
(Diff revision 1)
> +    Assert.equal(content.document.nodePrincipal.origin,
> +                 "http://example.com",
> +                 "Should have the example.com principal");
> +  });
> +
> +  yield BrowserTestUtils.closeWindow(win);

Opposite question to the mochitest-plain test here: do we need the yield etc. (see also bug 1274958 which is trying to clarify some of this)
Comment on attachment 8758307 [details]
MozReview Request: Bug 1276738 - Test that modal windows can be opened from the parent process. r?Gijs

https://reviewboard.mozilla.org/r/56428/#review53192

::: embedding/components/windowwatcher/test/test_modal_windows.html:20
(Diff revision 1)
> +  <script type="application/javascript;version=1.8">
> +  const {utils: Cu, interfaces: Ci} = Components;
> +
> +  Cu.import("resource://gre/modules/Services.jsm");
> +
> +  function testModalWindow() {

Maybe just import BrowserTestUtils and use `BrowserTestUtils.domWindowOpened()` instead?

::: embedding/components/windowwatcher/test/test_modal_windows.html:52
(Diff revision 1)
> +      ok(wbc.isWindowModal(), "Should report as modal");
> +
> +      win.close();
> +    });
> +
> +    let modal = window.openDialog("about:blank", "_blank", "modal", null);

.then() callbacks are called back async, so shouldn't we be waiting before resolving the add_task promise?

(I think it might be safe right now because of how promises are scheduled and the test framework will use the same thing to evaluate whether the task has finished, but that seems fragile.)

Here, too, I think it would be good to use a unique URL.
Attachment #8758307 - Flags: review?(gijskruitbosch+bugs) → review+
https://reviewboard.mozilla.org/r/56424/#review53208

::: embedding/components/windowwatcher/test/browser_new_content_window_from_chrome_principal.js:28
(Diff revision 1)
> +  let win = yield newWinPromise;
> +  let browser = win.gBrowser.selectedBrowser;
> +  yield BrowserTestUtils.browserLoaded(browser);
> +
> +  yield ContentTask.spawn(browser, null, function*() {
> +    Assert.equal(content.document.nodePrincipal.origin,

On second thought, does it make sense to also assert that `!content.document.nodePrincipal.isSystemPrincipal` ?
Attachment #8758308 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8758308 [details]
MozReview Request: Bug 1276738 - Test that newly opened dialogs can receive arguments. r?Gijs

https://reviewboard.mozilla.org/r/56430/#review53210
https://reviewboard.mozilla.org/r/56418/#review53184

> In order to make these easier to identify if they leak or come up as 'leftover' windows when the test is done, I would prefer if we used something like this for the URI: data:text/html,<p>This is window 1 for test_blank_named_window.html</p>

Done, thanks!

> Do we need to wait for this to finish in some way to avoid "this test left behind window X" ? or whatever?

I don't believe so, no - the other mochitests I've looked at seem to do things this way. *shrug*.
https://reviewboard.mozilla.org/r/56424/#review53208

> On second thought, does it make sense to also assert that `!content.document.nodePrincipal.isSystemPrincipal` ?

Sure.
https://reviewboard.mozilla.org/r/56424/#review53186

> You can use yield SpecialPowers.pushPrefEnv even from browser tests.

Ah, missed this one. Thanks.

> This is going to race, IME. There's no guarantee that the load event for example.com will happen before or after delayed-startup happens for the parent window.
> 
> I don't have a great idea for how to fix that beyond using a content task to check if the browser has loaded or not, and jiggling that about with waiting for the content message that indicates the load has happened.

Yeah, good call. I think I can modify waitForNewWindow to try to smooth this over. Patch coming up.

> Opposite question to the mochitest-plain test here: do we need the yield etc. (see also bug 1274958 which is trying to clarify some of this)

I guess I'm using it here because it's availble to me. I don't believe BrowserTestUtils is available to me in standard mochitests.

And the reason I'm using it is to reduce asynchronous behaviours (like reactions from post-window closing messages) from occurring between tests.

I suppose if I had a choice, I'd have something similar to BrowserTestUtils.closeWindow for my mochitest as well. I suppose if it's important enough, I could add one to SpecialPowers. Do you think this is the case?
Comment on attachment 8758299 [details]
MozReview Request: Bug 1276738 - Tag a bunch of tests that exercise opening windows with openwindow. r=me

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56412/diff/1-2/
Attachment #8758305 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8758300 [details]
MozReview Request: Bug 1276738 - Test that new windows opened from remote content get the right flags. r?gabor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56414/diff/1-2/
Comment on attachment 8758301 [details]
MozReview Request: Bug 1276738 - Test that named windows work properly. r?gabor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56416/diff/1-2/
Comment on attachment 8758302 [details]
MozReview Request: Bug 1276738 - Test _blank name usage in new windows. r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56418/diff/1-2/
Comment on attachment 8758304 [details]
MozReview Request: Bug 1276738 - Add a test for the size of newly opened window from content. r?gabor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56422/diff/1-2/
Comment on attachment 8758305 [details]
MozReview Request: Bug 1276738 - Ensure that .open() on web content called with chrome privileges results in a new window with the appropriate principal. r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56424/diff/1-2/
Comment on attachment 8758306 [details]
MozReview Request: Bug 1276738 - Add a test to ensure that we clone sessionStorage when opening new windows. r?gabor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56426/diff/1-2/
Comment on attachment 8758307 [details]
MozReview Request: Bug 1276738 - Test that modal windows can be opened from the parent process. r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56428/diff/1-2/
Comment on attachment 8758308 [details]
MozReview Request: Bug 1276738 - Test that newly opened dialogs can receive arguments. r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56430/diff/1-2/
Attachment #8758303 - Attachment is obsolete: true
Attachment #8758303 - Flags: review?(ehsan)
https://reviewboard.mozilla.org/r/56424/#review53186

> I guess I'm using it here because it's availble to me. I don't believe BrowserTestUtils is available to me in standard mochitests.
> 
> And the reason I'm using it is to reduce asynchronous behaviours (like reactions from post-window closing messages) from occurring between tests.
> 
> I suppose if I had a choice, I'd have something similar to BrowserTestUtils.closeWindow for my mochitest as well. I suppose if it's important enough, I could add one to SpecialPowers. Do you think this is the case?

Upon consideration of this, however, I think it's less necessary to wait for the window session traffic to cease between tests for mochitests. My reasoning is that mochitest-plain tests, for the most part, should behave the same way, regardless of what sessionstore tests are in flight. So for now, I'm going to drop this issue, but I'm going to needinfo you to get your opinion too.
ni to Gijs on comment 29.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #30)
> ni to Gijs on comment 29.

If we don't need it for these tests, and try is green, I will be the last person to say that we should start adding them. :-)
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8758305 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8758305 [details]
MozReview Request: Bug 1276738 - Ensure that .open() on web content called with chrome privileges results in a new window with the appropriate principal. r?Gijs

https://reviewboard.mozilla.org/r/56424/#review53380

LGTM!
Comment on attachment 8758300 [details]
MozReview Request: Bug 1276738 - Test that new windows opened from remote content get the right flags. r?gabor

https://reviewboard.mozilla.org/r/56414/#review53576

::: embedding/components/windowwatcher/test/browser_new_remote_window_flags.js:49
(Diff revision 2)
> +/**
> + * Content can open a window using a target="_blank" link
> + */
> +add_task(function* test_new_remote_window_flags() {

I think you forgot to update the comment after copy paste here, and the function* name.

::: embedding/components/windowwatcher/test/browser_new_remote_window_flags.js:65
(Diff revision 2)
> +/**
> + * Content can open a window using a target="_blank" link
> + */
> +add_task(function* test_new_remote_window_flags() {

Same here.
Attachment #8758300 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8758301 [details]
MozReview Request: Bug 1276738 - Test that named windows work properly. r?gabor

https://reviewboard.mozilla.org/r/56416/#review53580

::: embedding/components/windowwatcher/test/test_named_window.html:43
(Diff revision 2)
> +      let win2 = window.open("about:blank#2", "my_window");
> +      is(win1, win2, "Should have gotten back the same window");

Shouldn't we test the location.href of the window here too?

::: embedding/components/windowwatcher/test/test_named_window.html:50
(Diff revision 2)
> +
> +      let link = document.getElementById("link");
> +      link.setAttribute("target", NAME);
> +      link.click();
> +
> +      yield new Promise(resolve => SimpleTest.executeSoon(resolve));

I'm a bit concerned that spinning the event loop here is not enough to guarantee the success of this test. Even though loading about:blank should not take long... I would prefer using a progress listener here. (and for win2 test above as well if needed, I'm not sure how does window.open act in that case)
Attachment #8758301 - Flags: review?(gkrizsanits)
Comment on attachment 8758306 [details]
MozReview Request: Bug 1276738 - Add a test to ensure that we clone sessionStorage when opening new windows. r?gabor

https://reviewboard.mozilla.org/r/56426/#review53588
Attachment #8758306 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8758304 [details]
MozReview Request: Bug 1276738 - Add a test for the size of newly opened window from content. r?gabor

https://reviewboard.mozilla.org/r/56422/#review53598

::: embedding/components/windowwatcher/test/browser.ini:7
(Diff revision 2)
>  tags = openwindow
>  
>  [browser_new_remote_window_flags.js]
>  run-if = e10s
> +[browser_new_sized_window.js]
> +skip-if = os == 'win' # Bug 1276802 - Opening windows from content on Windows might not get the size right

weird... this works for me on windows
Attachment #8758304 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8758300 [details]
MozReview Request: Bug 1276738 - Test that new windows opened from remote content get the right flags. r?gabor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56414/diff/2-3/
Attachment #8758301 - Flags: review?(gkrizsanits)
Comment on attachment 8758301 [details]
MozReview Request: Bug 1276738 - Test that named windows work properly. r?gabor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56416/diff/2-3/
Comment on attachment 8758302 [details]
MozReview Request: Bug 1276738 - Test _blank name usage in new windows. r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56418/diff/2-3/
Comment on attachment 8758304 [details]
MozReview Request: Bug 1276738 - Add a test for the size of newly opened window from content. r?gabor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56422/diff/2-3/
Comment on attachment 8758305 [details]
MozReview Request: Bug 1276738 - Ensure that .open() on web content called with chrome privileges results in a new window with the appropriate principal. r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56424/diff/2-3/
Comment on attachment 8758306 [details]
MozReview Request: Bug 1276738 - Add a test to ensure that we clone sessionStorage when opening new windows. r?gabor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56426/diff/2-3/
Comment on attachment 8758307 [details]
MozReview Request: Bug 1276738 - Test that modal windows can be opened from the parent process. r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56428/diff/2-3/
Comment on attachment 8758308 [details]
MozReview Request: Bug 1276738 - Test that newly opened dialogs can receive arguments. r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56430/diff/2-3/
Comment on attachment 8758301 [details]
MozReview Request: Bug 1276738 - Test that named windows work properly. r?gabor

https://reviewboard.mozilla.org/r/56416/#review53678

Big thanks for these tests!
Attachment #8758301 - Flags: review?(gkrizsanits) → review+
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #45)
> Comment on attachment 8758301 [details]
> MozReview Request: Bug 1276738 - Test that named windows work properly.
> r?gabor
> 
> https://reviewboard.mozilla.org/r/56416/#review53678
> 
> Big thanks for these tests!

Big thanks for the reviews! :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d903a6a12aa7182252ec3e220c89b569c14e11b
Bug 1276738 - Tag a bunch of tests that exercise opening windows with openwindow. r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/88f049f304465bb2aaee131d775d6ea95bfa7502
Bug 1276738 - Test that new windows opened from remote content get the right flags. r=gabor

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d23fffa886b0b78a2d6f9584f05e4af9a019c63
Bug 1276738 - Test that named windows work properly. r=gabor

https://hg.mozilla.org/integration/mozilla-inbound/rev/73f7e5190b38c89efc0e8c68c84770097e9f95e4
Bug 1276738 - Test _blank name usage in new windows. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a7d17e9e15d84f22d56a056cb48c28b3ba23c7b
Bug 1276738 - Add a test for the size of newly opened window from content. r=gabor

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea2da46bde0a4a3039d69e198ad40afefc92a5ec
Bug 1276738 - Ensure that .open() on web content called with chrome privileges results in a new window with the appropriate principal. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac161595496d7605cf4aae0282c1451484fea91
Bug 1276738 - Add a test to ensure that we clone sessionStorage when opening new windows. r=gabor

https://hg.mozilla.org/integration/mozilla-inbound/rev/c60082ec436a31a40cc03cb99191e0f715592eef
Bug 1276738 - Test that modal windows can be opened from the parent process. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/14e071360b24812c8a9107ae7adbba47e75f28b4
Bug 1276738 - Test that newly opened dialogs can receive arguments. r=Gijs
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=29272368&repo=mozilla-inbound
Flags: needinfo?(mconley)
https://hg.mozilla.org/integration/mozilla-inbound/rev/762c89ba596c7d27d241d0805b24ed960dc0b9eb
Bug 1276738 - Tag a bunch of tests that exercise opening windows with openwindow. r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a1f29170541959dccb0c8f8942689370bb99515
Bug 1276738 - Test that new windows opened from remote content get the right flags. r=gabor

https://hg.mozilla.org/integration/mozilla-inbound/rev/967e0e4a0f0a1452d1fdeeea1c9ed373e6f17c2e
Bug 1276738 - Test that named windows work properly. r=gabor

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1e4d8d8f4d3e861a7b2dc2390002e45d1d4d526
Bug 1276738 - Test _blank name usage in new windows. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/c66723bf292698444a723269dca9c9bb58ab3d21
Bug 1276738 - Add a test for the size of newly opened window from content. r=gabor

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5abd4c6319dc6296fa322cac1c4e0f853ae6757
Bug 1276738 - Ensure that .open() on web content called with chrome privileges results in a new window with the appropriate principal. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/0997a7d078e2ee01436c24a22cc894589a57f3f8
Bug 1276738 - Add a test to ensure that we clone sessionStorage when opening new windows. r=gabor

https://hg.mozilla.org/integration/mozilla-inbound/rev/695bae385079c1dc5f7e3e805a12815e7ea1da74
Bug 1276738 - Test that modal windows can be opened from the parent process. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/f78335c964c461d2c46155ead9a1ce96ab0bb970
Bug 1276738 - Test that newly opened dialogs can receive arguments. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f96e5937d9f151fff347a60159082188d467b5
Bug 1276738 - Disable windowwatcher mochitests for Android. r=test-only
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa3af24c15258ba985b42ba8b8cd591bd324c010
Bug 1276738 - Follow-up. Adjust note in mochitest.ini regarding why tests are disabled for Android. r=test-only DONTBUILD
Flags: needinfo?(mconley)
You need to log in before you can comment on or make changes to this bug.