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

RESOLVED FIXED in Firefox 48

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 48
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(e10s+, firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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+
tracking-e10s: ? → +
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)

Comment 2

3 years ago
This is bug 967873, right?
(Assignee)

Comment 3

3 years ago
(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

Updated

3 years ago
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)
(Assignee)

Comment 4

2 years ago
Created attachment 8725728 [details]
MozReview Request: Bug 1100700 - fix browser_unloaddialogs.js so it runs in e10s mode, r?mconley

Review commit: https://reviewboard.mozilla.org/r/37603/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37603/
Attachment #8725728 - Flags: review?(mconley)
(Assignee)

Comment 5

2 years ago
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)

Updated

2 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Attachment #8725728 - Flags: review?(mconley)
(Assignee)

Comment 6

2 years ago
https://reviewboard.mozilla.org/r/37601/#review34423

Clearing review because the trypush shows this breaks the next test (browser_urlHighlight.js)
(Assignee)

Comment 7

2 years ago
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)
(Assignee)

Comment 8

2 years ago
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...
(Assignee)

Comment 9

2 years ago
(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?

Comment 11

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/815bc777e53b

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/815bc777e53b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.