Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(14)

Issue 2889703002: Ensure that all code paths which call FrameTreeNode::ResetNavigationRequest set NavigationHandle::G… (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 1 week ago by jam
Modified:
1 month ago
Reviewers:
nasko (slow)
CC:
chromium-reviews, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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} Committed: https://chromium.googlesource.com/chromium/src/+/cb4ae15478ddb2f45a6e306a3de4ff53b6b46269

Patch Set 1 #

Patch Set 2 : remove NavigatorImpl::OnBeginNavigation since I'll remove that code in a separate cl #

Total comments: 9

Patch Set 3 : review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+209 lines, -92 lines) Patch
M content/browser/browser_side_navigation_browsertest.cc View 2 chunks +5 lines, -1 line 0 comments Download
M content/browser/cross_site_transfer_browsertest.cc View 4 chunks +4 lines, -0 lines 0 comments Download
M content/browser/frame_host/navigation_handle_impl_browsertest.cc View 1 2 3 chunks +45 lines, -91 lines 0 comments Download
M content/browser/frame_host/navigation_request.cc View 2 chunks +2 lines, -0 lines 0 comments Download
M content/browser/frame_host/navigator_impl.cc View 1 1 chunk +6 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 3 chunks +10 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_manager.cc View 2 chunks +7 lines, -0 lines 0 comments Download
M content/browser/site_per_process_browsertest.cc View 3 chunks +3 lines, -0 lines 0 comments Download
A content/public/test/navigation_handle_observer.h View 1 chunk +58 lines, -0 lines 0 comments Download
A content/public/test/navigation_handle_observer.cc View 1 chunk +67 lines, -0 lines 0 comments Download
M content/test/BUILD.gn View 1 chunk +2 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 37 (27 generated)
jam
1 month, 1 week ago (2017-05-16 22:39:44 UTC) #21
nasko (slow)
LGTM with a few nits. https://codereview.chromium.org/2889703002/diff/60001/content/browser/frame_host/navigation_handle_impl_browsertest.cc File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2889703002/diff/60001/content/browser/frame_host/navigation_handle_impl_browsertest.cc#newcode1053 content/browser/frame_host/navigation_handle_impl_browsertest.cc:1053: // renderer can't access, ...
1 month ago (2017-05-18 05:14:15 UTC) #23
jam
https://codereview.chromium.org/2889703002/diff/60001/content/browser/frame_host/navigation_handle_impl_browsertest.cc File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2889703002/diff/60001/content/browser/frame_host/navigation_handle_impl_browsertest.cc#newcode1053 content/browser/frame_host/navigation_handle_impl_browsertest.cc:1053: // renderer can't access, the right error code is ...
1 month ago (2017-05-18 16:00:33 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2889703002/80001
1 month ago (2017-05-18 16:02:29 UTC) #27
nasko (slow)
On 2017/05/18 16:00:33, jam wrote: > https://codereview.chromium.org/2889703002/diff/60001/content/browser/frame_host/navigation_handle_impl_browsertest.cc#newcode1076 > content/browser/frame_host/navigation_handle_impl_browsertest.cc:1076: if > (!IsBrowserSideNavigationEnabled()) > On 2017/05/18 ...
1 month ago (2017-05-18 16:04:25 UTC) #28
jam
On 2017/05/18 16:04:25, nasko wrote: > On 2017/05/18 16:00:33, jam wrote: > > > https://codereview.chromium.org/2889703002/diff/60001/content/browser/frame_host/navigation_handle_impl_browsertest.cc#newcode1076 ...
1 month ago (2017-05-18 16:13:42 UTC) #29
nasko (slow)
On 2017/05/18 16:13:42, jam wrote: > On 2017/05/18 16:04:25, nasko wrote: > > On 2017/05/18 ...
1 month ago (2017-05-18 16:33:06 UTC) #30
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/448664)
1 month ago (2017-05-18 21:24:57 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2889703002/80001
1 month ago (2017-05-18 23:20:26 UTC) #34
commit-bot: I haz the power
1 month ago (2017-05-19 01:36:09 UTC) #37
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/cb4ae15478ddb2f45a6e306a3de4...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318