Closed Bug 1276738 Opened 9 years ago Closed 9 years ago

Add tests for nsWindowWatcher's window opening behaviours

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(9 files, 1 obsolete file)

MozReview Request: Bug 1276738 - Tag a bunch of tests that exercise opening windows with openwindow. r=me
58 bytes, text/x-review-board-request
mconley
: review+
Details
MozReview Request: Bug 1276738 - Test that new windows opened from remote content get the right flags. r?gabor
58 bytes, text/x-review-board-request
gkrizsanits
: review+
Details
MozReview Request: Bug 1276738 - Test that named windows work properly. r?gabor
58 bytes, text/x-review-board-request
gkrizsanits
: review+
Details
MozReview Request: Bug 1276738 - Test _blank name usage in new windows. r?Gijs
58 bytes, text/x-review-board-request
Gijs
: review+
Details
MozReview Request: Bug 1276738 - Add a test for the size of newly opened window from content. r?gabor
58 bytes, text/x-review-board-request
gkrizsanits
: review+
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
58 bytes, text/x-review-board-request
Gijs
: review+
Details
MozReview Request: Bug 1276738 - Add a test to ensure that we clone sessionStorage when opening new windows. r?gabor
58 bytes, text/x-review-board-request
gkrizsanits
: review+
Details
MozReview Request: Bug 1276738 - Test that modal windows can be opened from the parent process. r?Gijs
58 bytes, text/x-review-board-request
Gijs
: review+
Details
MozReview Request: Bug 1276738 - Test that newly opened dialogs can receive arguments. r?Gijs
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)
Depends on: 1277544
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.

Attachment

General

Creator:
Created:
Updated:
Size: