Introduce AncestorThrottle, which will process 'X-Frame-Options' headers.

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}

[Mike West, 2016-01-21 14:52:01]

mkwst@chromium.org changed reviewers:
+ clamy@chromium.org, davidben@chromium.org, ellyjones@chromium.org,
nasko@chromium.org

[Mike West, 2016-01-21 14:52:01]

CCing folks to whom I'd asked questions on the previous CL. :)

[Mike West, 2016-01-22 07:38:04]

Patchset #2 (id:20001) has been deleted

[Mike West, 2016-01-22 12:55:11]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-22 12:55:21]

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

[commit-bot: I haz the power, 2016-01-22 13:23:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-22 13:23:28]

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_...)

[nasko, 2016-02-12 23:21:41]

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.

[nasko, 2016-02-26 18:19:14]

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!

[clamy, 2016-03-17 16:48:25]

@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/.

[Mike West, 2016-03-18 07:32:43]

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.

[Elly Fong-Jones, 2016-04-07 21:03:23]

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
==========

[Elly Fong-Jones, 2016-04-07 21:03:23]

ellyjones@chromium.org changed reviewers:
- ellyjones@chromium.org

[Mike West, 2016-04-12 10:39:53]

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
==========

[Mike West, 2016-04-12 20:13:30]

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.

[nasko, 2016-04-12 22:21:19]

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?

[Mike West, 2016-04-13 13:28:08]

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.

[carlosk, 2016-04-13 14:39:02]

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
==========

[carlosk, 2016-04-13 14:58:16]

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.

[Mike West, 2016-04-13 17:30:12]

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. ;)

[carlosk, 2016-04-18 12:15:34]

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.

[Mike West, 2016-04-25 13:06:21]

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)!

[Mike West, 2016-04-25 17:16:04]

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?

[Mike West, 2016-04-29 12:35:25]

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?

[nasko, 2016-04-29 18:56:02]

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?

[Mike West, 2016-05-02 09:37:52]

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()`. :)

[Mike West, 2016-05-02 09:59:46]

Patchset #7 (id:140001) has been deleted

[Mike West, 2016-05-02 14:17:29]

Patchset #8 (id:180001) has been deleted

[nasko, 2016-05-02 17:22:09]

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 ;).

[Mike West, 2016-05-02 18:02:22]

mkwst@chromium.org changed reviewers:
+ isherman@chromium.org, jochen@chromium.org, mmenke@chromium.org
- clamy@chromium.org, davidben@chromium.org

[Mike West, 2016-05-02 18:02:23]

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, 2016-05-02 18:08:14]

mmenke@chromium.org changed reviewers:
+ edwardjung@chromium.org

[mmenke, 2016-05-02 18:08:15]

[+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.

[mmenke, 2016-05-02 18:17:29]

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.

[Ilya Sherman, 2016-05-02 18:36:38]

histograms.xml rs lgtm

[jochen (gone - plz use gerrit), 2016-05-03 10:59:22]

blink lgtm assuming devtools is happy with temporarily breaking this

[edwardjung, 2016-05-03 14:39:47]

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?

[Mike West, 2016-05-03 15:59:15]

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.

[edwardjung, 2016-05-03 16:30:04]

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).

[Mike West, 2016-05-03 18:13:34]

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!)

[edwardjung, 2016-05-03 18:55:56]

> 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.

[Mike West, 2016-05-04 15:57:47]

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?

[edwardjung, 2016-05-04 21:23:41]

>
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

[Mike West, 2016-05-05 08:14:28]

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

[Mike West, 2016-05-05 09:47:35]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-05 09:47:50]

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

[commit-bot: I haz the power, 2016-05-05 11:23:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-05 11:23:36]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-05-05 12:36:27]

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.  :)

[Mike West, 2016-05-06 09:36:03]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-05-06 09:36:04]

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.")

[commit-bot: I haz the power, 2016-05-06 09:36:19]

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

[Mike West, 2016-05-06 11:23:24]

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?

[commit-bot: I haz the power, 2016-05-06 11:24:43]

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
==========

[commit-bot: I haz the power, 2016-05-06 11:24:45]

Committed patchset #13 (id:300001)

[commit-bot: I haz the power, 2016-05-06 11:26:16]

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}
==========

[commit-bot: I haz the power, 2016-05-06 11:26:18]

Patchset 13 (id:??) landed as
https://crrev.com/26a6fc92ae361b4271f8f2197abe7eb063fc43ed
Cr-Commit-Position: refs/heads/master@{#392032}

[carlosk, 2016-05-10 14:05:35]

carlosk@chromium.org changed reviewers:
+ carlosk@chromium.org

[carlosk, 2016-05-10 14:05:36]

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).