1655866 - beforeunload event is not fired in OOP iframes when reloading documents

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: A test case

Since bug 1533949, beforeunload event is properly handled in OOP iframes when closing documents, but it's not working when document is reloaded.

STR;

1. Open the attaching file
2. Right click on the iframe there to make the document's mUserHasInteracted flag dirty
3. Ctrl+r to reload the document

Comment 1

[:Gijs (he/him)]

Do you mean the event doesn't fire or you mean the dialog doesn't show, or both?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

The event doesn't fire. (thus the dialog doesn't show).

Comment 3

[Chris Peterson [:cpeterson]]

M6b because users might lose data if beforeunload is not firing.

needinfo'ing Nika to provide more details

Comment 4

[Nika Layzell [:nika] (ni? for response)]

This bug doesn't occur when closing windows as it was fixed in the frontend only for beforeunload calls when the tab is being closed, but the calls to beforeunload when navigating a document were never updated. The call to permitUnload in the tab-close case are performed async within BrowserElementChild here: https://searchfox.org/mozilla-central/source/toolkit/actors/BrowserElementChild.jsm#25-35. This call is performed on each process-contiguous root here: https://searchfox.org/mozilla-central/source/toolkit/actors/BrowserElementParent.jsm#190,210-214

Unfortunately the calls to PermitUnload for other navigations are a bit more spread-out. The primary call-site is in InternalLoad, which is used for most types of navigations (https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/docshell/base/nsDocShell.cpp#8781-8793), but there are exceptions where PermitUnload needs to be called differently, such as:

When creating an about:blank document in some cases (I don't know how often this one actually comes up): https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/docshell/base/nsDocShell.cpp#6380
When loading a javascript: URI which decides to actually produce a response: https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/dom/jsurl/nsJSProtocolHandler.cpp#749
When calling window.close(): https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/dom/base/nsGlobalWindowOuter.cpp#6016

All of these calls inherently use a synchronous API which expects to be able to determine if the unload was blocked in a sync action. This is quite unfortunate, as it makes the process of switching to be Fission-compatible much more complicated.

"fortunately" for us (note the scare quotes), calls to permitUnload already spin a nested event loop in some cases due to both calling JS which does so (https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/layout/base/nsDocumentViewer.cpp#1252-1255), and because of the modal dialog which is presented to the user (https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/layout/base/nsDocumentViewer.cpp#1286-1292), meaning that the calling code should already be fairly resilient to nested event loop issues.

This may allow us to work around the beforeunload issue by internally spinning a nested event loop, and waiting for replies from other processes, if any contexts in the subtree are both: a) out-of-process, and b) have a beforeunload listener registered. We can know if the beforeunload listener is attached to any specific frame by changing the mHasBeforeUnload bool on WindowGlobalParent into a synced field on WindowContext. Doing this check before doing any IPC should help us avoid severe performance regressions, though we will probably still be hurting navigation performance in some cases.

If another API is also added to WindowContext or WindowGlobalParent which avoids the nested event loop and instead returns a promise, this API could even be used by frontend JS code for tab closing.

There may also be some spec loophole which lets us do the work async for cross-origin subframes, but I don't see one, unfortunately: https://html.spec.whatwg.org/multipage/browsing-the-web.html#prompt-to-unload-a-document

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1655866: Part 1 - Add async version of beforeUnloadCheck prompt. r=Gijs

Comment 6

[Kris Maglione [:kmag]]

Attachment: Bug 1655866: Part 2 - Move HasBeforeUnload flag to WindowContext. r=nika

Comment 7

[Kris Maglione [:kmag]]

Attachment: Bug 1655866: Part 3 - Refactor PermitUnload to simplify handling OOP frames. r=nika

Comment 8

[Kris Maglione [:kmag]]

Attachment: Bug 1655866: Part 4 - Handle OOP beforeunload listeners in PermitUnload(). r=nika

Comment 9

[Kris Maglione [:kmag]]

Attachment: Bug 1655866: Part 5 - Use native PermitUnload implementation from front-end code. r=nika

Comment 10

[Kris Maglione [:kmag]]

Attachment: Bug 1655866: Part 6 - Remove sync before unload prompt code. r=nika

Comment 11

[Kris Maglione [:kmag]]

Attachment: Bug 1655866: Part 7 - Add test. r=nika

Comment 12

[Kris Maglione [:kmag]]

Attachment: Bug 1655866: Part 8 - Remove expired onbeforeunload histograms. r=nika

Comment 13

[Kris Maglione [:kmag]]

Attachment: Bug 1655866: Part 9 - Use the same PermitUnloadAction enum in WGP and nsIContentViewer. r=nika

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ddbbfff8e0eb
Part 1 - Add async version of beforeUnloadCheck prompt. r=geckoview-reviewers,Gijs,agi
https://hg.mozilla.org/integration/autoland/rev/ec36af35fa0d
Part 2 - Move HasBeforeUnload flag to WindowContext. r=nika
https://hg.mozilla.org/integration/autoland/rev/ec794040ee6e
Part 3 - Refactor PermitUnload to simplify handling OOP frames. r=nika
https://hg.mozilla.org/integration/autoland/rev/55fb069319f1
Part 4 - Handle OOP beforeunload listeners in PermitUnload(). r=nika
https://hg.mozilla.org/integration/autoland/rev/4b76024db21c
Part 5 - Use native PermitUnload implementation from front-end code. r=nika,mconley
https://hg.mozilla.org/integration/autoland/rev/0ce59fc216ad
Part 6 - Remove sync before unload prompt code. r=nika,geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/091a4043d78a
Part 7 - Add test. r=nika

Comment 15

[Kris Maglione [:kmag]]

Attachment: Bug 1655866: Follow-up: Fix rebase botch.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa0bf905d4cb
Follow-up: Fix rebase botch.

Comment 17

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/ddbbfff8e0eb
https://hg.mozilla.org/mozilla-central/rev/ec36af35fa0d
https://hg.mozilla.org/mozilla-central/rev/ec794040ee6e
https://hg.mozilla.org/mozilla-central/rev/55fb069319f1
https://hg.mozilla.org/mozilla-central/rev/4b76024db21c
https://hg.mozilla.org/mozilla-central/rev/0ce59fc216ad
https://hg.mozilla.org/mozilla-central/rev/091a4043d78a
https://hg.mozilla.org/mozilla-central/rev/fa0bf905d4cb

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/01d1aff2c274
Port bug 1655866 part 6 - Remove sync before unload prompt code. rs=bustage-fix DONTBUILD

Comment 19

[Pascal Chevrel:pascalc]

Kris, given the number of regressions in the past week, should we back it out and reland once these issues are fixed?

Comment 20

[Kris Maglione [:kmag]]

(In reply to Pascal Chevrel:pascalc from comment #19)

Kris, given the number of regressions in the past week, should we back it out and reland once these issues are fixed?

No, they should all be fixed by today.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/98ca31c9f8ef
Part 8 - Remove expired onbeforeunload histograms. r=nika

Comment 22

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/98ca31c9f8ef

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/428c9c1eabb8
Part 9 - Use the same PermitUnloadAction enum in WGP and nsIContentViewer. r=nika

Comment 24

[Razvan Maries]

Backed out for perma failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=H7FkAM2wTlGAw8a6-ZJnVA.0&resultStatus=testfailed%2Cbusted%2Cexception&revision=428c9c1eabb8108f043c5c385d4cc3e5c0927372

Logs:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=318208222&repo=autoland&lineNumber=3689
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=318208232&repo=autoland&lineNumber=114931
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=318208610&repo=autoland&lineNumber=109217
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=318209062&repo=autoland&lineNumber=12726
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=318208638&repo=autoland&lineNumber=3698

Backout: https://hg.mozilla.org/integration/autoland/rev/5d4cb53c7b4c1668c97e9ed290d4c506801f4fd3

Comment 25

[Chris Peterson [:cpeterson]]

(In reply to Razvan Maries from comment #24)

Backed out for perma failures.

Reopening this bug since the fixes were backed out for perma failures.

Comment 26

[Kris Maglione [:kmag]]

The fix was not backed out.