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

Issue 2973433003: Block redirects to renderer-debug urls. (Closed)

Created:
4 months, 3 weeks ago by arthursonzogni
Modified:
4 months, 2 weeks ago
CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, ananta, nasko, Charlie Harrison, ncarter (slow)
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Block redirects to renderer-debug urls. Redirects toward renderer-debug urls (chrome://crash, javascript:[...], ...) were not expected to happen in the NavigationHandleImpl. It caused WebContentsObserver::DidFinishNavigation not being called and consequently, bugs in some WebContentsObservers. This CL blocks any redirects to renderer-debug urls. One test is added. The blocking of the URL was moved (and partially removed?) in: https://crrev.com/2436253002 This CL tries to restore the previous behavior. BUG=728398 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel Review-Url: https://codereview.chromium.org/2973433003 Cr-Commit-Position: refs/heads/master@{#486091} Committed: https://chromium.googlesource.com/chromium/src/+/b980b4b5d4408bea7e83cbf1f70e7143edb41716

Patch Set 1 #

Patch Set 2 : improve the test. #

Patch Set 3 : Add CanRedirectToURL #

Total comments: 2

Patch Set 4 : Nit: Charlie Harrison #

Total comments: 28

Patch Set 5 : Rebase #

Patch Set 6 : Addressed comments. Add CanRequestURL again. #

Total comments: 8

Patch Set 7 : addressed comment (@creis) and add 4 tests. #

Patch Set 8 : Rebase. #

Patch Set 9 : Removing the 4 tests unrelated to this change (will add them in a followup). #

Patch Set 10 : Put a more specific comment. #

Total comments: 4

Patch Set 11 : Addressed comment (@clamy) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+202 lines, -39 lines) Patch
M chrome/browser/captive_portal/captive_portal_tab_helper.cc View 1 2 3 4 5 2 chunks +2 lines, -7 lines 0 comments Download
M content/browser/child_process_security_policy_impl.h View 1 2 3 4 5 6 1 chunk +7 lines, -0 lines 0 comments Download
M content/browser/child_process_security_policy_impl.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +24 lines, -0 lines 0 comments Download
M content/browser/child_process_security_policy_unittest.cc View 1 2 3 4 5 25 chunks +80 lines, -1 line 0 comments Download
M content/browser/frame_host/navigation_handle_impl.h View 1 2 3 4 5 6 7 1 chunk +0 lines, -4 lines 0 comments Download
M content/browser/frame_host/navigation_handle_impl.cc View 1 2 3 4 5 6 7 4 chunks +7 lines, -12 lines 0 comments Download
M content/browser/frame_host/navigation_handle_impl_browsertest.cc View 1 2 3 4 5 7 8 4 chunks +56 lines, -0 lines 0 comments Download
M content/browser/frame_host/navigation_request.cc View 1 2 3 4 5 6 3 chunks +26 lines, -15 lines 0 comments Download

Messages

Total messages: 90 (65 generated)
arthursonzogni
Hi everyone, I would like to go back to this CL: https://crrev.com/2436253002 (8 month ago) ...
4 months, 3 weeks ago (2017-07-04 12:20:11 UTC) #6
arthursonzogni
We talk with clamy@, CanRequestURL() should be called in the process that will load the ...
4 months, 3 weeks ago (2017-07-04 13:18:03 UTC) #10
arthursonzogni
Hi Charlie and Mike, I wrote ChildProcessSecurityPolicyImpl::CanRedirectToURL, could you please review it? The way it ...
4 months, 3 weeks ago (2017-07-05 12:32:23 UTC) #25
Charlie Harrison
Since we know this happens in the wild, does it change web-facing behavior? If so ...
4 months, 3 weeks ago (2017-07-05 13:26:33 UTC) #27
clamy
On 2017/07/05 13:26:33, Charlie Harrison wrote: > Since we know this happens in the wild, ...
4 months, 3 weeks ago (2017-07-05 13:30:25 UTC) #28
Charlie Harrison
Thanks a lot for making this change. I have one small nit and a question: ...
4 months, 3 weeks ago (2017-07-05 13:52:31 UTC) #29
arthursonzogni
On 2017/07/05 13:52:31, Charlie Harrison wrote: > Thanks a lot for making this change. I ...
4 months, 3 weeks ago (2017-07-05 14:52:19 UTC) #30
mmenke
On 2017/07/05 14:52:19, arthursonzogni wrote: > On 2017/07/05 13:52:31, Charlie Harrison wrote: > > Thanks ...
4 months, 3 weeks ago (2017-07-05 15:26:39 UTC) #31
Charlie Harrison
non-owner LGTM, FYI Camille and I are investigating the non-PlzNavigate issue in https://codereview.chromium.org/2971833002/
4 months, 3 weeks ago (2017-07-05 18:32:41 UTC) #32
arthursonzogni
Thanks for the reviews mmenke@ and csharrison@! Charlie (creis@): gentle ping :-) What do you ...
4 months, 3 weeks ago (2017-07-06 15:31:16 UTC) #33
Charlie Reis
[CC nick@] Sorry for the delay-- between the holiday and some appts, I'm a bit ...
4 months, 2 weeks ago (2017-07-07 17:13:00 UTC) #34
clamy
@creis: As explained in the comments below, using CanRequestURL is not possible in that case, ...
4 months, 2 weeks ago (2017-07-10 12:29:19 UTC) #38
arthursonzogni
I added a new patchset. It addresses the comments you made and: * CanRedirectToURL allows ...
4 months, 2 weeks ago (2017-07-10 16:06:29 UTC) #52
arthursonzogni
https://codereview.chromium.org/2973433003/diff/160001/chrome/browser/captive_portal/captive_portal_tab_helper.cc File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right): https://codereview.chromium.org/2973433003/diff/160001/chrome/browser/captive_portal/captive_portal_tab_helper.cc#newcode67 chrome/browser/captive_portal/captive_portal_tab_helper.cc:67: // Remove this code if it is never reached ...
4 months, 2 weeks ago (2017-07-10 16:07:05 UTC) #53
Charlie Reis
Thanks! LGTM with a few nits and one bug fix. https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_process_security_policy_impl.cc File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_process_security_policy_impl.cc#newcode670 ...
4 months, 2 weeks ago (2017-07-10 21:16:22 UTC) #56
arthursonzogni
Thanks! I addressed the comments. I had to had 4 new tests that checks what ...
4 months, 2 weeks ago (2017-07-11 16:21:31 UTC) #62
arthursonzogni
I am a little in a hurry to commit this change. In order to be ...
4 months, 2 weeks ago (2017-07-12 13:39:06 UTC) #72
clamy
Thanks! Some comments on comments. https://codereview.chromium.org/2973433003/diff/400001/content/browser/child_process_security_policy_impl.cc File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2973433003/diff/400001/content/browser/child_process_security_policy_impl.cc#newcode686 content/browser/child_process_security_policy_impl.cc:686: // Note about redirects ...
4 months, 2 weeks ago (2017-07-12 14:19:57 UTC) #73
arthursonzogni
Thanks Camille! https://codereview.chromium.org/2973433003/diff/400001/content/browser/child_process_security_policy_impl.cc File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2973433003/diff/400001/content/browser/child_process_security_policy_impl.cc#newcode686 content/browser/child_process_security_policy_impl.cc:686: // Note about redirects and some special ...
4 months, 2 weeks ago (2017-07-12 14:27:38 UTC) #76
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/2973433003/420001
4 months, 2 weeks ago (2017-07-12 16:27:24 UTC) #80
Charlie Reis
Thanks for the investigation-- still LGTM. (Sorry for not getting to this yesterday, but I ...
4 months, 2 weeks ago (2017-07-12 17:09:59 UTC) #81
commit-bot: I haz the power
Committed patchset #11 (id:420001) as https://chromium.googlesource.com/chromium/src/+/b980b4b5d4408bea7e83cbf1f70e7143edb41716
4 months, 2 weeks ago (2017-07-12 21:44:28 UTC) #85
hongchan
A revert of this CL (patchset #11 id:420001) has been created in https://codereview.chromium.org/2982623002/ by hongchan@chromium.org. ...
4 months, 2 weeks ago (2017-07-12 22:02:06 UTC) #86
findit-for-me
Findit (https://goo.gl/kROfz5) confirmed this CL at revision 486091 as the culprit for failures in the ...
4 months, 2 weeks ago (2017-07-12 22:28:45 UTC) #87
arthursonzogni
4 months, 2 weeks ago (2017-07-13 08:21:49 UTC) #88
Message was sent while issue was closed.
Reverted for compile error :-(
```
std::vector<GURL> started_navigation = {redirecting_url};
std::vector<GURL> redirected_navigation = {};
std::vector<GURL> finished_navigation = {redirecting_url};
```
There is cross-compiler differences. Using the copy-initializer with an empty
list-initializer may work or not depending on the compiler.
https://stackoverflow.com/a/17264506

Obviously, I can use the default constructor instead.

Powered by Google App Engine
This is Rietveld efc10ee0f