1370100 - Make browser_dead_object.js correctly wait for the window to be destroyed instead of relying on the scheduling of the corresponding event

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Comment 1

[(no longer active)]

Attachment: Make browser_dead_object.js correctly wait for the window to be destroyed instead of relying on the scheduling of the corresponding event

Kris, I have a few more of these fixes that I had forgotten to submit for review.  Since you reviewed the last two, do you mind taking a look at the rest also since they're all essentially the same kind of fix only applied to different tests across the tree?  Thanks!

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8874245 [details] [diff] [review]
Make browser_dead_object.js correctly wait for the window to be destroyed instead of relying on the scheduling of the corresponding event

Review of attachment 8874245 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/tests/browser/browser_dead_object.js
@@ +9,5 @@
>    const url = "http://mochi.test:8888/browser/js/xpconnect/tests/browser/browser_deadObjectOnUnload.html";
>    let newTab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, url);
> +  let dwu = content.window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> +                          .getInterface(Components.interfaces.nsIDOMWindowUtils);
> +  let innerWindowId = dwu.currentInnerWindowID;

Please use `browser.innerWindowID` instead, or do this from inside the ContentTask. This shouldn't work without CPOWs in e10s mode, and the no-cpows-in-tests ESLint rule should forbid it in any case.

@@ +16,2 @@
>      let doc = content.document;
> +    Components.utils.import("resource://testing-common/TestUtils.jsm");

The toolkit/ and browser/ ESLint rules don't like global imports like this in browser mochitests, even though it should be perfectly fine in a ContentTask. I don't think those rules are enabled for xpconnect, but it would be nice to comply with them in new code in case we want to enable them in the future.

Comment 3

[(no longer active)]

(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #2)
> @@ +16,2 @@
> >      let doc = content.document;
> > +    Components.utils.import("resource://testing-common/TestUtils.jsm");
> 
> The toolkit/ and browser/ ESLint rules don't like global imports like this
> in browser mochitests, even though it should be perfectly fine in a
> ContentTask. I don't think those rules are enabled for xpconnect, but it
> would be nice to comply with them in new code in case we want to enable them
> in the future.

What should I do instead?

Comment 4

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #3)
> (In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #2)
> > @@ +16,2 @@
> > >      let doc = content.document;
> > > +    Components.utils.import("resource://testing-common/TestUtils.jsm");
> > 
> > The toolkit/ and browser/ ESLint rules don't like global imports like this
> > in browser mochitests, even though it should be perfectly fine in a
> > ContentTask. I don't think those rules are enabled for xpconnect, but it
> > would be nice to comply with them in new code in case we want to enable them
> > in the future.
> 
> What should I do instead?

Most tests just do something like `var {TestUtils} = Components.utils.import("resource://testing-common/TestUtils.jsm", {})` but you could also import it into a temporary object.

I think you might also be able to use `Cu.import("resource://...", this)` in ContentTask scripts, but I'm not sure.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b393ec48b4d0
Make browser_dead_object.js correctly wait for the window to be destroyed instead of relying on the scheduling of the corresponding event; r=kmag

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b393ec48b4d0