Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(13)

Issue 2436253002: PlzNavigate: Fix the FindInPageControllerTest.SearchWithinSpecialURL browser test. (Closed)

Created:
4 years, 2 months ago by ananta
Modified:
4 years, 1 month ago
Reviewers:
Charlie Reis, jam, nasko
CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, Randy Smith (Not in Mondays), darin-cc_chromium.org, loading-reviews_chromium.org, mmenke
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

PlzNavigate: Fix the FindInPageControllerTest.SearchWithinSpecialURL browser test. This test was initiating a navigation to the chrome\test\data folder and performing a number of find requests on the page returned. With PlzNavigate the request to navigate to chrome\test\data folder would get blocked in ResourceLoader::OnReceivedRedirect because the process id (-1) did not have access to the url being navigated to. In the non PlzNavigate case this is not an issue because the child process is given access to the URL during navigation. Proposed fix for PlzNavigate is to not do the ChildProcessSecurityPolicy check in the ResourceLoader::OnReceivedRedirect function and instead do this check on the UI thread in the NavigationRequest::OnRedirectChecksComplete function. Additionally we also grant access to the URL in the NavigatorImpl::RequestNavigation function. The above test is now enabled for Windows. BUG=175711 Disabled due to crbug.com/175711 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/7bb43d67968cfd1f9d56e0dbd1ce1dbb2fefe46f Cr-Commit-Position: refs/heads/master@{#427279}

Patch Set 1 #

Total comments: 8

Patch Set 2 : The URL security checks for PlzNavigate should be handled in ResourceLoader for non frame resources… #

Total comments: 9

Patch Set 3 : Update comments #

Patch Set 4 : Don't check for URL access in NavigationRequest::OnRedirectChecksComplete() for browser initiated n… #

Patch Set 5 : Revert changes to navigator_impl.cc #

Total comments: 4

Patch Set 6 : Set source_site_instance in cases where it is null and a source contents is available and use it fo… #

Patch Set 7 : Perform access checks on the URL only for renderer initiated navigations. #

Patch Set 8 : Revert changes to browser_navigator.cc #

Total comments: 3

Patch Set 9 : Use FilterURL to validate access from the renderer for a URL being redirected. #

Total comments: 6

Patch Set 10 : Address creis review comments #

Total comments: 2

Patch Set 11 : Address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+33 lines, -10 lines) Patch
M chrome/browser/ui/find_bar/find_bar_host_browsertest.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M content/browser/frame_host/navigation_request.cc View 1 2 3 4 5 6 7 8 9 10 4 chunks +15 lines, -1 line 0 comments Download
M content/browser/loader/DEPS View 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/loader/resource_loader.cc View 1 2 3 4 5 6 7 8 9 2 chunks +15 lines, -7 lines 0 comments Download

Messages

Total messages: 50 (27 generated)
ananta
4 years, 2 months ago (2016-10-21 01:55:36 UTC) #4
ananta
+nasko
4 years, 2 months ago (2016-10-21 18:33:55 UTC) #9
jam
oops, sorry I forgot to hit publish :( https://codereview.chromium.org/2436253002/diff/1/content/browser/loader/resource_loader.cc File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2436253002/diff/1/content/browser/loader/resource_loader.cc#newcode262 content/browser/loader/resource_loader.cc:262: if ...
4 years, 2 months ago (2016-10-21 20:22:38 UTC) #10
jam
https://codereview.chromium.org/2436253002/diff/1/content/browser/frame_host/navigation_request.cc File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2436253002/diff/1/content/browser/frame_host/navigation_request.cc#newcode520 content/browser/frame_host/navigation_request.cc:520: !ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL( it seems a bit odd to be checking ...
4 years, 2 months ago (2016-10-21 20:28:21 UTC) #11
ananta
https://codereview.chromium.org/2436253002/diff/1/content/browser/frame_host/navigation_request.cc File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2436253002/diff/1/content/browser/frame_host/navigation_request.cc#newcode520 content/browser/frame_host/navigation_request.cc:520: !ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL( On 2016/10/21 20:28:20, jam wrote: > it seems ...
4 years, 2 months ago (2016-10-21 21:08:29 UTC) #12
ananta
https://codereview.chromium.org/2436253002/diff/1/content/browser/loader/resource_loader.cc File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2436253002/diff/1/content/browser/loader/resource_loader.cc#newcode262 content/browser/loader/resource_loader.cc:262: if (!IsBrowserSideNavigationEnabled()) { On 2016/10/21 20:22:38, jam wrote: > ...
4 years, 2 months ago (2016-10-21 21:08:59 UTC) #13
nasko
Adding creis@, as I don't know the history of why the IO thread check on ...
4 years, 2 months ago (2016-10-21 21:21:07 UTC) #15
jam
https://codereview.chromium.org/2436253002/diff/1/content/browser/frame_host/navigation_request.cc File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2436253002/diff/1/content/browser/frame_host/navigation_request.cc#newcode520 content/browser/frame_host/navigation_request.cc:520: !ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL( On 2016/10/21 21:08:29, ananta wrote: > On 2016/10/21 ...
4 years, 2 months ago (2016-10-21 21:25:24 UTC) #16
ananta
https://codereview.chromium.org/2436253002/diff/20001/content/browser/frame_host/navigator_impl.cc File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2436253002/diff/20001/content/browser/frame_host/navigator_impl.cc#newcode1141 content/browser/frame_host/navigator_impl.cc:1141: ChildProcessSecurityPolicyImpl::GetInstance()->GrantRequestURL( On 2016/10/21 21:21:07, nasko (slow) wrote: > Why ...
4 years, 2 months ago (2016-10-21 21:53:01 UTC) #17
nasko
https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/resource_loader.cc File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/resource_loader.cc#newcode262 content/browser/loader/resource_loader.cc:262: if (!IsBrowserSideNavigationEnabled() || info->GetChildID() != -1 || On 2016/10/21 ...
4 years, 2 months ago (2016-10-21 22:47:49 UTC) #20
Charlie Reis
https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/resource_loader.cc File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/resource_loader.cc#newcode262 content/browser/loader/resource_loader.cc:262: if (!IsBrowserSideNavigationEnabled() || info->GetChildID() != -1 || On 2016/10/21 ...
4 years, 2 months ago (2016-10-21 23:01:44 UTC) #21
ananta
https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/resource_loader.cc File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/resource_loader.cc#newcode262 content/browser/loader/resource_loader.cc:262: if (!IsBrowserSideNavigationEnabled() || info->GetChildID() != -1 || On 2016/10/21 ...
4 years, 2 months ago (2016-10-22 00:20:42 UTC) #22
ananta
https://codereview.chromium.org/2436253002/diff/140001/content/browser/frame_host/navigation_request.cc File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2436253002/diff/140001/content/browser/frame_host/navigation_request.cc#newcode521 content/browser/frame_host/navigation_request.cc:521: if (!browser_initiated_ && source_site_instance()) { Verified that the browser_initiated_ ...
4 years, 2 months ago (2016-10-22 03:47:32 UTC) #29
jam
https://codereview.chromium.org/2436253002/diff/140001/content/browser/frame_host/navigation_request.cc File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2436253002/diff/140001/content/browser/frame_host/navigation_request.cc#newcode521 content/browser/frame_host/navigation_request.cc:521: if (!browser_initiated_ && source_site_instance()) { actually, we should probably ...
4 years, 1 month ago (2016-10-24 16:59:16 UTC) #32
ananta
https://codereview.chromium.org/2436253002/diff/140001/content/browser/frame_host/navigation_request.cc File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2436253002/diff/140001/content/browser/frame_host/navigation_request.cc#newcode521 content/browser/frame_host/navigation_request.cc:521: if (!browser_initiated_ && source_site_instance()) { On 2016/10/24 16:59:16, jam ...
4 years, 1 month ago (2016-10-24 19:53:19 UTC) #35
Charlie Reis
Thanks! A few more questions to see if I understand the new checks. https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/resource_loader.cc File ...
4 years, 1 month ago (2016-10-24 22:04:36 UTC) #36
ananta
https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/resource_loader.cc File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/resource_loader.cc#newcode262 content/browser/loader/resource_loader.cc:262: if (!IsBrowserSideNavigationEnabled() || info->GetChildID() != -1 || On 2016/10/24 ...
4 years, 1 month ago (2016-10-24 23:01:41 UTC) #37
Charlie Reis
Thanks, and sorry for all the corner cases to consider! LGTM.
4 years, 1 month ago (2016-10-24 23:15:41 UTC) #42
jam
lgtm with nit https://codereview.chromium.org/2436253002/diff/180001/content/browser/frame_host/navigation_request.cc File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2436253002/diff/180001/content/browser/frame_host/navigation_request.cc#newcode347 content/browser/frame_host/navigation_request.cc:347: NavigationRequest::OnRedirectChecksComplete(NavigationThrottle::CANCEL); why not just call frame_tree_node_->ResetNavigationRequest(false); ...
4 years, 1 month ago (2016-10-25 00:17:09 UTC) #43
ananta
https://codereview.chromium.org/2436253002/diff/180001/content/browser/frame_host/navigation_request.cc File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2436253002/diff/180001/content/browser/frame_host/navigation_request.cc#newcode347 content/browser/frame_host/navigation_request.cc:347: NavigationRequest::OnRedirectChecksComplete(NavigationThrottle::CANCEL); On 2016/10/25 00:17:08, jam wrote: > why not ...
4 years, 1 month ago (2016-10-25 03:37:03 UTC) #44
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/2436253002/200001
4 years, 1 month ago (2016-10-25 03:37:24 UTC) #47
commit-bot: I haz the power
Committed patchset #11 (id:200001)
4 years, 1 month ago (2016-10-25 05:08:42 UTC) #48
commit-bot: I haz the power
4 years, 1 month ago (2016-10-25 05:11:00 UTC) #50
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/7bb43d67968cfd1f9d56e0dbd1ce1dbb2fefe46f
Cr-Commit-Position: refs/heads/master@{#427279}

Powered by Google App Engine
This is Rietveld 408576698