Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 11 months ago by Mike West Modified:
4 years, 7 months ago Reviewers:
jochen (gone - plz use gerrit), edwardjung, carlosk, nasko, Ilya Sherman, mmenke CC:
blink-reviews, carlosk, chromium-reviews, clamy, creis+watch_chromium.org, darin-cc_chromium.org, davidben, gavinp+loader_chromium.org, jam, Nate Chapin, loading-reviews_chromium.org, nasko+codewatch_chromium.org, site-isolation-reviews_chromium.org, tyoshino+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@block-response Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce AncestorThrottle, which will process 'X-Frame-Options' headers.
This moves the ancestor-based blocking behavior from Blink up into the
browser, and depends on https://codereview.chromium.org/1616943003 for
some infrastructure changes.
BUG=555418
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/26a6fc92ae361b4271f8f2197abe7eb063fc43ed
Cr-Commit-Position: refs/heads/master@{#392032}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Rebase. #Patch Set 3 : Fix. #
Total comments: 14
Patch Set 4 : Hrm. #
Total comments: 2
Patch Set 5 : Rebasing on top of https://codereview.chromium.org/1920873002 #Patch Set 6 : Drop XFO from Blink. #
Total comments: 5
Patch Set 7 : Rebase. #Patch Set 8 : Test+ErrorPage #
Total comments: 10
Patch Set 9 : mmenke@ + Windows #Patch Set 10 : rebase #
Total comments: 1
Patch Set 11 : WINDOWS. #
Total comments: 1
Patch Set 12 : CONNECTION_REFUSED #
Total comments: 3
Patch Set 13 : DCHECK. #
Total comments: 1
Created: 4 years, 7 months ago
Messages
Total messages: 59 (19 generated)
mkwst@chromium.org changed reviewers: + clamy@chromium.org, davidben@chromium.org, ellyjones@chromium.org, nasko@chromium.org
CCing folks to whom I'd asked questions on the previous CL. :)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1617043002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617043002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Few comments, mostly nits. https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... File content/browser/frame_host/ancestor_throttle.cc (right): https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... content/browser/frame_host/ancestor_throttle.cc:76: return NavigationThrottle::PROCEED; Shouldn't we be failing closed? https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... content/browser/frame_host/ancestor_throttle.cc:98: static_cast<NavigationHandleImpl*>(navigation_handle()); Why do you need a static cast here? GetRenderFrameHost() is on the public interface. https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... content/browser/frame_host/ancestor_throttle.cc:114: static_cast<NavigationHandleImpl*>(navigation_handle()); Same as above, no need for casting. https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... File content/browser/frame_host/ancestor_throttle.h (right): https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... content/browser/frame_host/ancestor_throttle.h:54: std::string* failed_parse); nit: failed_parsing? https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl.cc:350: case NavigationThrottle::BLOCK_RESPONSE: Why did we lose the default? https://codereview.chromium.org/1617043002/diff/1/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/1617043002/diff/1/content/content_browser.gyp... content/content_browser.gypi:750: 'browser/frame_host/ancestor_throttle.cc', Alphabetical ordering. https://codereview.chromium.org/1617043002/diff/1/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/1617043002/diff/1/content/content_tests.gypi#... content/content_tests.gypi:458: 'browser/frame_host/ancestor_throttle_unittest.cc', Alphabetical ordering.
On 2016/02/12 23:21:41, nasko wrote: > Few comments, mostly nits. > > https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... > File content/browser/frame_host/ancestor_throttle.cc (right): > > https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... > content/browser/frame_host/ancestor_throttle.cc:76: return > NavigationThrottle::PROCEED; > Shouldn't we be failing closed? > > https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... > content/browser/frame_host/ancestor_throttle.cc:98: > static_cast<NavigationHandleImpl*>(navigation_handle()); > Why do you need a static cast here? GetRenderFrameHost() is on the public > interface. > > https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... > content/browser/frame_host/ancestor_throttle.cc:114: > static_cast<NavigationHandleImpl*>(navigation_handle()); > Same as above, no need for casting. > > https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... > File content/browser/frame_host/ancestor_throttle.h (right): > > https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... > content/browser/frame_host/ancestor_throttle.h:54: std::string* failed_parse); > nit: failed_parsing? > > https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... > File content/browser/frame_host/navigation_handle_impl.cc (right): > > https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... > content/browser/frame_host/navigation_handle_impl.cc:350: case > NavigationThrottle::BLOCK_RESPONSE: > Why did we lose the default? > > https://codereview.chromium.org/1617043002/diff/1/content/content_browser.gypi > File content/content_browser.gypi (right): > > https://codereview.chromium.org/1617043002/diff/1/content/content_browser.gyp... > content/content_browser.gypi:750: 'browser/frame_host/ancestor_throttle.cc', > Alphabetical ordering. > > https://codereview.chromium.org/1617043002/diff/1/content/content_tests.gypi > File content/content_tests.gypi (right): > > https://codereview.chromium.org/1617043002/diff/1/content/content_tests.gypi#... > content/content_tests.gypi:458: > 'browser/frame_host/ancestor_throttle_unittest.cc', > Alphabetical ordering. ping!
@mkwst: ping. Are you still planning on continuing with that CL? Note that I'm introducing support in PlzNavigate for NavigationThrottle::WillProcessResponse in https://codereview.chromium.org/1811913003/.
On 2016/03/17 at 16:48:25, clamy wrote: > @mkwst: ping. Are you still planning on continuing with that CL? Note that I'm introducing support in PlzNavigate for NavigationThrottle::WillProcessResponse in https://codereview.chromium.org/1811913003/. This has been on the back burner for a while now. I'll make sure to clean it up for M51.
Description was changed from ========== Introduce AncestorThrottle, which will process 'X-Frame-Options' headers. This moves the ancestor-based blocking behavior from Blink up into the browser, and depends on https://codereview.chromium.org/1616943003 for some infrastructure changes. BUG=555418 ========== to ========== Introduce AncestorThrottle, which will process 'X-Frame-Options' headers. This moves the ancestor-based blocking behavior from Blink up into the browser, and depends on https://codereview.chromium.org/1616943003 for some infrastructure changes. BUG=555418 ==========
ellyjones@chromium.org changed reviewers: - ellyjones@chromium.org
Description was changed from ========== Introduce AncestorThrottle, which will process 'X-Frame-Options' headers. This moves the ancestor-based blocking behavior from Blink up into the browser, and depends on https://codereview.chromium.org/1616943003 for some infrastructure changes. BUG=555418 ========== to ========== Introduce AncestorThrottle, which will process 'X-Frame-Options' headers. This moves the ancestor-based blocking behavior from Blink up into the browser, and depends on https://codereview.chromium.org/1616943003 for some infrastructure changes. BUG=555418 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Picking this back up again. We need to teach the throttle to deal in some way with the `frame-ancestor` CSP directive, as we only process `X-Frame-Options` if that directive isn't present. I'll try to find some baby steps towards that that don't involve hoisting all of CSP up to the browser process. :/ https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... File content/browser/frame_host/ancestor_throttle.cc (right): https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... content/browser/frame_host/ancestor_throttle.cc:76: return NavigationThrottle::PROCEED; On 2016/02/12 at 23:21:40, nasko (very slow Apr 8-18) wrote: > Shouldn't we be failing closed? Sure. https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... content/browser/frame_host/ancestor_throttle.cc:98: static_cast<NavigationHandleImpl*>(navigation_handle()); On 2016/02/12 at 23:21:40, nasko (very slow Apr 8-18) wrote: > Why do you need a static cast here? GetRenderFrameHost() is on the public interface. Done. https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... content/browser/frame_host/ancestor_throttle.cc:114: static_cast<NavigationHandleImpl*>(navigation_handle()); On 2016/02/12 at 23:21:40, nasko (very slow Apr 8-18) wrote: > Same as above, no need for casting. Done. https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... File content/browser/frame_host/ancestor_throttle.h (right): https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... content/browser/frame_host/ancestor_throttle.h:54: std::string* failed_parse); On 2016/02/12 at 23:21:40, nasko (very slow Apr 8-18) wrote: > nit: failed_parsing? Renamed to match implementation. https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl.cc:350: case NavigationThrottle::BLOCK_RESPONSE: On 2016/02/12 at 23:21:40, nasko (very slow Apr 8-18) wrote: > Why did we lose the default? Because this completely enumerates all the options of `NavigationThrottle::ThrottleCheckResult`, so the `default:` will never be hit, and the compiler will tell us when we need to add a new item? https://codereview.chromium.org/1617043002/diff/1/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/1617043002/diff/1/content/content_browser.gyp... content/content_browser.gypi:750: 'browser/frame_host/ancestor_throttle.cc', On 2016/02/12 at 23:21:40, nasko (very slow Apr 8-18) wrote: > Alphabetical ordering. Done. https://codereview.chromium.org/1617043002/diff/1/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/1617043002/diff/1/content/content_tests.gypi#... content/content_tests.gypi:458: 'browser/frame_host/ancestor_throttle_unittest.cc', On 2016/02/12 at 23:21:40, nasko (very slow Apr 8-18) wrote: > Alphabetical ordering. Done.
Some more nits/questions. https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... File content/browser/frame_host/ancestor_throttle.h (right): https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... content/browser/frame_host/ancestor_throttle.h:5: #ifndef CHROME_BROWSER_SECURITY_ANCESTOR_THROTTLE_H_ Mismatched include guard and file path/name. https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... File content/browser/frame_host/ancestor_throttle_unittest.cc (right): https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... content/browser/frame_host/ancestor_throttle_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016? :) https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (left): https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:404: default: Why remove the default case? https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:435: default: Same as above, why remove it? https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:164: CHECK(state_ >= WILL_PROCESS_RESPONSE) Would we know the RFH at this point?
More issues with this patch: 1. We're not triggering `onload` _or_ `onerror` events on the iframe in Blink, which isn't great. 2. Devtools doesn't display any detail about the response. :( https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... File content/browser/frame_host/ancestor_throttle.h (right): https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... content/browser/frame_host/ancestor_throttle.h:5: #ifndef CHROME_BROWSER_SECURITY_ANCESTOR_THROTTLE_H_ On 2016/04/12 at 22:21:19, nasko (very slow Apr 8-18) wrote: > Mismatched include guard and file path/name. Done, thanks! https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... File content/browser/frame_host/ancestor_throttle_unittest.cc (right): https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... content/browser/frame_host/ancestor_throttle_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/04/12 at 22:21:19, nasko (very slow Apr 8-18) wrote: > 2016? :) 2015! 2015 FOREVER! https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (left): https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:404: default: On 2016/04/12 at 22:21:19, nasko (very slow Apr 8-18) wrote: > Why remove the default case? Because we completely enumerate the cases, and by doing so we ensure that the compiler will tell us if we missed one? https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:435: default: On 2016/04/12 at 22:21:19, nasko (very slow Apr 8-18) wrote: > Same as above, why remove it? Same as above: we completely enumerate the cases, and by doing so we ensure that the compiler will tell us if we missed one? https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:164: CHECK(state_ >= WILL_PROCESS_RESPONSE) On 2016/04/12 at 22:21:19, nasko (very slow Apr 8-18) wrote: > Would we know the RFH at this point? We set it at the top of `NavigationHandleImpl::WillProcessResponse`, so I think so? https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... I may well be missing something, though.
Description was changed from ========== Introduce AncestorThrottle, which will process 'X-Frame-Options' headers. This moves the ancestor-based blocking behavior from Blink up into the browser, and depends on https://codereview.chromium.org/1616943003 for some infrastructure changes. BUG=555418 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Introduce AncestorThrottle, which will process 'X-Frame-Options' headers. This moves the ancestor-based blocking behavior from Blink up into the browser, and depends on https://codereview.chromium.org/1616943003 for some infrastructure changes. BUG=555418 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
On 2016/04/13 13:28:08, Mike West wrote: > 2. Devtools doesn't display any detail about the response. Are you referring to layout tests not showing the console output? I spent quite some time investigating this and I found that: navigation_handle()->GetRenderFrameHost()->GetParent()->AddMessageToConsole(...); will not work because the DevTools agent is not set in layout tests. So in RenderFrameImpl::AddMessageToConsole the message is silently dropped. This was surprising because if one just runs Chrome the messages do appear. Also, IDK how important this is, even if this message would go through it wouldn't use the correct "category" (aka MessageSource from ConsoleTypes.h). So locally I created a new method/IPC to deal exclusively with security console logs (MessageSource::SecurityMessageSource). I'll split that part up and upload it to a new CL then notify you.
On 2016/04/13 at 14:58:16, carlosk wrote: > On 2016/04/13 13:28:08, Mike West wrote: > > 2. Devtools doesn't display any detail about the response. > > Are you referring to layout tests not showing the console output? I spent quite some time investigating this and I found that: No, I was talking about headers and etc. in the network tab. > navigation_handle()->GetRenderFrameHost()->GetParent()->AddMessageToConsole(...); > > will not work because the DevTools agent is not set in layout tests. So in RenderFrameImpl::AddMessageToConsole the message is silently dropped. This was surprising because if one just runs Chrome the messages do appear. Hrm. It would be nice if we had a clear way to add messages to the console of some document somewhere. I guess we need to teach content_shell about the devtools agent? I mean, layout tests do have a console, so I hope it's just a matter of wiring things up to each other. > Also, IDK how important this is, even if this message would go through it wouldn't use the correct "category" (aka MessageSource from ConsoleTypes.h). So locally I created a new method/IPC to deal exclusively with security console logs (MessageSource::SecurityMessageSource). I'll split that part up and upload it to a new CL then notify you. The category isn't super important, as I don't think it's actually exposed to users anywhere in devtools. pfeldman@ might disagree, though. :) Happy to look at a CL, though. In fact, if you're working on this too, I'm _happy_ to just let you do the whole thing. ;)
On 2016/04/13 17:30:12, Mike West wrote: > On 2016/04/13 at 14:58:16, carlosk wrote: > > On 2016/04/13 13:28:08, Mike West wrote: > > > 2. Devtools doesn't display any detail about the response. > > > > Are you referring to layout tests not showing the console output? I spent > quite some time investigating this and I found that: > > No, I was talking about headers and etc. in the network tab. Ack. > navigation_handle()->GetRenderFrameHost()->GetParent()->AddMessageToConsole(...); > > > > will not work because the DevTools agent is not set in layout tests. So in > RenderFrameImpl::AddMessageToConsole the message is silently dropped. This was > surprising because if one just runs Chrome the messages do appear. > > Hrm. It would be nice if we had a clear way to add messages to the console of > some document somewhere. I guess we need to teach content_shell about the > devtools agent? I mean, layout tests do have a console, so I hope it's just a > matter of wiring things up to each other. Finally it is. But the wiring was complicated for someone looking at it for the 1st time. > > Also, IDK how important this is, even if this message would go through it > wouldn't use the correct "category" (aka MessageSource from ConsoleTypes.h). So > locally I created a new method/IPC to deal exclusively with security console > logs (MessageSource::SecurityMessageSource). I'll split that part up and upload > it to a new CL then notify you. > > The category isn't super important, as I don't think it's actually exposed to > users anywhere in devtools. pfeldman@ might disagree, though. :) > > Happy to look at a CL, though. https://codereview.chromium.org/1890513004/ > In fact, if you're working on this too, I'm _happy_ to just let you do the whole > thing. ;) If you can move forward with this it would be great from a PlzNavigate perspective: we're only two SWEs. :) Otherwise, I'll pick it up when I'm done with MIX checks.
Worked out one set of issues, and added rudimentary `frame-ancestors` handling. Let's see how it looks on the bots. https://codereview.chromium.org/1617043002/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.cc (left): https://codereview.chromium.org/1617043002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:5767: return false; Returning false here for layout tests means that blink hangs, as nothing ever loads in the frame, and therefore no `load` event is fired. :( https://codereview.chromium.org/1617043002/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1617043002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:2174: !RenderThreadImpl::current()->layout_test_mode()) { It's super-strange that we're checking whether we're in layout test mode here. Why was the original code doing this check? What tests will fail based on this change? The world may never know (until the bots come back with results)!
On 2016/04/25 at 13:06:21, Mike West (slow until 25th) wrote: > https://codereview.chromium.org/1617043002/diff/80001/content/renderer/render... > content/renderer/render_frame_impl.cc:2174: !RenderThreadImpl::current()->layout_test_mode()) { > It's super-strange that we're checking whether we're in layout test mode here. Why was the original code doing this check? What tests will fail based on this change? The world may never know (until the bots come back with results)! Looks like there are a few texts that depend on the current behavior (which I think we need to change regardless of this patch, as it actually changes rendering in layout tests in bad ways): moving that work out to https://codereview.chromium.org/1920873002. Beyond that, this isn't looking terrible. There's one inspector test I need to address, and one other test that might be related, but I think this is ready to skim in a bit more detail. clamy@, nasko@, mind taking a look?
On 2016/04/25 at 17:16:04, Mike West (slow until 25th) wrote: > Looks like there are a few texts that depend on the current behavior (which I think we need to change regardless of this patch, as it actually changes rendering in layout tests in bad ways): moving that work out to https://codereview.chromium.org/1920873002. Landed this a minute ago, after fighting with more tests than I expected to have to fight with. > Beyond that, this isn't looking terrible. There's one inspector test I need to address, and one other test that might be related, but I think this is ready to skim in a bit more detail. clamy@, nasko@, mind taking a look? Friendly ping?
I think the content/ side is in a good shape. I need to nitpick a bit more on the parsing parts, but the overall structure and the rest of the code looks ready with a few comments addressed. I can't really comment intelligently on the Blink parts. https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... File content/browser/frame_host/ancestor_throttle.h (right): https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... content/browser/frame_host/ancestor_throttle.h:5: #ifndef CHROME_BROWSER_SECURITY_ANCESTOR_THROTTLE_H_ On 2016/04/13 13:28:08, Mike West (slow until 25th) wrote: > On 2016/04/12 at 22:21:19, nasko (very slow Apr 8-18) wrote: > > Mismatched include guard and file path/name. > > Done, thanks! Hmm, I must be getting really old. I don't see this being updated in PS6. https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... File content/browser/frame_host/ancestor_throttle_unittest.cc (right): https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... content/browser/frame_host/ancestor_throttle_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/04/13 13:28:08, Mike West (slow until 25th) wrote: > On 2016/04/12 at 22:21:19, nasko (very slow Apr 8-18) wrote: > > 2016? :) > > 2015! 2015 FOREVER! I understand you really like it, but let's ensure we stamp new files with 2016 ;). Let's not live in the past ;). https://codereview.chromium.org/1617043002/diff/120001/content/browser/frame_... File content/browser/frame_host/ancestor_throttle.cc (right): https://codereview.chromium.org/1617043002/diff/120001/content/browser/frame_... content/browser/frame_host/ancestor_throttle.cc:101: navigation_handle()->GetRenderFrameHost()->GetParent()->AddMessageToConsole( As it is, this can result in a crash, since we register the throttle even for main frames, which will return nullptr from GetParent(). https://codereview.chromium.org/1617043002/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1617043002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:288: throttles_.push_back(std::move(ancestor_throttle)); Should we register this throttle only in the case of subframes? No need to check main frames, right?
Thanks, Nasko! https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... File content/browser/frame_host/ancestor_throttle.h (right): https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... content/browser/frame_host/ancestor_throttle.h:5: #ifndef CHROME_BROWSER_SECURITY_ANCESTOR_THROTTLE_H_ On 2016/04/29 at 18:56:02, nasko (slow) wrote: > On 2016/04/13 13:28:08, Mike West (slow until 25th) wrote: > > On 2016/04/12 at 22:21:19, nasko (very slow Apr 8-18) wrote: > > > Mismatched include guard and file path/name. > > > > Done, thanks! > > Hmm, I must be getting really old. I don't see this being updated in PS6. Yeah. Sorry. I'm old too and forgot. :/ https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... File content/browser/frame_host/ancestor_throttle_unittest.cc (right): https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_h... content/browser/frame_host/ancestor_throttle_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/04/29 at 18:56:02, nasko (slow) wrote: > On 2016/04/13 13:28:08, Mike West (slow until 25th) wrote: > > On 2016/04/12 at 22:21:19, nasko (very slow Apr 8-18) wrote: > > > 2016? :) > > > > 2015! 2015 FOREVER! > > I understand you really like it, but let's ensure we stamp new files with 2016 ;). Let's not live in the past ;). *sigh* FINE. https://codereview.chromium.org/1617043002/diff/120001/content/browser/frame_... File content/browser/frame_host/ancestor_throttle.cc (right): https://codereview.chromium.org/1617043002/diff/120001/content/browser/frame_... content/browser/frame_host/ancestor_throttle.cc:101: navigation_handle()->GetRenderFrameHost()->GetParent()->AddMessageToConsole( On 2016/04/29 at 18:56:02, nasko (slow) wrote: > As it is, this can result in a crash, since we register the throttle even for main frames, which will return nullptr from GetParent(). We don't register the throttle for main frames, as discussed below. I can add a `DCHECK(navigation_handle()->GetRenderFrameHost()->GetParent())` if that's helpful? https://codereview.chromium.org/1617043002/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1617043002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:288: throttles_.push_back(std::move(ancestor_throttle)); On 2016/04/29 at 18:56:02, nasko (slow) wrote: > Should we register this throttle only in the case of subframes? No need to check main frames, right? It's a great idea! In fact, it's exactly what this patch already does, as `MaybeCreateThrottleFor` will only create a throttle if `!handle->IsInMainFrame()`. :)
Patchset #7 (id:140001) has been deleted
Patchset #8 (id:180001) has been deleted
content/ LGTM, assuming the include guard actually says CONTENT_ ;). https://codereview.chromium.org/1617043002/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1617043002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:288: throttles_.push_back(std::move(ancestor_throttle)); On 2016/05/02 09:37:52, Mike West wrote: > On 2016/04/29 at 18:56:02, nasko (slow) wrote: > > Should we register this throttle only in the case of subframes? No need to > check main frames, right? > > It's a great idea! In fact, it's exactly what this patch already does, as > `MaybeCreateThrottleFor` will only create a throttle if > `!handle->IsInMainFrame()`. :) Ah, I knew I saw it once and missed it on the last round. Thanks! https://codereview.chromium.org/1617043002/diff/200001/content/browser/frame_... File content/browser/frame_host/ancestor_throttle.h (right): https://codereview.chromium.org/1617043002/diff/200001/content/browser/frame_... content/browser/frame_host/ancestor_throttle.h:5: #ifndef CHROME_BROWSER_FRAME_HOST_ANCESTOR_THROTTLE_H_ This code lives in content/ now ;).
mkwst@chromium.org changed reviewers: + isherman@chromium.org, jochen@chromium.org, mmenke@chromium.org - clamy@chromium.org, davidben@chromium.org
Thanks, Nasko! I'll fix the `CONTENT` bit. jochen@: Would you take a look at the Blink bits? I'll need to circle back with the devtools folks to figure out a reasonable way of actually fixing `inspector/network/x-frame-options-deny.html`; until I do the devtools presentation of the failed load won't have the response headers, which is pretty annoying. I don't think we should ship it that way, but I also don't think it'll be a problem to get a fix into M5whatever we're on. mmenke@: Can you evaluate the error code changes that touch components/error_page and net/base? I hope I'm doing things correctly there. isherman@: Assuming that mmenke@ is happy with the error code, can you stamp the change to histograms.xml?
mmenke@chromium.org changed reviewers: + edwardjung@chromium.org
[+edwardjung]: Mind reviewing the error strings this adds? https://codereview.chromium.org/1617043002/diff/200001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/1617043002/diff/200001/net/base/net_error_lis... net/base/net_error_list.h:112: // checks, for instance. Missing a close paren.
https://codereview.chromium.org/1617043002/diff/200001/content/browser/frame_... File content/browser/frame_host/ancestor_throttle.cc (right): https://codereview.chromium.org/1617043002/diff/200001/content/browser/frame_... content/browser/frame_host/ancestor_throttle.cc:129: while (headers->EnumerateHeader(&iter, "x-frame-options", &value)) { Is there a w3c doc we could link for this, too? https://codereview.chromium.org/1617043002/diff/200001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1617043002/diff/200001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:175: CHECK(state_ >= WILL_PROCESS_RESPONSE) nit: While you're here, CHECK_GE https://codereview.chromium.org/1617043002/diff/200001/content/browser/loader... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1617043002/diff/200001/content/browser/loader... content/browser/loader/navigation_resource_throttle.cc:239: controller()->CancelWithError(net::ERR_BLOCKED_BY_RESPONSE); Is this only for subframes, or is it for the main frame, too? Note that when we fail with any error except for ERR_ABORTED, we commit the error page. For main frames only, if the page was not generated by a POST, we try to automatically reload the page, which I assume will lose whatever context this caused us to error out the page. For POSTs, F5 will show a re-post confirmation dialog and then allow users to actually load the page. Even without auto-reload, the user can reload a page manually, and session restore will do the same.
histograms.xml rs lgtm
blink lgtm assuming devtools is happy with temporarily breaking this
https://codereview.chromium.org/1617043002/diff/240001/components/error_page_... File components/error_page_strings.grdp (right): https://codereview.chromium.org/1617043002/diff/240001/components/error_page_... components/error_page_strings.grdp:459: The page at this URL has requested not to be displayed in this context. Is there a simpler way to put summarise this error? Happy to leave the technical details under 'Details', but for regular users I feel we can simplify it in some way. Can this be described as a security issue due to server restrictions. Does this only appear in frames?
On 2016/05/03 at 14:39:47, edwardjung wrote: > https://codereview.chromium.org/1617043002/diff/240001/components/error_page_... > File components/error_page_strings.grdp (right): > > https://codereview.chromium.org/1617043002/diff/240001/components/error_page_... > components/error_page_strings.grdp:459: The page at this URL has requested not to be displayed in this context. > Is there a simpler way to put summarise this error? Happy to leave the technical details under 'Details', but for regular users I feel we can simplify it in some way. "This page cannot be displayed"? That said, I don't think this string actually appears to users, actually, because of your next question: > Can this be described as a security issue due to server restrictions. Does this only appear in frames? Yes, this only appears in frames.
On 2016/05/03 15:59:15, Mike West wrote: > On 2016/05/03 at 14:39:47, edwardjung wrote: > > > https://codereview.chromium.org/1617043002/diff/240001/components/error_page_... > > File components/error_page_strings.grdp (right): > > > > > https://codereview.chromium.org/1617043002/diff/240001/components/error_page_... > > components/error_page_strings.grdp:459: The page at this URL has requested not > to be displayed in this context. > > Is there a simpler way to put summarise this error? Happy to leave the > technical details under 'Details', but for regular users I feel we can simplify > it in some way. > > "This page cannot be displayed"? > > That said, I don't think this string actually appears to users, actually, > because of your next question: > > > Can this be described as a security issue due to server restrictions. Does > this only appear in frames? > > Yes, this only appears in frames. In frames the summary message is displayed when you hover over the frame so it's still important to get right. Where possible we try to reuse existing strings. IDS_ERRORPAGES_DETAILS_CONNECTION_REFUSED - "The server refused the connection" seems appropriate. I'm checking with rachelis@ (who for future reference is the Chrome UX gatekeeper for the net error strings).
On 2016/05/03 at 16:30:04, edwardjung wrote: > On 2016/05/03 15:59:15, Mike West wrote: > > On 2016/05/03 at 14:39:47, edwardjung wrote: > > > > > https://codereview.chromium.org/1617043002/diff/240001/components/error_page_... > > > File components/error_page_strings.grdp (right): > > > > > > > > https://codereview.chromium.org/1617043002/diff/240001/components/error_page_... > > > components/error_page_strings.grdp:459: The page at this URL has requested not > > to be displayed in this context. > > > Is there a simpler way to put summarise this error? Happy to leave the > > technical details under 'Details', but for regular users I feel we can simplify > > it in some way. > > > > "This page cannot be displayed"? > > > > That said, I don't think this string actually appears to users, actually, > > because of your next question: > > > > > Can this be described as a security issue due to server restrictions. Does > > this only appear in frames? > > > > Yes, this only appears in frames. > > In frames the summary message is displayed when you hover over the frame so it's still important to get right. Hrm. I think only the detail message is displayed on hover. I might be misremembering the quick test I did though, and I'm not in front of a machine where I can play with it. Tomorrow. :) > Where possible we try to reuse existing strings. IDS_ERRORPAGES_DETAILS_CONNECTION_REFUSED - "The server refused the connection" seems appropriate. I'm checking with rachelis@ (who for future reference is the Chrome UX gatekeeper for the net error strings). Yeah, that could work. It's a bit of a different thing, since we're breaking off the connection rather than the server refusing it, but I suppose it ends up being a pretty similar impact on users. (Hi, Rachel!)
> Hrm. I think only the detail message is displayed on hover. I might be > misremembering the quick test I did though, and I'm not in front of a machine > where I can play with it. Tomorrow. :) Aaah, yes it is details. My mistake, a lot of the errors just use the summary for the hover message. In that case we shouldn't introduce a new string for summary and reuse whatever is the appropriate. I've commented on the details string and will check with rachelis. https://codereview.chromium.org/1617043002/diff/260001/components/error_page_... File components/error_page_strings.grdp (right): https://codereview.chromium.org/1617043002/diff/260001/components/error_page_... components/error_page_strings.grdp:475: </message> This seems like a too long a message to show for the rollover state. For small iframes we're relying on the title tooltip of the icon so we shouldn't have markup in the message as this will be displayed as is.
On 2016/05/03 at 18:55:56, edwardjung wrote: > > Hrm. I think only the detail message is displayed on hover. I might be > > misremembering the quick test I did though, and I'm not in front of a machine > > where I can play with it. Tomorrow. :) > > Aaah, yes it is details. My mistake, a lot of the errors just use the summary for the hover message. > In that case we shouldn't introduce a new string for summary and reuse whatever is the appropriate. > > I've commented on the details string and will check with rachelis. > > https://codereview.chromium.org/1617043002/diff/260001/components/error_page_... > File components/error_page_strings.grdp (right): > > https://codereview.chromium.org/1617043002/diff/260001/components/error_page_... > components/error_page_strings.grdp:475: </message> > This seems like a too long a message to show for the rollover state. For small iframes we're relying on the title tooltip of the icon so we shouldn't have markup in the message as this will be displayed as is. I've updated the CL with the "connection refused" text. WDYT?
> https://codereview.chromium.org/1617043002/diff/260001/components/error_page_... > > components/error_page_strings.grdp:475: </message> > > This seems like a too long a message to show for the rollover state. For small > iframes we're relying on the title tooltip of the icon so we shouldn't have > markup in the message as this will be displayed as is. > > I've updated the CL with the "connection refused" text. WDYT? That works, localized_error.cc LGTM
Thanks! I think //net/base is the only thing left; mmenke@, WDYT? https://codereview.chromium.org/1617043002/diff/200001/content/browser/frame_... File content/browser/frame_host/ancestor_throttle.cc (right): https://codereview.chromium.org/1617043002/diff/200001/content/browser/frame_... content/browser/frame_host/ancestor_throttle.cc:129: while (headers->EnumerateHeader(&iter, "x-frame-options", &value)) { On 2016/05/02 at 18:17:28, mmenke wrote: > Is there a w3c doc we could link for this, too? Sure! Linked to RFC7034. https://codereview.chromium.org/1617043002/diff/200001/content/browser/frame_... File content/browser/frame_host/ancestor_throttle.h (right): https://codereview.chromium.org/1617043002/diff/200001/content/browser/frame_... content/browser/frame_host/ancestor_throttle.h:5: #ifndef CHROME_BROWSER_FRAME_HOST_ANCESTOR_THROTTLE_H_ On 2016/05/02 at 17:22:09, nasko (slow) wrote: > This code lives in content/ now ;). Yes, sorry. Typo. :( https://codereview.chromium.org/1617043002/diff/200001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1617043002/diff/200001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:175: CHECK(state_ >= WILL_PROCESS_RESPONSE) On 2016/05/02 at 18:17:28, mmenke wrote: > nit: While you're here, CHECK_GE Done. https://codereview.chromium.org/1617043002/diff/200001/content/browser/loader... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1617043002/diff/200001/content/browser/loader... content/browser/loader/navigation_resource_throttle.cc:239: controller()->CancelWithError(net::ERR_BLOCKED_BY_RESPONSE); On 2016/05/02 at 18:17:29, mmenke wrote: > Is this only for subframes, or is it for the main frame, too? Note that when we fail with any error except for ERR_ABORTED, we commit the error page. The throttle introduced in this CL is only for subframes. In theory, throttles could be added which would cancel the main frame as well. I'll add a comment here noting the special considerations for main frame cancellations; if we add that kind of functionality, we'll need to carefully consider what the right behavior ought to be. https://codereview.chromium.org/1617043002/diff/200001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/1617043002/diff/200001/net/base/net_error_lis... net/base/net_error_list.h:112: // checks, for instance. On 2016/05/02 at 18:08:15, mmenke wrote: > Missing a close paren. The nittiest of nits. :P
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1617043002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617043002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/1617043002/diff/280001/content/browser/loader... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1617043002/diff/280001/content/browser/loader... content/browser/loader/navigation_resource_throttle.cc:245: // consider the right thing to do here. Can we add a DCHECK here, too (Grab the ResourceRequestInfo::ForRequest, and DCHECK it's not a main frame), just to make sure if that changes, someone thinks about the issue? https://codereview.chromium.org/1617043002/diff/280001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/1617043002/diff/280001/net/base/net_error_lis... net/base/net_error_list.h:112: // checks, for instance). On 2016/05/05 08:14:28, Mike West (OOO until the 6th) wrote: > On 2016/05/02 at 18:08:15, mmenke wrote: > > Missing a close paren. > > The nittiest of nits. :P I take that as a challenge to be nittier! Technically, close parens go after other punctuation, like periods. :)
The CQ bit was checked by mkwst@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, isherman@chromium.org, nasko@chromium.org, edwardjung@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1617043002/#ps300001 (title: "DCHECK.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1617043002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617043002/300001
https://codereview.chromium.org/1617043002/diff/280001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/1617043002/diff/280001/net/base/net_error_lis... net/base/net_error_list.h:112: // checks, for instance). On 2016/05/05 at 12:36:27, mmenke wrote: > On 2016/05/05 08:14:28, Mike West (OOO until the 6th) wrote: > > On 2016/05/02 at 18:08:15, mmenke wrote: > > > Missing a close paren. > > > > The nittiest of nits. :P > > I take that as a challenge to be nittier! ;) > Technically, close parens go after other punctuation, like periods. :) Even if the parenthetical is a dependent clause in the larger sentence? Adjusting this sentence as you suggest would end up with something like "Blah blah (blah blah.).", right?
Message was sent while issue was closed.
Description was changed from ========== Introduce AncestorThrottle, which will process 'X-Frame-Options' headers. This moves the ancestor-based blocking behavior from Blink up into the browser, and depends on https://codereview.chromium.org/1616943003 for some infrastructure changes. BUG=555418 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Introduce AncestorThrottle, which will process 'X-Frame-Options' headers. This moves the ancestor-based blocking behavior from Blink up into the browser, and depends on https://codereview.chromium.org/1616943003 for some infrastructure changes. BUG=555418 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #13 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Introduce AncestorThrottle, which will process 'X-Frame-Options' headers. This moves the ancestor-based blocking behavior from Blink up into the browser, and depends on https://codereview.chromium.org/1616943003 for some infrastructure changes. BUG=555418 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Introduce AncestorThrottle, which will process 'X-Frame-Options' headers. This moves the ancestor-based blocking behavior from Blink up into the browser, and depends on https://codereview.chromium.org/1616943003 for some infrastructure changes. BUG=555418 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/26a6fc92ae361b4271f8f2197abe7eb063fc43ed Cr-Commit-Position: refs/heads/master@{#392032} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/26a6fc92ae361b4271f8f2197abe7eb063fc43ed Cr-Commit-Position: refs/heads/master@{#392032}
Message was sent while issue was closed.
carlosk@chromium.org changed reviewers: + carlosk@chromium.org
Message was sent while issue was closed.
Sorry about the post-mortem comment but I only re-looked at this change now. https://codereview.chromium.org/1617043002/diff/300001/content/public/browser... File content/public/browser/navigation_throttle.h (right): https://codereview.chromium.org/1617043002/diff/300001/content/public/browser... content/public/browser/navigation_throttle.h:38: // be returned from WillProcessResponse. Should also mention that BLOCK_RESPONSE should only be used for subframes (for now at least, as stated in navigation_resource_throttle.cc). |
