Issue metadata
Sign in to add a comment
|
Audit calls of FrameTreeNode::ResetNavigationRequest to see which need to call NavigationHandleImpl::set_net_error_code |
||||||||||||||||||||||||
Issue descriptionBackground: discussion on https://codereview.chromium.org/2768873006 There have been a few instances where we weren't setting the error code when cancelling the request, i.e. the above link and also https://codereview.chromium.org/2742853002. So we should audit the places below to check the behavior without PlzNavigate to make sure we're consistent. NavigationRequest::OnRequestRedirected -FilterURL fails NavigationRequest::OnResponseStarted -ShouldTransferNavigation returns false -this only happens for extensions NavigatorImpl::OnBeginNavigation -sends a stop IPC, which when it comes back should set the abort error code NavigatorImpl::CancelNavigation RenderFrameHostImpl::DispatchBeforeUnload RenderFrameHostManager::CancelPendingIfNecessary -need to investigate what should happen RenderFrameHostImpl::FailedNavigation -should already be setting it today
,
May 15 2017
,
May 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cb4ae15478ddb2f45a6e306a3de4ff53b6b46269 commit cb4ae15478ddb2f45a6e306a3de4ff53b6b46269 Author: jam <jam@chromium.org> Date: Fri May 19 01:35:51 2017 Ensure that all code paths which call FrameTreeNode::ResetNavigationRequest set NavigationHandle::GetNetErrorCode(). The places were 1) NavigationRequest::OnRequestRedirected (redirected URL fails FilterURL check) The non-PlzNavigate code was setting this in ResourceLoader::OnReceivedRedirect. Added NavigationHandleImplBrowserTest.ErrorCodeOnRedirect to test this. 2) NavigationRequest::OnResponseStarted (cross-process commit cancelled by embedder) Neither PlzNavigate or old code path were setting this. Added this check to existing test CrossSiteTransferTest.NoLeakOnCrossSiteCancel. Fixed PlzNavigate in the above method, and non-Navigate in RenderFrameHostManager::OnCrossSiteResponse. 3) NavigatorImpl::CancelNavigation (navigation aborted case, i.e. beforeunload cancels it) This codepath is only hit for PlzNavigate, since non-PlzNavigate doesn't create a NavigationHandle until after beforeunload succeeds. Added test in NavigationHandleImplBrowserTest.ErrorCodeOnAbortedNavigation. 4) RenderFrameHostImpl::DispatchBeforeUnload This is a PlzNavigate-only code path. Added checks to existing test BrowserSideNavigationBrowserTest.UnloadDuringNavigation. 5) RenderFrameHostManager::CancelPendingIfNecessary Fixed in the above method for PlzNavigate, and in RenderFrameHostImpl::DispatchBeforeUnload for old code path. Added checks to existing test SitePerProcessBrowserTest.RenderViewHostKeepsSwappedOutStateIfPendingRFHDies. 6) RenderFrameHostImpl::FailedNavigation is only called by NavigationRequest::OnRequestFailed which already sets the error code. I've audited all code path leading to this to check that they set an error code, and added a DCHECK to confirm. 7) NavigatorImpl::OnBeginNavigation This looks like dead code, I've sent https://codereview.chromium.org/2890613002/ to remove it. BUG= 705119 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2889703002 Cr-Commit-Position: refs/heads/master@{#473017} [modify] https://crrev.com/cb4ae15478ddb2f45a6e306a3de4ff53b6b46269/content/browser/browser_side_navigation_browsertest.cc [modify] https://crrev.com/cb4ae15478ddb2f45a6e306a3de4ff53b6b46269/content/browser/cross_site_transfer_browsertest.cc [modify] https://crrev.com/cb4ae15478ddb2f45a6e306a3de4ff53b6b46269/content/browser/frame_host/navigation_handle_impl_browsertest.cc [modify] https://crrev.com/cb4ae15478ddb2f45a6e306a3de4ff53b6b46269/content/browser/frame_host/navigation_request.cc [modify] https://crrev.com/cb4ae15478ddb2f45a6e306a3de4ff53b6b46269/content/browser/frame_host/navigator_impl.cc [modify] https://crrev.com/cb4ae15478ddb2f45a6e306a3de4ff53b6b46269/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/cb4ae15478ddb2f45a6e306a3de4ff53b6b46269/content/browser/frame_host/render_frame_host_manager.cc [modify] https://crrev.com/cb4ae15478ddb2f45a6e306a3de4ff53b6b46269/content/browser/site_per_process_browsertest.cc [add] https://crrev.com/cb4ae15478ddb2f45a6e306a3de4ff53b6b46269/content/public/test/navigation_handle_observer.cc [add] https://crrev.com/cb4ae15478ddb2f45a6e306a3de4ff53b6b46269/content/public/test/navigation_handle_observer.h [modify] https://crrev.com/cb4ae15478ddb2f45a6e306a3de4ff53b6b46269/content/test/BUILD.gn
,
May 19 2017
,
Nov 7 2017
,
Nov 7 2017
Apologies, applied the wrong component in bulk. |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by clamy@chromium.org
, Apr 20 2017