Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2386983002:
PlzNavigate: cancel any navigations if DispatchBeforeUnload is called (other than for starting a ... (Closed)
DescriptionPlzNavigate: cancel any navigations if DispatchBeforeUnload is called (other than for starting a navigation obviously).
Otherwise, if a render view is closed while a navigation is in progress, we will ignore the beforeunload ack that eventually comes from the renderer and the render view will never close.
This fixes
ChromeRenderProcessHostTest.DevToolsOnSelfInOwnProcess
ChromeRenderProcessHostTest.DevToolsOnSelfInOwnProcessPPT
with PlzNavigate.
BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/03e86d027151639ad79c2074199f4e75beab0674
Cr-Commit-Position: refs/heads/master@{#423378}
Patch Set 1 #Patch Set 2 #Patch Set 3 : guard against NavigatorImpl::RequestNavigation resetting the navigation, and add reduced test case #
Total comments: 1
Messages
Total messages: 28 (21 generated)
Description was changed from ========== Don't reset the navigation state when a navigate succeeds or fails with PlzNavigate. Otherwise, if a render view is closed while a commit is pending, we will ignore the beforeunload ack that eventually comes from the renderer. This fixes ContinueWhereILeftOffTest.PostWithPassword ContinueWhereILeftOffTest.PostWithPasswordBrowserClose with PlzNavigate. BUG=504347 ========== to ========== Don't reset the navigation state when a navigate succeeds or fails with PlzNavigate. Otherwise, if a render view is closed while a commit is pending, we will ignore the beforeunload ack that eventually comes from the renderer. This fixes ContinueWhereILeftOffTest.PostWithPassword ContinueWhereILeftOffTest.PostWithPasswordBrowserClose with PlzNavigate. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Don't reset the navigation state when a navigate succeeds or fails with PlzNavigate. Otherwise, if a render view is closed while a commit is pending, we will ignore the beforeunload ack that eventually comes from the renderer. This fixes ContinueWhereILeftOffTest.PostWithPassword ContinueWhereILeftOffTest.PostWithPasswordBrowserClose with PlzNavigate. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Don't reset the waiting state when a navigate succeeds or fails with PlzNavigate. Otherwise, if a render view is closed while a navigation is in progress, we will ignore the beforeunload ack that eventually comes from the renderer and the render view will never close. This fixes ContinueWhereILeftOffTest.PostWithPassword ContinueWhereILeftOffTest.PostWithPasswordBrowserClose with PlzNavigate. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
jam@chromium.org changed reviewers: + clamy@chromium.org
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Don't reset the waiting state when a navigate succeeds or fails with PlzNavigate. Otherwise, if a render view is closed while a navigation is in progress, we will ignore the beforeunload ack that eventually comes from the renderer and the render view will never close. This fixes ContinueWhereILeftOffTest.PostWithPassword ContinueWhereILeftOffTest.PostWithPasswordBrowserClose with PlzNavigate. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Don't reset the waiting state when a navigate succeeds or fails with PlzNavigate. Otherwise, if a render view is closed while a navigation is in progress, we will ignore the beforeunload ack that eventually comes from the renderer and the render view will never close. This fixes ChromeRenderProcessHostTest.DevToolsOnSelfInOwnProcess ChromeRenderProcessHostTest.DevToolsOnSelfInOwnProcessPPT with PlzNavigate. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Don't reset the waiting state when a navigate succeeds or fails with PlzNavigate. Otherwise, if a render view is closed while a navigation is in progress, we will ignore the beforeunload ack that eventually comes from the renderer and the render view will never close. This fixes ChromeRenderProcessHostTest.DevToolsOnSelfInOwnProcess ChromeRenderProcessHostTest.DevToolsOnSelfInOwnProcessPPT with PlzNavigate. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: cancel any navigations if DispatchBeforeUnload is called (other than for starting a navigation obviously). Otherwise, if a render view is closed while a navigation is in progress, we will ignore the beforeunload ack that eventually comes from the renderer and the render view will never close. This fixes ChromeRenderProcessHostTest.DevToolsOnSelfInOwnProcess ChromeRenderProcessHostTest.DevToolsOnSelfInOwnProcessPPT with PlzNavigate. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
jam@chromium.org changed reviewers: + nasko@chromium.org - clamy@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2386983002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2386983002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2338: frame_tree_node_->ResetNavigationRequest(false); If the beforeunload handler stops the tab from closing though, that means that the tab will stay around and the navigation is cancelled. Seems strange behavior, but I just tested it and it looks like this is how it works now :(.
On 2016/10/05 21:25:17, nasko (slow) wrote: > LGTM > > https://codereview.chromium.org/2386983002/diff/40001/content/browser/frame_h... > File content/browser/frame_host/render_frame_host_impl.cc (right): > > https://codereview.chromium.org/2386983002/diff/40001/content/browser/frame_h... > content/browser/frame_host/render_frame_host_impl.cc:2338: > frame_tree_node_->ResetNavigationRequest(false); > If the beforeunload handler stops the tab from closing though, that means that > the tab will stay around and the navigation is cancelled. Seems strange > behavior, but I just tested it and it looks like this is how it works now :(. Actually, I made a mistake. The navigation is still ongoing in non-PlzNavigate mode.
The CQ bit was checked by jam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/05 21:27:07, nasko (slow) wrote: > On 2016/10/05 21:25:17, nasko (slow) wrote: > > LGTM > > > > > https://codereview.chromium.org/2386983002/diff/40001/content/browser/frame_h... > > File content/browser/frame_host/render_frame_host_impl.cc (right): > > > > > https://codereview.chromium.org/2386983002/diff/40001/content/browser/frame_h... > > content/browser/frame_host/render_frame_host_impl.cc:2338: > > frame_tree_node_->ResetNavigationRequest(false); > > If the beforeunload handler stops the tab from closing though, that means that > > the tab will stay around and the navigation is cancelled. Seems strange > > behavior, but I just tested it and it looks like this is how it works now :(. > > Actually, I made a mistake. The navigation is still ongoing in non-PlzNavigate > mode. Let me clarify, since my last response was not very useful :(. The current behavior is a bit strange, as a slow navigation will not be cancelled by the closing of the tab. If the user clicks "Stay" in the beforeunload dialog to cancel the tab closure, the slow navigation will eventually commit. However, once the dialog is dismissed, the UI does not indicate that any navigation is happening - no spinner and a reload button is shown. Once the slow navigation commits, we do a very sudden change of the document without any feedback to the user. As such, cancelling the navigation when closing the tab and displaying a beforeunload dialog I think will be overall more consistent user experience. LGTM.
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: cancel any navigations if DispatchBeforeUnload is called (other than for starting a navigation obviously). Otherwise, if a render view is closed while a navigation is in progress, we will ignore the beforeunload ack that eventually comes from the renderer and the render view will never close. This fixes ChromeRenderProcessHostTest.DevToolsOnSelfInOwnProcess ChromeRenderProcessHostTest.DevToolsOnSelfInOwnProcessPPT with PlzNavigate. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: cancel any navigations if DispatchBeforeUnload is called (other than for starting a navigation obviously). Otherwise, if a render view is closed while a navigation is in progress, we will ignore the beforeunload ack that eventually comes from the renderer and the render view will never close. This fixes ChromeRenderProcessHostTest.DevToolsOnSelfInOwnProcess ChromeRenderProcessHostTest.DevToolsOnSelfInOwnProcessPPT with PlzNavigate. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: cancel any navigations if DispatchBeforeUnload is called (other than for starting a navigation obviously). Otherwise, if a render view is closed while a navigation is in progress, we will ignore the beforeunload ack that eventually comes from the renderer and the render view will never close. This fixes ChromeRenderProcessHostTest.DevToolsOnSelfInOwnProcess ChromeRenderProcessHostTest.DevToolsOnSelfInOwnProcessPPT with PlzNavigate. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: cancel any navigations if DispatchBeforeUnload is called (other than for starting a navigation obviously). Otherwise, if a render view is closed while a navigation is in progress, we will ignore the beforeunload ack that eventually comes from the renderer and the render view will never close. This fixes ChromeRenderProcessHostTest.DevToolsOnSelfInOwnProcess ChromeRenderProcessHostTest.DevToolsOnSelfInOwnProcessPPT with PlzNavigate. BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/03e86d027151639ad79c2074199f4e75beab0674 Cr-Commit-Position: refs/heads/master@{#423378} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/03e86d027151639ad79c2074199f4e75beab0674 Cr-Commit-Position: refs/heads/master@{#423378} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
