Closed Bug 1326779 Opened 3 years ago Closed 3 years ago

[e10s] onbeforeunload event handler doesn't work if I navigate to parent process

Categories

(Core :: DOM: Events, defect)

48 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: arni2033, Assigned: jessica)

Details

(Keywords: multiprocess, regression)

Attachments

(2 files, 2 obsolete files)

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1:  (testcase)
1. Open url   data:text/html,<body onbeforeunload="console.log(Date.now());return 1">
2. Click on the page
3. Type in urlbar "about:preferences", press Enter (to work around bug1208208, bug1208222, bug1193291)

AR:  Browser navigates to about:preferences
ER:  Browser should show notification asking whether I want to navigate to another page


STR_2:  (original)
1. Open https://translate.google.com/#en/ru/hello
2. Type "world" instead of "hello" in the input field on the page
3. Click Back button in urlbar to navigate back to https://translate.google.com/#en/ru/hello
4. Type "about:newtab" in urlbar, press Enter
5. Click Back button in urlbar to na

AR:  When Google translator loads, it navigates to https://translate.google.com/#en/ru/world
ER:  Google translator should stay on https://translate.google.com/#en/ru/hello
Component: Untriaged → DOM: Events
Product: Firefox → Core
No longer blocks: 1277113
I saw this on nightly 2017-01-02 but didn't see this on ff47. Sounds a regression in the period.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> I saw this on nightly 2017-01-02 but didn't see this on ff47. Sounds a
> regression in the period.

I can reproduce on Firefox47.0.1 as well as Nightly53.0a1.
I think this had never been worked under e10s.
Keywords: multiprocess
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> I saw this on nightly 2017-01-02 but didn't see this on ff47. Sounds a
> regression in the period.

Additional note: my previous comment applies for STR1. I couldn't reproduce the issue by STR2.
Some findings about this bug...
When navigating to a parent process page (e.g. about:preferences), loading uri and then nsDocumentViewer::PermitUnload() are all done in parent process. nsDocumentViewer::PermitUnload uses EventDispatcher::DispatchDOMEvent() to send the 'beforeunload' event, so there is no chance for the event to pass to content process. That's why JSEventHandler and web content does not get the 'beforeunload' event when navigating to a parent process page.
That beforeunload shouldn't be dispatched to child process.
There is one beforeunload for the page loaded in child process when that page is about to be unloaded and then if the page loaded in the parent process is unloaded, beforeunload is dispatched to that.

PermitUnload should be called in the child process.
I'm hoping the stuff mystor and samael has been working on would fix the process switch.
Attached patch WIP (obsolete) — Splinter Review
Thanks for samael's help! Here is a quick fix.

There two places where we need to fix. One is, when user types in the url bar directly, LoadInOtherProcess [1] is called without calling PermitUnload if process needs to be changed; second is when using back/forward button, in nsDocShell, we do ShouldLoadURI [2] before PermitUnload [3].

Note that there is no prompt when switching from parent process page to content page, I guess this is expected...


[1] http://searchfox.org/mozilla-central/rev/8144fcc62054e278fe5db9666815cc13188c96fa/browser/base/content/browser.js#896
[2] http://searchfox.org/mozilla-central/rev/8144fcc62054e278fe5db9666815cc13188c96fa/docshell/base/nsDocShell.cpp#10482
[3] http://searchfox.org/mozilla-central/rev/8144fcc62054e278fe5db9666815cc13188c96fa/docshell/base/nsDocShell.cpp#10513
About STR_2, I couldn't reproduce on latest nightly so I tried mozregression and I found it's fixed somewhere between mozilla central commits 4e3c16ea..23dc78b7. I'm not sure which bug exactly but it should be unrelated to STR_1.
Comment on attachment 8825331 [details] [diff] [review]
WIP

May I have your feedback on this? Thanks.
Attachment #8825331 - Flags: feedback?(sawang)
Attachment #8825331 - Flags: feedback?(bugs)
Comment on attachment 8825331 [details] [diff] [review]
WIP

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

(In reply to Jessica Jong [:jessica] from comment #6)
> Note that there is no prompt when switching from parent process page to
> content page, I guess this is expected...

I found browser.xml also implements PermitUnload by forwarding to its content viewer, so maybe we simply didn't use the right way to test it yesterday.
Attachment #8825331 - Flags: feedback?(sawang) → feedback+
Comment on attachment 8825331 [details] [diff] [review]
WIP


>@@ -872,16 +872,22 @@ function _loadURIWithFlags(browser, uri,
>       if (params.userContextId) {
>         browser.webNavigation.setOriginAttributesBeforeLoading({ userContextId: params.userContextId });
>       }
> 
>       browser.webNavigation.loadURIWithOptions(uri, flags,
>                                                referrer, referrerPolicy,
>                                                postData, null, null);
>     } else {
>+      // Check if the current browser is allowed to unload.
>+      let {permitUnload, timedOut} = browser.permitUnload();

Hmm, so browser.permitUnload does send
permitUnload message to child
http://searchfox.org/mozilla-central/rev/225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/toolkit/content/widgets/remote-browser.xml#334
ok.


We need some tests to check that beforeunload event is dispatched once, but only once (to ensure we don't end up calling permitUnload several times).

How would we get the prompt working, or, why is it not working? 
In  _loadURIWithFlags case we have some timeout, so I'd expect us to have at least some time there to show the prompt 
(I think the timeout there is wrong, since we should wait for permitUnload to finish or something)
And in nsDocShell case isn't it child process triggering the load, so I'd expect prompt to be shown.
Attachment #8825331 - Flags: feedback?(sawang)
Attachment #8825331 - Flags: feedback?(bugs)
Attachment #8825331 - Flags: feedback+
Comment on attachment 8825331 [details] [diff] [review]
WIP

Sorry, this was +'ed
Attachment #8825331 - Flags: feedback?(sawang) → feedback+
Sorry, comment 4 was a bit misleading. When navigating to a chrome process page, loading uri is done first on content and then change process if needed. It's just we are not doing PermitUnload before changing process. So after this fix, before changing process we fire 'beforeunload' to the current document correctly.

(In reply to Samael Wang [:freesamael][:sawang] from comment #9)
> 
> I found browser.xml also implements PermitUnload by forwarding to its
> content viewer, so maybe we simply didn't use the right way to test it
> yesterday.

You're right. I changed a way to test it and it does work!

(In reply to Olli Pettay [:smaug] from comment #10)
> 
> Hmm, so browser.permitUnload does send
> permitUnload message to child
> http://searchfox.org/mozilla-central/rev/
> 225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/toolkit/content/widgets/remote-
> browser.xml#334
> ok.
> 
> 
> We need some tests to check that beforeunload event is dispatched once, but
> only once (to ensure we don't end up calling permitUnload several times).

Sure, I'll think of some tests.

> 
> How would we get the prompt working, or, why is it not working? 

Actually, the prompt is working is both cases now (parent -> child, child -> parent).

> In  _loadURIWithFlags case we have some timeout, so I'd expect us to have at
> least some time there to show the prompt 
> (I think the timeout there is wrong, since we should wait for permitUnload
> to finish or something)

The timeout is for when we do not receive the 'start' message from parent, so in normal cases we do wait for permitUnload to finish.

> And in nsDocShell case isn't it child process triggering the load, so I'd
> expect prompt to be shown.

Yes, it is shown. :)
Assignee: nobody → jjong
Attached patch patch, v1. (obsolete) — Splinter Review
Attachment #8825331 - Attachment is obsolete: true
Attachment #8827051 - Flags: review?(bugs)
Comment on attachment 8827051 [details] [diff] [review]
patch, v1.

>+ * Test navigation from a chrome page to a conte page. Also check that only one
nit, s/conte/content/

>+  let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser,
>+  	                                                    "about:support");
nit, align params
Attachment #8827051 - Flags: review?(bugs) → review+
Attached patch patch, v2.Splinter Review
addressed nits in comment 14.
Attachment #8827051 - Attachment is obsolete: true
Attachment #8827327 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0ca66f6ac46
[e10s] Fire beforeunload event when navigating to a page in different process. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e0ca66f6ac46
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(jjong)
Comment on attachment 8828702 [details] [diff] [review]
patch for aurora.

Approval Request Comment
[Feature/Bug causing the regression]: e10s
[User impact if declined]: beforeunload event does not work when navigating from a chrome page to a content page and viceversa
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes, manually
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal
[Why is the change risky/not risky?]: just calls an extra function (permitUnload) when changing processes
[String changes made/needed]: None
Flags: needinfo?(jjong)
Attachment #8828702 - Flags: approval-mozilla-aurora?
Comment on attachment 8828702 [details] [diff] [review]
patch for aurora.

e10s event handling fix, aurora52+
Attachment #8828702 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.