Closed Bug 1370100 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file)

No description provided.
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!
Attachment #8874245 - Flags: review?(kmaglione+bmo)
Assignee: nobody → ehsan
Blocks: 1361461
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.
Attachment #8874245 - Flags: review?(kmaglione+bmo) → review+
(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?
(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.
Pushed by eakhgari@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/b393ec48b4d0
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.