Protect pages with pending state from proactive discarding |
|||
Issue description(Chris, could you triage and route this bug?) Just realized (after chat with fdoray) that we somehow missed protecting pages that have pending state i.e. beforeunload handler returns non-empty string. My original suggestion was to run an experiment where we run the handler pre-emptively before freezing and note if it has pending state: - if yes: then it is NOT eligible for discarding - if no: then it is eligible for discarding Periodic unfreezing makes this more complicated (as we'll keep running the handler before each freeze), though maybe still worth trying. Alternatively, we'd have to run beforeunload at the time of discarding to check for pending state, but that is not desirable. What's the bug component for Catan issues?
,
Aug 28
,
Aug 29
Looking into this. It looks like we'll have to hook into the browser side logic
for firing the before unload handler and waiting for the response, but
preventing it from displaying a dialog. There's currently a fair bit of logic
involved in running the handler, showing the message and then preventing the
close from occurring:
[In the browser]
1. WebContents::DispatchBeforeUnload
2. WebContents::DispatchBeforeUnload
3. RenderFrameHostImpl::DispatchBeforeUnload(with an unload reason)
- This is the workhorse function, with the logic for deciding whether or
not the frame tree is navigated, etc, deciding whether or not a call
has timed out, canceling ongoing navigations if appropriate,
handling multiple calls, different reasons, etc.
4. RenderFrameHostImpl::CheckOrDispatchBeforeUnloadForSubtree
- Sends a FrameMsg_BeforeUnload to the renderer
[In the renderer]
5. RenderFrameImpl::OnBeforeUnload
6. WebLocalFrame::DispatchBeforeUnloadEvent
7. WebLocalFrameImpl::DispatchBeforeUnloadEvent
8. FrameLoader::ShouldClose
9. Document::DispatchBeforeUnloadEvent
10. ChromeClient::OpenBeforeUnloadConfirmPanel
11. ChromeClient::OpenBeforeUnloadConfirmPanelDelegate
12. ChromeClientImpl::OpenBeforeUnloadConfirmPanelDelegate
13. WebLocalFrameClient::RunModalBeforeUnloadDialog
14. RenderFrameImpl::RunModalBeforeUnloadDialog
- Sends the actual FrameHostMsg_RunBeforeUnloadConfirm message.
- This is a DELAY_REPLY message, so we synchronously block waiting for
a response from the browser.
[Back in the browser]
16. RenderFrameHostImpl::OnRunBeforeUnloadConfirm
- Logic around displaying the dialog. This appears to only be called if
there actually is a dialog to show. Updates
|has_shown_beforeunload_dialog_| state. This is invoked via a
FrameHostMsg_RunBeforeUnloadConfirm message from the renderer.
17. RenderFrameHostDelegate::RunBeforeUnloadConfirm
18. WebContentsImpl::RunBeforeUnloadConfirm
19. .... a bunch of JavaScriptDialogManager stuff...
20. WebContentsImpl::OnDialogClosed
21. - Calls FrameTreeNode::BeforeUnloadCanceled
- deals with resetting ongoing navigation state if necessary
- notifies WebContentsObserver::BeforeUnloadDialogCancelled
- AUDIT THESE!
22. RenderFrameHostImpl::JavaScriptDialogClosed
23. RenderFrameHostImpl::SendJavaScriptDialogReply
- Sends a FrameHostMsg_RunJavaScriptDialog message reply.
[Back in the renderer]
(Synchronously returns control to RunModuleBeforeUnloadDialog where we
had been blocked since step 14.)
(The return message is actually a FrameHostMsg_RunJavaScriptDialog reply, which
is the same type as expected by the FrameHostMsg_RunBeforeUnloadConfirm reply.)
24. RenderFrameImpl::RunModalBeforeUnloadDialog
- At this point the stack unwinds all the way back to step 5.
25. RenderFrameImpl::OnBeforeUnload
- Sends a FrameHostMsg_BeforeUnload_ACK. The ACK contains a |proceed = true|
argument which is the result of the whole unload handler rigamarole,
including the potential sychronous round trip to the browser to display the
dialog and get the result.
[Back in the browser]
26. RenderFrameHostImpl::OnBeforeUnloadACK
27. RenderFrameHostImpl::ProcessBeforeUnloadACK
28. RenderFrameHostImpl::ProcessBeforeUnloadACKFromFrame
- Splits into a couple notification paths from here.
[Back in the browser, path 1]
29. RenderFrameHostDelegate::DidCancelLoading
31. WebContentsImpl::DidCancelLoading
[Back in the browser, path 2]
32. RenderFrameHostManager::BeforeUnloadFiredFromRenderManager
33. WebContentsImpl::BeforeUnloadFiredFromRenderManager
- Notifies WebContentsObserver::BeforeUnloadFired, but |proceed| is not
included in that call.
34. Notifies WebContentsDelegate::BeforeUnloadFired
35. Browser::BeforeUnloadFired
BeforeUnloadFiredFromRenderManager
My initial guess is that we want the entire round-trip, so that the renderer
code doesn't need to be special cased for browser behaviour. The ACK that is
emitted in step 25 contains the information we care about, but we need to
ensure the entire round-trip *doesn't* cause an actual dialog to be displayed.
From the renderer's point of view it will be like it was displayed but the
user clicked "stay on page".
I propose the following
- Modify WebContents::DispatchBeforeUnload to accept a |dont_display_dialog|,
or something of the like. If a dialog is requested it will simply immediately
pretend the used clicked "stay on page" and cancel the unload.
- This delegates to RenderFrameHostImpl::DispatchBeforeUnload, which would be
augmented with a new unload reason. Under this before unload reason the logic
requesting the dialog be displayed would be modified.
- Augment WebContentsObserver::BeforeUnloadFired to include the value of
|proceed|.
A client in the browser that wants to know whether there's pending state would
then:
- Register a WCO.
- Initiate a WebContents::DispatchBeforeUnload(dont_display_dialog=true)
- Observe the call to WCO::BeforeUnloadFired to see if |proceed| is true or not.
I'm going to experiment with this to see if there are other issues at play, or any unintended side-effects.
,
Aug 29
,
Aug 29
Hmm... it looks like there's already something like this in UnloadController::TryToCloseWindow. Maybe that's a better entry point...
,
Sep 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e200b3b1e457e2a674992fbeeb2bf5541095e515 commit e200b3b1e457e2a674992fbeeb2bf5541095e515 Author: Chris Hamilton <chrisha@chromium.org> Date: Tue Sep 11 17:10:40 2018 Add another BeforeUnloadType in support of discarding. In order to not discard sites with potential unsaved user state it is necessary to run beforeunload handlers to see if they return the empty string or not. When running handlers in this context it is undesirable to launch a modal dialog, but rather to silently block the discard and continue running the page. This CL adds a new BeforeUnloadType to RenderFrameHostImpl in support of this. A follow-up change will expose functionality for invoking DispatchBeforeUnload with this BeforeUnloadType, and returning the results of the unload via a callback. BUG=877550 Change-Id: Iad029fb0f70f9f44695f15c5ca0c0f6e4ed7ff78 Reviewed-on: https://chromium-review.googlesource.com/1208962 Commit-Queue: Chris Hamilton <chrisha@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#590362} [modify] https://crrev.com/e200b3b1e457e2a674992fbeeb2bf5541095e515/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/e200b3b1e457e2a674992fbeeb2bf5541095e515/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/e200b3b1e457e2a674992fbeeb2bf5541095e515/content/browser/frame_host/render_frame_host_impl_browsertest.cc [modify] https://crrev.com/e200b3b1e457e2a674992fbeeb2bf5541095e515/content/browser/web_contents/web_contents_impl.h
,
Sep 11
Note that we actually want 2 code paths here: (1) We need to be able to query the state of the beforeUnload handler from the browser at any moment. This is being done by adding appropriate plumbing to WebContents::DispatchBeforeUnload and WebContentsObserver::BeforeUnloadFired. (2) We need to know the beforeUnload state of a tab that is already frozen. This will be done directly in the renderer by running the beforeUnload handler as part of the "freezing" lifecycle state transition, and communicating the state back to the browser as part of the lifecycle transition ack.
,
Sep 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/916b109ded853d592d1585297ba16a46ce1ac882 commit 916b109ded853d592d1585297ba16a46ce1ac882 Author: Hitoshi Yoshida <peria@chromium.org> Date: Wed Sep 12 02:14:10 2018 Revert "Add another BeforeUnloadType in support of discarding." This reverts commit e200b3b1e457e2a674992fbeeb2bf5541095e515. Reason for revert: This CL seems to break tests. See issue 883160 for details Original change's description: > Add another BeforeUnloadType in support of discarding. > > In order to not discard sites with potential unsaved user state it is necessary > to run beforeunload handlers to see if they return the empty string or not. When running > handlers in this context it is undesirable to launch a modal dialog, but rather to > silently block the discard and continue running the page. > > This CL adds a new BeforeUnloadType to RenderFrameHostImpl in support of this. A > follow-up change will expose functionality for invoking DispatchBeforeUnload with > this BeforeUnloadType, and returning the results of the unload via a callback. > > BUG=877550 > > Change-Id: Iad029fb0f70f9f44695f15c5ca0c0f6e4ed7ff78 > Reviewed-on: https://chromium-review.googlesource.com/1208962 > Commit-Queue: Chris Hamilton <chrisha@chromium.org> > Reviewed-by: Alex Moshchuk <alexmos@chromium.org> > Cr-Commit-Position: refs/heads/master@{#590362} TBR=chrisha@chromium.org,alexmos@chromium.org Change-Id: If974ab384c3fdda13ca8e3eb89940d5a4026fa68 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 877550, 883160 Reviewed-on: https://chromium-review.googlesource.com/1220408 Reviewed-by: Hitoshi Yoshida <peria@chromium.org> Commit-Queue: Hitoshi Yoshida <peria@chromium.org> Cr-Commit-Position: refs/heads/master@{#590575} [modify] https://crrev.com/916b109ded853d592d1585297ba16a46ce1ac882/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/916b109ded853d592d1585297ba16a46ce1ac882/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/916b109ded853d592d1585297ba16a46ce1ac882/content/browser/frame_host/render_frame_host_impl_browsertest.cc [modify] https://crrev.com/916b109ded853d592d1585297ba16a46ce1ac882/content/browser/web_contents/web_contents_impl.h
,
Sep 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8199770a5531d69fa3b465c386f0682945c8e7e5 commit 8199770a5531d69fa3b465c386f0682945c8e7e5 Author: Chris Hamilton <chrisha@chromium.org> Date: Wed Sep 12 14:18:06 2018 Reland "Add another BeforeUnloadType in support of discarding." This is a reland of e200b3b1e457e2a674992fbeeb2bf5541095e515 Original change's description: > Add another BeforeUnloadType in support of discarding. > > In order to not discard sites with potential unsaved user state it is necessary > to run beforeunload handlers to see if they return the empty string or not. When running > handlers in this context it is undesirable to launch a modal dialog, but rather to > silently block the discard and continue running the page. > > This CL adds a new BeforeUnloadType to RenderFrameHostImpl in support of this. A > follow-up change will expose functionality for invoking DispatchBeforeUnload with > this BeforeUnloadType, and returning the results of the unload via a callback. > > BUG=877550 > > Change-Id: Iad029fb0f70f9f44695f15c5ca0c0f6e4ed7ff78 > Reviewed-on: https://chromium-review.googlesource.com/1208962 > Commit-Queue: Chris Hamilton <chrisha@chromium.org> > Reviewed-by: Alex Moshchuk <alexmos@chromium.org> > Cr-Commit-Position: refs/heads/master@{#590362} TBR=alexmos@chromium.org Bug: 877550 Change-Id: I7484671a0fe003f520204f835357a2b62ef63dcc Reviewed-on: https://chromium-review.googlesource.com/1221471 Reviewed-by: Chris Hamilton <chrisha@chromium.org> Commit-Queue: Chris Hamilton <chrisha@chromium.org> Cr-Commit-Position: refs/heads/master@{#590666} [modify] https://crrev.com/8199770a5531d69fa3b465c386f0682945c8e7e5/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/8199770a5531d69fa3b465c386f0682945c8e7e5/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/8199770a5531d69fa3b465c386f0682945c8e7e5/content/browser/frame_host/render_frame_host_impl_browsertest.cc [modify] https://crrev.com/8199770a5531d69fa3b465c386f0682945c8e7e5/content/browser/web_contents/web_contents_impl.h
,
Sep 18
Note that there's some fall-out here when a devtools window is attached: these windows intercept beforeunload requests, and the |auto_cancel| parameters gets lost in the shuffle. Thus, a browser initiated call to DispatchBeforeUnload(auto_cancel=true) will result in a dialog appearing. This is generally disconcerting to the user. Will address in a follow-up CL.
,
Sep 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3b3e315db5ef3b1fa846dcdfd93c49948bae6e2e commit 3b3e315db5ef3b1fa846dcdfd93c49948bae6e2e Author: Chris Hamilton <chrisha@chromium.org> Date: Wed Sep 19 13:16:21 2018 Add |auto_cancel| parameter to WebContents::DispatchBeforeUnload. This exposes a parameter on WebContents::DispatchBeforeUnload which causes the beforeunload dialog to automatically cancel itself if the beforeunload handler returns a non-empty string. This also adds plumbing to expose the value of |proceed| to the existing WebContentsObserver::BeforeUnloadFired. A follow-up CL will use these changes in order to create a "beforeunload" helper to determine whether or not a page contains unsaved user data, and is therefore unsafe for proactive discarding. BUG=877550 Change-Id: I3d7b09709985656dc48bbbf408f8b2bdf87b186c Reviewed-on: https://chromium-review.googlesource.com/1220307 Commit-Queue: Chris Hamilton <chrisha@chromium.org> Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Egor Pasko <pasko@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#592378} [modify] https://crrev.com/3b3e315db5ef3b1fa846dcdfd93c49948bae6e2e/chrome/browser/devtools/devtools_sanity_browsertest.cc [modify] https://crrev.com/3b3e315db5ef3b1fa846dcdfd93c49948bae6e2e/chrome/browser/devtools/devtools_window.cc [modify] https://crrev.com/3b3e315db5ef3b1fa846dcdfd93c49948bae6e2e/chrome/browser/prerender/prerender_manager.cc [modify] https://crrev.com/3b3e315db5ef3b1fa846dcdfd93c49948bae6e2e/chrome/browser/ui/fast_unload_controller.cc [modify] https://crrev.com/3b3e315db5ef3b1fa846dcdfd93c49948bae6e2e/chrome/browser/ui/tab_contents/core_tab_helper.cc [modify] https://crrev.com/3b3e315db5ef3b1fa846dcdfd93c49948bae6e2e/chrome/browser/ui/tab_contents/core_tab_helper.h [modify] https://crrev.com/3b3e315db5ef3b1fa846dcdfd93c49948bae6e2e/chrome/browser/ui/unload_controller.cc [modify] https://crrev.com/3b3e315db5ef3b1fa846dcdfd93c49948bae6e2e/chromecast/browser/cast_web_view_default.cc [modify] https://crrev.com/3b3e315db5ef3b1fa846dcdfd93c49948bae6e2e/content/browser/browser_side_navigation_browsertest.cc [modify] https://crrev.com/3b3e315db5ef3b1fa846dcdfd93c49948bae6e2e/content/browser/devtools/protocol/page_handler.cc [modify] https://crrev.com/3b3e315db5ef3b1fa846dcdfd93c49948bae6e2e/content/browser/web_contents/web_contents_impl.cc [modify] https://crrev.com/3b3e315db5ef3b1fa846dcdfd93c49948bae6e2e/content/browser/web_contents/web_contents_impl.h [modify] https://crrev.com/3b3e315db5ef3b1fa846dcdfd93c49948bae6e2e/content/public/browser/web_contents.h [modify] https://crrev.com/3b3e315db5ef3b1fa846dcdfd93c49948bae6e2e/content/public/browser/web_contents_observer.h [modify] https://crrev.com/3b3e315db5ef3b1fa846dcdfd93c49948bae6e2e/ui/views/controls/webview/web_dialog_view.cc
,
Sep 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3b92e5f0570399d4dbcacf74da8556edb24b7a82 commit 3b92e5f0570399d4dbcacf74da8556edb24b7a82 Author: Chris Hamilton <chrisha@chromium.org> Date: Tue Sep 25 16:51:29 2018 [RC] Create DiscardBeforeUnloadHelper. This will be used to determine if a tab is unsafe to discard due to a beforeunload handler that returns a non-empty string. BUG=877550 Change-Id: Ib0f9c11fbf74ca6a49c23c6b5866dde5faa2885f Reviewed-on: https://chromium-review.googlesource.com/1231739 Commit-Queue: Chris Hamilton <chrisha@chromium.org> Reviewed-by: François Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#593969} [modify] https://crrev.com/3b92e5f0570399d4dbcacf74da8556edb24b7a82/chrome/browser/BUILD.gn [add] https://crrev.com/3b92e5f0570399d4dbcacf74da8556edb24b7a82/chrome/browser/resource_coordinator/discard_before_unload_helper.cc [add] https://crrev.com/3b92e5f0570399d4dbcacf74da8556edb24b7a82/chrome/browser/resource_coordinator/discard_before_unload_helper.h [add] https://crrev.com/3b92e5f0570399d4dbcacf74da8556edb24b7a82/chrome/browser/resource_coordinator/discard_before_unload_helper_browsertest.cc [modify] https://crrev.com/3b92e5f0570399d4dbcacf74da8556edb24b7a82/chrome/test/BUILD.gn [add] https://crrev.com/3b92e5f0570399d4dbcacf74da8556edb24b7a82/chrome/test/data/emptybeforeunload.html
,
Oct 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/da35714929fe57157891b28031e8f8d272081a70 commit da35714929fe57157891b28031e8f8d272081a70 Author: Chris Hamilton <chrisha@chromium.org> Date: Tue Oct 09 16:41:54 2018 [RC] Track beforeunload handler state in PageCUs as FrameCUs are frozen. This CL allows the RC to determine whether or not a page is safe for discarding once it has transitioned to being frozen. This is accomplished by the following: (1) Making it so Document::DispatchBeforeUnloadEvent can be called on a frame without invoking an UnloadConfirmPanel. (2) Modifying LocalFrame::DidFreeze to first invoke DispatchBeforeUnloadEvent and to persist the state of the beforeunload handler to the FrameCU via FrameCoordinationUnit::SetHasBeforeUnload. (3) Modifying PageCUImpl to track how many frames are frozen. (4) When PageCUImpl transitions to all frames being frozen, the beforeunload states from each frame are aggregated to determine the PageCUImpl beforeunload state. A future CL will integrate this state into the discarding logic. BUG=877550 Change-Id: Ib900b099b4fe9f49e8e219d455d9dbba3dedaa6d Reviewed-on: https://chromium-review.googlesource.com/c/1252904 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: François Doray <fdoray@chromium.org> Reviewed-by: Will Harris <wfh@chromium.org> Reviewed-by: Shubhie Panicker <panicker@chromium.org> Commit-Queue: Chris Hamilton <chrisha@chromium.org> Cr-Commit-Position: refs/heads/master@{#597951} [modify] https://crrev.com/da35714929fe57157891b28031e8f8d272081a70/services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.cc [modify] https://crrev.com/da35714929fe57157891b28031e8f8d272081a70/services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h [modify] https://crrev.com/da35714929fe57157891b28031e8f8d272081a70/services/resource_coordinator/coordination_unit/frame_coordination_unit_impl_unittest.cc [modify] https://crrev.com/da35714929fe57157891b28031e8f8d272081a70/services/resource_coordinator/coordination_unit/page_coordination_unit_impl.cc [modify] https://crrev.com/da35714929fe57157891b28031e8f8d272081a70/services/resource_coordinator/coordination_unit/page_coordination_unit_impl.h [modify] https://crrev.com/da35714929fe57157891b28031e8f8d272081a70/services/resource_coordinator/coordination_unit/page_coordination_unit_impl_unittest.cc [modify] https://crrev.com/da35714929fe57157891b28031e8f8d272081a70/services/resource_coordinator/public/mojom/coordination_unit.mojom [modify] https://crrev.com/da35714929fe57157891b28031e8f8d272081a70/third_party/blink/renderer/core/dom/document.cc [modify] https://crrev.com/da35714929fe57157891b28031e8f8d272081a70/third_party/blink/renderer/core/dom/document.h [modify] https://crrev.com/da35714929fe57157891b28031e8f8d272081a70/third_party/blink/renderer/core/frame/local_frame.cc [modify] https://crrev.com/da35714929fe57157891b28031e8f8d272081a70/third_party/blink/renderer/core/loader/frame_loader.cc [modify] https://crrev.com/da35714929fe57157891b28031e8f8d272081a70/third_party/blink/renderer/platform/instrumentation/resource_coordinator/frame_resource_coordinator.cc [modify] https://crrev.com/da35714929fe57157891b28031e8f8d272081a70/third_party/blink/renderer/platform/instrumentation/resource_coordinator/frame_resource_coordinator.h [modify] https://crrev.com/da35714929fe57157891b28031e8f8d272081a70/tools/metrics/histograms/enums.xml |
|||
►
Sign in to add a comment |
|||
Comment 1 by panicker@chromium.org
, Aug 28