New issue
Advanced search Search tips

Issue 869932 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Clean up logic that restricts beforeunload dialogs to at most one per navigation

Project Member Reported by alex...@chromium.org, Aug 1

Issue description

This is one of the followups to the beforeunload OOPIF fix in  issue 853021 .  As part of that refactor, some state was introduced to keep track of whether a dialog has been shown on the browser side (see  has_shown_beforeunload_dialog_ in RenderFrameHostImpl::OnRunBeforeUnloadConfirm [1]).  This state is kept on a RFH which is waiting for beforeunload responses, which we find by walking up the ancestor chain and looking for a frame with is_waiting_for_beforeunload_ack_ set to true (since either the frame itself or one of its descendants could be showing the dialog).

However, in a renderer-initiated navigation, the navigating frame, as well as its same-site descendants, run beforeunload handlers before it notifying the browser about the navigation.  This means that if one of those beforeunload handlers shows a dialog, OnRunBeforeUnloadConfirm won't know which frame is running beforeunload (because that's only set later as part of OnBeginNavigation), and won't be able to find and utilize has_shown_beforeunload_dialog_.  This means that if an OOPIF subsequently runs its own beforeunload handler (kicked off from OnBeginNavigation), and that handler attempts to show a dialog, has_shown_beforeunload_dialog_ won't stop it, though it will stop any subsequent attempts.  Essentially, this means that in the worst case, a navigation can now show at most two dialogs.

We could fix this by plumbing the navigating frame into OnRunBeforeUnloadConfirm(), so we know where to set has_shown_beforeunload_dialog_ to true.  Or alternatively, we could add a separate IPC which is sent to the browser when we're starting to run beforeunload as part of a renderer-initiated navigation, which would set is_waiting_for_beforeunload_ack_ on the right frame.

Also, it doesn't make much sense to have this enforcement in both browser (in OnRunBeforeUnloadConfirm) and renderer processes (in Document::DispatchBeforeUnloadEvent [2]).  Ideally, we should just move it to the browser side, though there's some work in the renderer that I'm not sure we can move, such as Intervention::GenerateReport() for the "BeforeUnloadMultiple" intervention.

Some more notes here: https://docs.google.com/document/d/1XmwjGNUl_PF9OgDTs0JsRe0iuGrtEk6mzDYGkkbqHKg/edit?usp=sharing

[1] https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_host_impl.cc?l=2313&rcl=7eb2180cf9d43c64984e0f190ab17e1cc5a17bd6

[2] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/dom/document.cc?l=3521&rcl=b822dab24251df3891739e3cedb37dab519ebff7
 

Sign in to add a comment