Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2973433003:
Block redirects to renderer-debug urls. (Closed)
|
Created:
3 years, 5 months ago by arthursonzogni Modified:
3 years, 5 months 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. |
DescriptionBlock 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) #Messages
Total messages: 90 (65 generated)
The CQ bit was checked by arthursonzogni@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 checked by arthursonzogni@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...
arthursonzogni@chromium.org changed reviewers: + clamy@chromium.org, mmenke@chromium.org
Hi everyone, I would like to go back to this CL: https://crrev.com/2436253002 (8 month ago) The block with CanRequestURL was moved from ResourceLoader::OnReceivedRedirect to NavigationRequest::OnRequestRedirect. The problem is that now, the check only happens for renderer-initiated navigations that have |site_source_instance_| defined. Prior to this CL, it happened all the time. It looks like a regression. What do you think? I can't say, looking at the discussion, why it is like this. Not blocking some requests here causes the issue https://crbug.com/736658 and https://crbug.com/704892. In my CL, the trybots are green, but I did not expect it, since my CL is incomplete. I still doesn't really know which process I should check for |CanRequestURL()|. It could be: * The one that has initiated the navigation. * The one that is rendering the current frame. * The one that will render the future frame. It looks like it was checked against source_site_instance()->GetProcess(). I am not sure what source_site_instance() represents and when it is not undefined. +CC creis@, ananta@, jam@, nasko@
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
We talk with clamy@, CanRequestURL() should be called in the process that will load the future frame. It can only happen when we have one, i.e. at commit time. It should already be the case(?). The plan is to add ChildProcessSecurityPolicyImpl::CanRedirectToURL(const GURL&). It will be almost a copy-paste of ChildProcessSecurityPolicyImpl::CanRedirectToURL(), but only the part that doesn't depend on the process. That way, no redirect to a "renderer-debug" url will happen.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
Patchset #3 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
The CQ bit was checked by arthursonzogni@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 ========== [TEST] 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=736658,704892 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ========== to ========== 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=736658,704892 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ==========
arthursonzogni@chromium.org changed reviewers: + creis@chromium.org, mkwst@chromium.org
Hi Charlie and Mike, I wrote ChildProcessSecurityPolicyImpl::CanRedirectToURL, could you please review it? The way it is written makes that none of CanRedirectToURL and CanRequestURL/CanCommitURL is a subset of the other. In CanRedirectToURL, we know we are in a middle of a redirect. So we can be more restrictive. In CanRequestURL/CanCommitURL, we know on which process it happens. Process related permission/prohibition can be applied here. FYI, for solving the two bugs, I only need the block with "IsPseudoScheme". +CC csharrison@: FYI, this should fix https://crbug.com/736658.
csharrison@chromium.org changed reviewers: + csharrison@chromium.org
Since we know this happens in the wild, does it change web-facing behavior? If so maybe this deserves a PSA to blink-dev. However, I'll defer to Mike's decision if that is necessary.
On 2017/07/05 13:26:33, Charlie Harrison wrote: > Since we know this happens in the wild, does it change web-facing behavior? If > so maybe this deserves a PSA to blink-dev. However, I'll defer to Mike's > decision if that is necessary. My understanding is that these redirects are going to be blocked eventually, and Arthur's CL just moves the blocking earlier in time. This shouldn't be visible to developers.
Thanks a lot for making this change. I have one small nit and a question: How does behavior change with and without PlzNavigate? We're seeing different crashes depending on the experiment, and I want to understand if there's another bug lurking even behind this one. https://codereview.chromium.org/2973433003/diff/140001/content/browser/child_... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2973433003/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:674: if (IsPseudoScheme(url.scheme())) { nit: const std::string& scheme = url.scheme() and use it below, to avoid a copy.
On 2017/07/05 13:52:31, Charlie Harrison wrote: > Thanks a lot for making this change. I have one small nit and a question: How > does behavior change with and without PlzNavigate? We're seeing different > crashes depending on the experiment, and I want to understand if there's another > bug lurking even behind this one. Not being blocked at this point when being redirected to a javascript-url is a PlzNavigate-only bug caused by https://crrev.com/2436253002. So, if there is non-PlzNavigate reports, it means that there is another problem behind this one :-( FYI, in release mode, with PlzNavigate, when being redirected to a javascript-url, the browser sends a few dumpWithoutCrashing and then crashes in: [...:FATAL:web_contents_observer_sanity_checker.cc(169)] Check failed: !NavigationIsOngoing(navigation_handle). https://codereview.chromium.org/2973433003/diff/140001/content/browser/child_... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2973433003/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:674: if (IsPseudoScheme(url.scheme())) { On 2017/07/05 13:52:31, Charlie Harrison wrote: > nit: > const std::string& scheme = url.scheme() and use it below, to avoid a copy. In this case, for consistency, the same thing should be done in a few similar places in this file. I will apply your suggestion everywhere, but in order not to pollute this CL, it will be in a separate CL. Please see: https://codereview.chromium.org/2966203002
On 2017/07/05 14:52:19, arthursonzogni wrote: > On 2017/07/05 13:52:31, Charlie Harrison wrote: > > Thanks a lot for making this change. I have one small nit and a question: How > > does behavior change with and without PlzNavigate? We're seeing different > > crashes depending on the experiment, and I want to understand if there's > another > > bug lurking even behind this one. > > Not being blocked at this point when being redirected to a javascript-url is a > PlzNavigate-only bug caused by https://crrev.com/2436253002. > So, if there is non-PlzNavigate reports, it means that there is another problem > behind this one :-( > > FYI, in release mode, with PlzNavigate, when being redirected to a > javascript-url, the browser sends a few dumpWithoutCrashing and then crashes in: > [...:FATAL:web_contents_observer_sanity_checker.cc(169)] Check failed: > !NavigationIsOngoing(navigation_handle). > > https://codereview.chromium.org/2973433003/diff/140001/content/browser/child_... > File content/browser/child_process_security_policy_impl.cc (right): > > https://codereview.chromium.org/2973433003/diff/140001/content/browser/child_... > content/browser/child_process_security_policy_impl.cc:674: if > (IsPseudoScheme(url.scheme())) { > On 2017/07/05 13:52:31, Charlie Harrison wrote: > > nit: > > const std::string& scheme = url.scheme() and use it below, to avoid a copy. > > In this case, for consistency, the same thing should be done in a few similar > places in this file. > I will apply your suggestion everywhere, but in order not to pollute this CL, it > will be in a separate CL. > Please see: https://codereview.chromium.org/2966203002 captive_portal LGTM
non-owner LGTM, FYI Camille and I are investigating the non-PlzNavigate issue in https://codereview.chromium.org/2971833002/
Thanks for the reviews mmenke@ and csharrison@! Charlie (creis@): gentle ping :-) What do you think of this?
[CC nick@] Sorry for the delay-- between the holiday and some appts, I'm a bit behind. I'm a bit concerned that this is going to introduce regressions, since it's kind of hard to reason about whether the new restrictions are equivalent or correct. It seems like we might end up blocking some redirects which were previously allowed. Maybe you can help me understand how this is safe in the comments below? Or maybe we can take a more conservative approach that uses an existing check (e.g., CanRequestURL on the initiating process?) plus a few redirect-specific checks if needed? Alternatively, we could land it and see what bugs get filed, but that seems risky this close to branch cut. https://codereview.chromium.org/2973433003/diff/160001/chrome/browser/captive... File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right): https://codereview.chromium.org/2973433003/diff/160001/chrome/browser/captive... chrome/browser/captive_portal/captive_portal_tab_helper.cc:67: // Remove this code if it is never reached until ~ 2017-July-10. Sorry for the delayed review. Might want to bump this date back. https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:670: bool ChildProcessSecurityPolicyImpl::CanRedirectToURL(const GURL& url) { It makes me nervous to be doing a narrower version of CanRequestURL here, especially with no comments pointing out the similarities and differences. If we change one in the future, we may need to change both. I'm not sure I understand why we can't use CanRequestURL on the initiating process? If that process isn't allowed to request the post-redirect URL, then we probably shouldn't be allowing it. (The destination process seems unimportant, but I could be wrong.) https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:686: return IsWebSafeScheme(scheme); We're basically skipping the CanCommitURL and IsHandledURL checks from CanRequestURL. Are we sure this won't break things? We tend to find regressions after introducing restrictions like this, and it's not clear to me if this is a safe assumption. (e.g., Could some non-web-safe schemes redirect to each other from the right process type?) https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_... File content/browser/child_process_security_policy_unittest.cc (right): https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_... content/browser/child_process_security_policy_unittest.cc:170: EXPECT_TRUE(p->CanRedirectToURL(GURL("data:text/html,<b>Hi</b>"))); Hmm, this is surprising to me in the other direction, since we don't allow redirects to data URLs. Maybe we should eventually prevent that here as well? https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_... content/browser/child_process_security_policy_unittest.cc:242: // No redirect to a blob-url or a filesystem-url are allowed. nit: redirects https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_... content/browser/child_process_security_policy_unittest.cc:355: // These requests for chrome:// pages should be granted if there is no nit: s/is/are/ https://codereview.chromium.org/2973433003/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2973433003/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:632: base::debug::DumpWithoutCrashing(); We shouldn't leave DumpWithoutCrashing calls in the codebase. We should either have a TODO to remove this (as in the earlier case) or change it to a CHECK if we really don't expect it to happen. https://codereview.chromium.org/2973433003/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2973433003/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:1523: // Redirects to a renderer debug URLs caused problems. nit: Drop "a" https://codereview.chromium.org/2973433003/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:1524: // See https://crbug.com/728398. Should this bug be listed in the CL description as well? https://codereview.chromium.org/2973433003/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (left): https://codereview.chromium.org/2973433003/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:546: source_site_instance()->GetProcess()->FilterURL(false, &url); I don't see how it's ok to remove the FilterURL call. It seems indirect to assume that everything FilterURL would catch will be caught by CanRedirectToURL, and I'm not sure if it's true. Is there a reason this needed to be removed? https://codereview.chromium.org/2973433003/diff/160001/content/public/browser... File content/public/browser/child_process_security_policy.h (right): https://codereview.chromium.org/2973433003/diff/160001/content/public/browser... content/public/browser/child_process_security_policy.h:90: // Determine whether a redirect is allowed to proceed. Please clarify why this needs to be different than CanRequestURL. It's not obvious at first glance how they would differ. https://codereview.chromium.org/2973433003/diff/160001/content/public/browser... content/public/browser/child_process_security_policy.h:91: virtual bool CanRedirectToURL(const GURL& url) = 0; This doesn't appear to be used outside content/, so it shouldn't be in the public interface. Can you make it be a public method on ChildProcessSecurityPolicyImpl instead?
Description was changed from ========== 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=736658,704892 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ========== to ========== 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=736658,704892,728398 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ==========
Description was changed from ========== 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=736658,704892,728398 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ========== to ========== 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=704892,728398 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ==========
Description was changed from ========== 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=704892,728398 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ========== to ========== 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,736658 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ==========
@creis: As explained in the comments below, using CanRequestURL is not possible in that case, because we don't have a process that we can give it. Using the current process is wrong in the case of a browser-initiated navigation: it did not initiated the navigation and it will not necessarily commit the navigation. The goal here is to define a certain number of checks that we can apply to redirected urls regardless of the process that requested/will commit them, and apply these checks on every redirects. https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:670: bool ChildProcessSecurityPolicyImpl::CanRedirectToURL(const GURL& url) { On 2017/07/07 17:12:59, Charlie Reis (slow) wrote: > It makes me nervous to be doing a narrower version of CanRequestURL here, > especially with no comments pointing out the similarities and differences. If > we change one in the future, we may need to change both. > > I'm not sure I understand why we can't use CanRequestURL on the initiating > process? If that process isn't allowed to request the post-redirect URL, then > we probably shouldn't be allowing it. (The destination process seems > unimportant, but I could be wrong.) The issue is that we need to block redirects to renderer debug urls for all kind of navigations. This include browser-initiated navigations and renderer-initiated navigations that went through the OpenURL path. In those two cases, it makes no sense to call CanRequestURL with the current process, since it's quite possible that it will not be the process that will commit them. And in the case of browser-initiated navigations, this process did not even initiated the navigation in the first place. This is why I wanted Arthur to write a function that could check whether a redirect was allowed regardless of the process instead of calling CanRequestURL. https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:686: return IsWebSafeScheme(scheme); On 2017/07/07 17:12:59, Charlie Reis (slow) wrote: > We're basically skipping the CanCommitURL and IsHandledURL checks from > CanRequestURL. Are we sure this won't break things? We tend to find > regressions after introducing restrictions like this, and it's not clear to me > if this is a safe assumption. (e.g., Could some non-web-safe schemes redirect > to each other from the right process type?) As explained above, we can't really use the process in that check. Would you prefer if we just remove this check and always return true instead? This is what we're doing in PlzNavigate currently.
Patchset #5 (id:180001) has been deleted
Patchset #6 (id:220001) has been deleted
The CQ bit was checked by arthursonzogni@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 ========== 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,736658 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ========== to ========== 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 ==========
Patchset #6 (id:240001) has been deleted
The CQ bit was checked by arthursonzogni@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 arthursonzogni@chromium.org to run a CQ dry run
Patchset #6 (id:260001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I added a new patchset. It addresses the comments you made and: * CanRedirectToURL allows now a super set of CanRequestURL. Only invalid URLs and the one with a pseudo scheme are blocked (<-- blocking this will fix 728398) * The FilterURL check that was removed is now back. This is called only for renderer initiated navigations.
https://codereview.chromium.org/2973433003/diff/160001/chrome/browser/captive... File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right): https://codereview.chromium.org/2973433003/diff/160001/chrome/browser/captive... chrome/browser/captive_portal/captive_portal_tab_helper.cc:67: // Remove this code if it is never reached until ~ 2017-July-10. On 2017/07/07 17:12:59, Charlie Reis (slow) wrote: > Sorry for the delayed review. Might want to bump this date back. Done. https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_... File content/browser/child_process_security_policy_unittest.cc (right): https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_... content/browser/child_process_security_policy_unittest.cc:170: EXPECT_TRUE(p->CanRedirectToURL(GURL("data:text/html,<b>Hi</b>"))); On 2017/07/07 17:12:59, Charlie Reis (slow) wrote: > Hmm, this is surprising to me in the other direction, since we don't allow > redirects to data URLs. Maybe we should eventually prevent that here as well? Yes, eventually. So, this is already enforced somewhere else in the code. Maybe we shouldn't enforce the same thing twice (moving it here maybe?) In the next patch, I will make CanRedirectToURL more permissive such that CanRequestURL => CanRedirectToURL. Therefore, I can't make CanRedirectToURL("data:xxx") returning false. https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_... content/browser/child_process_security_policy_unittest.cc:242: // No redirect to a blob-url or a filesystem-url are allowed. On 2017/07/07 17:12:59, Charlie Reis (slow) wrote: > nit: redirects Line removed. https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_... content/browser/child_process_security_policy_unittest.cc:355: // These requests for chrome:// pages should be granted if there is no On 2017/07/07 17:12:59, Charlie Reis (slow) wrote: > nit: s/is/are/ Undoing my change on this line. https://codereview.chromium.org/2973433003/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2973433003/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:632: base::debug::DumpWithoutCrashing(); On 2017/07/07 17:12:59, Charlie Reis (slow) wrote: > We shouldn't leave DumpWithoutCrashing calls in the codebase. We should either > have a TODO to remove this (as in the earlier case) or change it to a CHECK if > we really don't expect it to happen. Done. https://codereview.chromium.org/2973433003/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2973433003/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:1523: // Redirects to a renderer debug URLs caused problems. On 2017/07/07 17:12:59, Charlie Reis (slow) wrote: > nit: Drop "a" Done. https://codereview.chromium.org/2973433003/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:1524: // See https://crbug.com/728398. On 2017/07/07 17:12:59, Charlie Reis (slow) wrote: > Should this bug be listed in the CL description as well? Yes, this is the main bug. I didn't used the correct ones in my first CL description. Thanks! https://codereview.chromium.org/2973433003/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (left): https://codereview.chromium.org/2973433003/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:546: source_site_instance()->GetProcess()->FilterURL(false, &url); I think we should do both: * CanRedirectToURL(url) for every redirect. * CanRequestURL(process, url) for non-browser initiated requests. 1) I will keep this check, but I would prefer putting it at the top of this function. I don't want to update the NavigationRequest for a redirect that isn't authorized. Preventing us from reusing these data by mistake is safer. 2) I think that CanRequestURL is more appropriate than FilterURL. Both are the same, except that FilterURL will modify |url| instead of returning false. At this location, it uses a temporary variable in order not to modify an existing one and does the check |url == url::kAboutBlankURL| in order to get the original result of CanRequestURL. That is weird. 3) I will remove the comment that talks about CSP. AFAIK, CSP aren't involved in FilterURL/CanRequestURL. https://codereview.chromium.org/2973433003/diff/160001/content/public/browser... File content/public/browser/child_process_security_policy.h (right): https://codereview.chromium.org/2973433003/diff/160001/content/public/browser... content/public/browser/child_process_security_policy.h:90: // Determine whether a redirect is allowed to proceed. On 2017/07/07 17:13:00, Charlie Reis (slow) wrote: > Please clarify why this needs to be different than CanRequestURL. It's not > obvious at first glance how they would differ. Done. https://codereview.chromium.org/2973433003/diff/160001/content/public/browser... content/public/browser/child_process_security_policy.h:91: virtual bool CanRedirectToURL(const GURL& url) = 0; On 2017/07/07 17:13:00, Charlie Reis (slow) wrote: > This doesn't appear to be used outside content/, so it shouldn't be in the > public interface. Can you make it be a public method on > ChildProcessSecurityPolicyImpl instead? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_browser_side_navigation_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks! LGTM with a few nits and one bug fix. https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:670: bool ChildProcessSecurityPolicyImpl::CanRedirectToURL(const GURL& url) { On 2017/07/10 12:29:19, clamy wrote: > On 2017/07/07 17:12:59, Charlie Reis (slow) wrote: > > It makes me nervous to be doing a narrower version of CanRequestURL here, > > especially with no comments pointing out the similarities and differences. If > > we change one in the future, we may need to change both. > > > > I'm not sure I understand why we can't use CanRequestURL on the initiating > > process? If that process isn't allowed to request the post-redirect URL, then > > we probably shouldn't be allowing it. (The destination process seems > > unimportant, but I could be wrong.) > > The issue is that we need to block redirects to renderer debug urls for all kind > of navigations. This include browser-initiated navigations and > renderer-initiated navigations that went through the OpenURL path. In those two > cases, it makes no sense to call CanRequestURL with the current process, since > it's quite possible that it will not be the process that will commit them. And > in the case of browser-initiated navigations, this process did not even > initiated the navigation in the first place. This is why I wanted Arthur to > write a function that could check whether a redirect was allowed regardless of > the process instead of calling CanRequestURL. I'm happy with where we ended up. I think there's still a reason to call CanRequestURL on renderer-initiated navigations, but I agree that we want some additional checks that also apply to browser-initiated navigations where there's no initiating process. https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:686: return IsWebSafeScheme(scheme); On 2017/07/10 12:29:19, clamy wrote: > On 2017/07/07 17:12:59, Charlie Reis (slow) wrote: > > We're basically skipping the CanCommitURL and IsHandledURL checks from > > CanRequestURL. Are we sure this won't break things? We tend to find > > regressions after introducing restrictions like this, and it's not clear to me > > if this is a safe assumption. (e.g., Could some non-web-safe schemes redirect > > to each other from the right process type?) > > As explained above, we can't really use the process in that check. Would you > prefer if we just remove this check and always return true instead? This is what > we're doing in PlzNavigate currently. Sure. If we need to, we can tighten it in a separate CL. https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_... File content/browser/child_process_security_policy_unittest.cc (right): https://codereview.chromium.org/2973433003/diff/160001/content/browser/child_... content/browser/child_process_security_policy_unittest.cc:170: EXPECT_TRUE(p->CanRedirectToURL(GURL("data:text/html,<b>Hi</b>"))); On 2017/07/10 16:07:03, arthursonzogni wrote: > On 2017/07/07 17:12:59, Charlie Reis (slow) wrote: > > Hmm, this is surprising to me in the other direction, since we don't allow > > redirects to data URLs. Maybe we should eventually prevent that here as well? > > Yes, eventually. > So, this is already enforced somewhere else in the code. Maybe we shouldn't > enforce the same thing twice (moving it here maybe?) > > In the next patch, I will make CanRedirectToURL more permissive such that > CanRequestURL => CanRedirectToURL. Therefore, I can't make > CanRedirectToURL("data:xxx") returning false. Acknowledged. https://codereview.chromium.org/2973433003/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (left): https://codereview.chromium.org/2973433003/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:546: source_site_instance()->GetProcess()->FilterURL(false, &url); On 2017/07/10 16:07:04, arthursonzogni wrote: > I think we should do both: > * CanRedirectToURL(url) for every redirect. > * CanRequestURL(process, url) for non-browser initiated requests. > > 1) I will keep this check, but I would prefer putting it at the top of this > function. I don't want to update the NavigationRequest for a redirect that isn't > authorized. Preventing us from reusing these data by mistake is safer. > > 2) I think that CanRequestURL is more appropriate than FilterURL. Both are the > same, except that FilterURL will modify |url| instead of returning false. > At this location, it uses a temporary variable in order not to modify an > existing one and does the check |url == url::kAboutBlankURL| in order to get the > original result of CanRequestURL. That is weird. > > 3) I will remove the comment that talks about CSP. AFAIK, CSP aren't involved in > FilterURL/CanRequestURL. Thanks! This sounds reasonable to me. https://codereview.chromium.org/2973433003/diff/280001/content/browser/child_... File content/browser/child_process_security_policy_impl.h (right): https://codereview.chromium.org/2973433003/diff/280001/content/browser/child_... content/browser/child_process_security_policy_impl.h:276: // represents a stricter subset. It must be used when appropriate (i.e. Slight rephrase: It must also be used for renderer-initiated navigations. https://codereview.chromium.org/2973433003/diff/280001/content/browser/child_... File content/browser/child_process_security_policy_unittest.cc (right): https://codereview.chromium.org/2973433003/diff/280001/content/browser/child_... content/browser/child_process_security_policy_unittest.cc:242: EXPECT_TRUE(p->CanRedirectToURL(GURL("blob:http://localhost/some-guid"))); Are redirects to blob and filesystem URLs prevented elsewhere, similar to data? If so, maybe we can mention that in this file. https://codereview.chromium.org/2973433003/diff/280001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2973433003/diff/280001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:516: // access to the URL. We always allow browser initiated requests. nit: Rephrase last sentence, since browser-initiated requests are still subject to CanRedirectToURL above. https://codereview.chromium.org/2973433003/diff/280001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:524: frame_tree_node_->ResetNavigationRequest(false, true); Don't forget to return early! :)
Patchset #7 (id:300001) has been deleted
The CQ bit was checked by arthursonzogni@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.
Thanks!
I addressed the comments. I had to had 4 new tests that checks what
happens with blob-url/filesystem-url and redirects.
From the tests, I learnt that:
* data-url: Blocked by net::DataProtocolHandler::IsSafeRedirectTarget().
* blob-url: Not blocked, but a 'file not found' response is generated in
net::BlobURLRequestJob::DidStart().
* filesystem-url: Not blocked.
These are probably not tested elsewhere (patchset #4 blocked redirects to
filesystem-url and no tests were here to prevent me from doing this).
That's a lot of new lines. Does it still LGTY Charlie?
https://codereview.chromium.org/2973433003/diff/280001/content/browser/child_...
File content/browser/child_process_security_policy_impl.h (right):
https://codereview.chromium.org/2973433003/diff/280001/content/browser/child_...
content/browser/child_process_security_policy_impl.h:276: // represents a
stricter subset. It must be used when appropriate (i.e.
On 2017/07/10 21:16:22, Charlie Reis (slow) wrote:
> Slight rephrase: It must also be used for renderer-initiated navigations.
Done.
https://codereview.chromium.org/2973433003/diff/280001/content/browser/child_...
File content/browser/child_process_security_policy_unittest.cc (right):
https://codereview.chromium.org/2973433003/diff/280001/content/browser/child_...
content/browser/child_process_security_policy_unittest.cc:242:
EXPECT_TRUE(p->CanRedirectToURL(GURL("blob:http://localhost/some-guid")));
On 2017/07/10 21:16:22, Charlie Reis (slow) wrote:
> Are redirects to blob and filesystem URLs prevented elsewhere, similar to
data?
> If so, maybe we can mention that in this file.
I added some notes about this the CanRedirectToURL function.
Done.
https://codereview.chromium.org/2973433003/diff/280001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):
https://codereview.chromium.org/2973433003/diff/280001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:516: // access to the URL. We
always allow browser initiated requests.
On 2017/07/10 21:16:22, Charlie Reis (slow) wrote:
> nit: Rephrase last sentence, since browser-initiated requests are still
subject
> to CanRedirectToURL above.
Done.
https://codereview.chromium.org/2973433003/diff/280001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:524:
frame_tree_node_->ResetNavigationRequest(false, true);
On 2017/07/10 21:16:22, Charlie Reis (slow) wrote:
> Don't forget to return early! :)
:) Done.
Patchset #8 (id:340001) has been deleted
The CQ bit was checked by arthursonzogni@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by arthursonzogni@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
I am a little in a hurry to commit this change. In order to be able to commit it
faster, I removed the 4 additional tests. Those were not really related to this
change. They were just a way for us to understand and verify how chrome is
working with redirect to {blob,filesystem}-url. They will be added in a follow
up.
So now, there is nothing new to review, except this small comment that tries to
explain what to expect with {blob,filesystem}-url. Camille could you review this
part? I am not fully sure whether it needs to be rephrased or not. Thanks!
```
// Note about redirects and some special URLs:
// * data-url: Blocked by net::DataProtocolHandler::IsSafeRedirectTarget().
// * blob-url: Not necessary blocked, but a 'file not found' response will be
// generated in net::BlobURLRequestJob::DidStart().
// * filesystem-url: Not necessary blocked and the response can be displayed.
//
// Depending on their inner origins and if the request is browser-initiated or
// renderer-initiated, blob-urls and filesystem-urls might get blocked by
// CanCommitURL or in DocumentLoader::RedirectReceived.
```
Thanks! Some comments on comments. https://codereview.chromium.org/2973433003/diff/400001/content/browser/child_... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2973433003/diff/400001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:686: // Note about redirects and some special URLs: s/some/ https://codereview.chromium.org/2973433003/diff/400001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:688: // * blob-url: Not necessary blocked, but a 'file not found' response will be * Move the Depending... paragraph above this line. * s/Not necessary blocked, but /If not blocked, (same for the filesystem part).
The CQ bit was checked by arthursonzogni@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...
Thanks Camille! https://codereview.chromium.org/2973433003/diff/400001/content/browser/child_... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2973433003/diff/400001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:686: // Note about redirects and some special URLs: On 2017/07/12 14:19:57, clamy wrote: > s/some/ Done. https://codereview.chromium.org/2973433003/diff/400001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:688: // * blob-url: Not necessary blocked, but a 'file not found' response will be On 2017/07/12 14:19:56, clamy wrote: > * Move the Depending... paragraph above this line. > * s/Not necessary blocked, but /If not blocked, (same for the filesystem part). Done.
The CQ bit was unchecked by arthursonzogni@chromium.org
The CQ bit was checked by arthursonzogni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, csharrison@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2973433003/#ps420001 (title: "Addressed comment (@clamy)")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the investigation-- still LGTM. (Sorry for not getting to this yesterday, but I agree with your decision to add the other tests separately.)
CQ is committing da patch.
Bot data: {"patchset_id": 420001, "attempt_start_ts": 1499876837510780,
"parent_rev": "31b2b6fb8ebda8f9584443a6f7a6fb8e64c4a529", "commit_rev":
"20d3cb97ed88d48e6a069912fd3aea58f6491be2"}
CQ is committing da patch.
Bot data: {"patchset_id": 420001, "attempt_start_ts": 1499876837510780,
"parent_rev": "a0d90a5df5cf55c20e5409419800bc379a21dd16", "commit_rev":
"b980b4b5d4408bea7e83cbf1f70e7143edb41716"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/b980b4b5d4408bea7e83cbf1f70e... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:420001) as https://chromium.googlesource.com/chromium/src/+/b980b4b5d4408bea7e83cbf1f70e...
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:420001) has been created in https://codereview.chromium.org/2982623002/ by hongchan@chromium.org. The reason for reverting is: Compile error. https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%... https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.chromiumos%2FLi... FAILED: obj/content/test/content_browsertests/navigation_handle_impl_browsertest.o /b/c/goma_client/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/content/test/content_browsertests/navigation_handle_impl_browsertest.o.d -DHAS_OUT_OF_PROC_TEST_RUNNER -DV8_DEPRECATION_WARNINGS -DUSE_UDEV -DUSE_ASH=1 -DUSE_AURA=1 -DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -DUSE_X11=1 -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -DCR_CLANG_REVISION=\"307486-1\" -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DCOMPONENT_BUILD -DOS_CHROMEOS -D_DEBUG -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_GLIBCXX_DEBUG=1 -DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_32 -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_26 -DGL_GLEXT_PROTOTYPES -DUSE_GLX -DUSE_EGL -DGTEST_API_= -DGTEST_HAS_POSIX_RE=0 -DGTEST_LANG_CXX11=1 -DTOOLKIT_VIEWS=1 -DV8_USE_EXTERNAL_STARTUP_DATA -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -DUCHAR_TYPE=uint16_t -DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DSK_HAS_PNG_LIBRARY -DSK_HAS_WEBP_LIBRARY -DSK_HAS_JPEG_LIBRARY -DSKIA_DLL -DGR_GL_IGNORE_ES3_MSAA=0 -DSK_SUPPORT_GPU=1 -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DHAVE_PTHREAD -DPROTOBUF_USE_DLLS -DBORINGSSL_SHARED_LIBRARY -DUSING_V8_SHARED -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -DWTF_USE_WEBAUDIO_FFMPEG=1 -DWTF_USE_DEFAULT_RENDER_THEME=1 -DUSE_LIBJPEG_TURBO=1 -DUSING_V8_SHARED -DUNIT_TEST -DLEVELDB_PLATFORM_CHROMIUM=1 -I../.. -Igen -I../../build/linux/debian_jessie_amd64-sysroot/usr/include/glib-2.0 -I../../build/linux/debian_jessie_amd64-sysroot/usr/lib/x86_64-linux-gnu/glib-2.0/include -I../../third_party/libwebp/src -I../../third_party/khronos -I../../gpu -I../../third_party/googletest/src/googletest/include -I../../third_party/ced/src -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n -I../../skia/config -I../../skia/ext -I../../third_party/skia/include/c -I../../third_party/skia/include/config -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/encode -I../../third_party/skia/include/images -I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops -I../../third_party/skia/include/pdf -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../third_party/skia/third_party/vulkan -I../../third_party/skia/include/gpu -I../../third_party/skia/src/gpu -I../../third_party/skia/src/sksl -I../../third_party/mesa/src/include -I../../third_party/libwebm/source -I../../third_party/protobuf/src -Igen/protoc_out -I../../third_party/protobuf/src -I../../third_party/boringssl/src/include -I../../build/linux/debian_jessie_amd64-sysroot/usr/include/nss -I../../build/linux/debian_jessie_amd64-sysroot/usr/include/nspr -Igen -I../../third_party/WebKit -Igen/third_party/WebKit -I../../v8/include -Igen/v8/include -I../../third_party/WebKit/Source -I../../third_party/WebKit -Igen/blink -Igen/third_party/WebKit -I../../third_party/libjpeg_turbo -I../../third_party/iccjpeg -I../../third_party/libpng -I../../third_party/zlib -I../../third_party/ots/include -I../../v8/include -Igen/v8/include -I../../third_party/googletest/custom -I../../third_party/googletest/src/googlemock/include -I../../third_party/leveldatabase -I../../third_party/leveldatabase/src -I../../third_party/leveldatabase/src/include -I../../build/linux/debian_jessie_amd64-sysroot/usr/include/dbus-1.0 -I../../build/linux/debian_jessie_amd64-sysroot/usr/lib/x86_64-linux-gnu/dbus-1.0/include -fno-strict-aliasing -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -funwind-tables -fPIC -pipe -B../../third_party/binutils/Linux_x64/Release/bin -pthread -fcolor-diagnostics -fdebug-prefix-map=/b/c/b/linux_chromeos/src=. -m64 -march=x86-64 -Wall -Werror -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -Wno-undefined-var-template -Wno-nonportable-include-path -Wno-address-of-packed-member -Wno-unused-lambda-capture -Wno-user-defined-warnings -O0 -fno-omit-frame-pointer -g2 -gsplit-dwarf --sysroot=../../build/linux/debian_jessie_amd64-sysroot -fvisibility=hidden -Xclang -load -Xclang ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang check-auto-raw-pointer -Xclang -plugin-arg-find-bad-constructs -Xclang check-ipc -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -Wno-header-guard -Xclang -load -Xclang ../../third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.so -Xclang -add-plugin -Xclang blink-gc-plugin -Xclang -plugin-arg-blink-gc-plugin -Xclang use-chromium-style-naming -Wno-undefined-bool-conversion -Wno-tautological-undefined-compare -std=gnu++11 -fno-rtti -fno-exceptions -fvisibility-inlines-hidden -c ../../content/browser/frame_host/navigation_handle_impl_browsertest.cc -o obj/content/test/content_browsertests/navigation_handle_impl_browsertest.o ../../content/browser/frame_host/navigation_handle_impl_browsertest.cc:1562:23: error: chosen constructor is explicit in copy-initialization std::vector<GURL> redirected_navigation = {}; ^ ~~ ../../build/linux/debian_jessie_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/debug/vector:79:7: note: explicit constructor declared here vector(const _Allocator& __a = _Allocator()) ^ ../../content/browser/frame_host/navigation_handle_impl_browsertest.cc:1565:5: error: no type named 'Compare' in the global namespace EXPECT_EQ(redirected_navigation, logger.redirected_navigation_urls()); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../third_party/googletest/src/googletest/include/gtest/gtest.h:1924:63: note: expanded from macro 'EXPECT_EQ' EqHelper<GTEST_IS_NULL_LITERAL_(val1)>::Compare, \ ~~^ ../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:162:23: note: expanded from macro 'EXPECT_PRED_FORMAT2' GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_) ^~~~~~~~~~~ ../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:147:17: note: expanded from macro 'GTEST_PRED_FORMAT2_' GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), \ ^~~~~~~~~~~ ../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:77:52: note: expanded from macro 'GTEST_ASSERT_' if (const ::testing::AssertionResult gtest_ar = (expression)) \ ^~~~~~~~~~ ../../content/browser/frame_host/navigation_handle_impl_browsertest.cc:1565:5: error: expected ')' ../../third_party/googletest/src/googletest/include/gtest/gtest.h:1923:3: note: expanded from macro 'EXPECT_EQ' EXPECT_PRED_FORMAT2(::testing::internal:: \ ^ ../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:162:3: note: expanded from macro 'EXPECT_PRED_FORMAT2' GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_) ^ ../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:147:29: note: expanded from macro 'GTEST_PRED_FORMAT2_' GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), \ ^ <scratch space>:36:1: note: expanded from here "redirected_navigation" ^ ../../content/browser/frame_host/navigation_handle_impl_browsertest.cc:1565:5: note: to match this '(' ../../third_party/googletest/src/googletest/include/gtest/gtest.h:1923:3: note: expanded from macro 'EXPECT_EQ' EXPECT_PRED_FORMAT2(::testing::internal:: \ ^ ../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:162:3: note: expanded from macro 'EXPECT_PRED_FORMAT2' GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_) ^ ../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:147:28: note: expanded from macro 'GTEST_PRED_FORMAT2_' GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), \ ^ 3 errors generated..
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) confirmed this CL at revision 486091 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
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.
Message was sent while issue was closed.
Patchset #13 (id:460001) has been deleted
Message was sent while issue was closed.
Patchset #12 (id:440001) has been deleted |
