Closed Bug 1100700 Opened 5 years ago Closed 4 years ago

e10s: unload event doesn't fire on browser when tab is closed using removeCurrentTab (breaks browser_unloaddialogs.js)

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
e10s + ---
firefox48 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Looking at browser_unloaddialogs.js... I actually think it should be working! All the code is certainly being hit. However, this bit of code:


function onLoad(event)
{
  info("Page loaded");

  event.target.removeEventListener("load", onLoad, true);
  gBrowser.selectedBrowser.addEventListener("unload", done, true);

  executeSoon(function () {
    info("Closing page");
    gBrowser.removeCurrentTab();
  });
}

seems to not have the unload handler be triggered when removeCurrentTab happens.

I think that's a bug. Mike, am I missing something, and/or is this a dupe? (if so, can we ensure we update the comment for browser_unloaddialogs.js in browser/base/content/test/general/browser.ini? Thanks!
Flags: qe-verify-
Flags: needinfo?(mconley)
Flags: in-testsuite+
Flags: firefox-backlog+
Hm, this is new and strange. Indeed a bug. The shims should be allowing us to get the unload event from the remote browser.
Flags: needinfo?(mconley)
This is bug 967873, right?
(In reply to Josh Matthews [:jdm] from comment #2)
> This is bug 967873, right?

Possibly? I don't know for sure, but I'll mark it a dep because it sounds plausible enough.
Depends on: 967873
Summary: e10s: unload event doesn't fire on tab when tab is closed using removeCurrentTab (breaks browser_unloaddialogs.js) → e10s: unload event doesn't fire on browser when tab is closed using removeCurrentTab (breaks browser_unloaddialogs.js)
https://reviewboard.mozilla.org/r/37603/#review34159

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #1)
> Hm, this is new and strange. Indeed a bug. The shims should be allowing us
> to get the unload event from the remote browser.

AFAICT the unload event fires in the content process but the message/forwarded stuff never reaches the parent process. If I listen for the event in the content process and send a message from there it dies in transit.

The patch that I put up fixes the test to run, but if we want to address the unload event issue we might want to file a followup bug.

The other interesting thing is how to make this test fail... Just removing the disableDialogs() call from tabbrowser.xml wasn't enough to accomplish that either before or after these changes, so I kind of wonder to what degree this test is still useful.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #8725728 - Flags: review?(mconley)
https://reviewboard.mozilla.org/r/37601/#review34423

Clearing review because the trypush shows this breaks the next test (browser_urlHighlight.js)
Comment on attachment 8725728 [details]
MozReview Request: Bug 1100700 - fix browser_unloaddialogs.js so it runs in e10s mode, r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37603/diff/1-2/
Attachment #8725728 - Flags: review?(mconley)
No idea why that is only happening on some flavours of Linux, only on e10s. :-\

I took an axe to the test instead, hopefully that helps, or something. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=02e71b3b53f6

If that doesn't work I expect we might need to change browser_urlHighlight.js, but it'd be quite useful if I had even a small shimmer of a clue about what was actually broken about it...
(In reply to :Gijs Kruitbosch from comment #8)
> No idea why that is only happening on some flavours of Linux, only on e10s.
> :-\
> 
> I took an axe to the test instead, hopefully that helps, or something. New
> try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=02e71b3b53f6

Looks like it did! \o/
Attachment #8725728 - Flags: review?(mconley) → review+
Comment on attachment 8725728 [details]
MozReview Request: Bug 1100700 - fix browser_unloaddialogs.js so it runs in e10s mode, r?mconley

https://reviewboard.mozilla.org/r/37603/#review35041

Looks good!

::: browser/base/content/test/general/browser_unloaddialogs.js:36
(Diff revision 2)
> +    // beat:

Stage-direction-wise, "beat" speaks to me, but for other code spelunkers, I imagine it's a bit opaque. Can you quickly write in here why we're waiting for the next spin of the event loop?
https://hg.mozilla.org/mozilla-central/rev/815bc777e53b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.