(Re-)introduce AncestorThrottle to handle 'X-Frame-Options'.

(Re-)introduce AncestorThrottle to handle 'X-Frame-Options'.

This patch moves ancestor-based blocking behavior up from Blink into the
browser, which gives us some correctness assurances we can't match in
the renderer (for top-level plugins, etc), and ensures that a (malicious?)
renderer isn't responsible for policing itself.

This is a re-land of https://codereview.chromium.org/1617043002, after
clamy@'s heroic refactoring effort to process 'Content-Disposition' and
similar download-triggering mechanisms before handing things off to
this new throttle.

BUG=555418
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel

Committed: https://crrev.com/c5a2f126d6e96ef139e4211ff225169b810490f4
Cr-Commit-Position: refs/heads/master@{#440055}

[arthursonzogni, 2016-11-09 12:27:17]

Description was changed from

==========
Apply patch (CL=2321503002)
==========

to

==========
Apply patch (CL=2321503002)
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[arthursonzogni, 2016-11-09 12:34:51]

Description was changed from

==========
Apply patch (CL=2321503002)
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
A suggestion to make progress on:
https://codereview.chromium.org/2321503002/

It makes Handling BLOCK_RESPONSE on NavigationRequest(PlzNavigate) and there is
some quick fix.
==========

[arthursonzogni, 2016-11-09 12:35:58]

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

[commit-bot: I haz the power, 2016-11-09 12:36:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 12:49:03]

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

[commit-bot: I haz the power, 2016-11-09 12:49:04]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[arthursonzogni, 2016-11-09 15:21:02]

Description was changed from

==========
A suggestion to make progress on:
https://codereview.chromium.org/2321503002/

It makes Handling BLOCK_RESPONSE on NavigationRequest(PlzNavigate) and there is
some quick fix.
==========

to

==========
A suggestion to make progress on:
https://codereview.chromium.org/2321503002/

It makes Handling BLOCK_RESPONSE on NavigationRequest(PlzNavigate) and there is
some quick fix.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[arthursonzogni, 2016-11-09 15:22:21]

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

[commit-bot: I haz the power, 2016-11-09 15:22:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 16:56:53]

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

[commit-bot: I haz the power, 2016-11-09 16:56:54]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2016-11-22 16:01:50]

Description was changed from

==========
A suggestion to make progress on:
https://codereview.chromium.org/2321503002/

It makes Handling BLOCK_RESPONSE on NavigationRequest(PlzNavigate) and there is
some quick fix.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
(Re-)introduce AncestorThrottle to handle 'X-Frame-Options'.

This patch moves ancestor-based blocking behavior up from Blink into the
browser, which gives us some correctness assurances we can't match in
the renderer (for top-level plugins, etc), and ensures that a (malicious?)
renderer isn't responsible for policing itself.

This is a re-land of https://codereview.chromium.org/1617043002, after
clamy@'s heroic refactoring effort to process 'Content-Disposition' and
similar download-triggering mechanisms before handing things off to
this new throttle.

BUG=555418
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[arthursonzogni, 2016-11-22 16:03:07]

Description was changed from

==========
(Re-)introduce AncestorThrottle to handle 'X-Frame-Options'.

This patch moves ancestor-based blocking behavior up from Blink into the
browser, which gives us some correctness assurances we can't match in
the renderer (for top-level plugins, etc), and ensures that a (malicious?)
renderer isn't responsible for policing itself.

This is a re-land of https://codereview.chromium.org/1617043002, after
clamy@'s heroic refactoring effort to process 'Content-Disposition' and
similar download-triggering mechanisms before handing things off to
this new throttle.

BUG=555418
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
(Re-)introduce AncestorThrottle to handle 'X-Frame-Options'.

This patch moves ancestor-based blocking behavior up from Blink into the
browser, which gives us some correctness assurances we can't match in
the renderer (for top-level plugins, etc), and ensures that a (malicious?)
renderer isn't responsible for policing itself.

This is a re-land of https://codereview.chromium.org/1617043002, after
clamy@'s heroic refactoring effort to process 'Content-Disposition' and
similar download-triggering mechanisms before handing things off to
this new throttle.

BUG=555418
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel
==========

[arthursonzogni, 2016-11-22 16:03:30]

Patchset #4 (id:60001) has been deleted

[arthursonzogni, 2016-11-23 15:12:48]

Patchset #5 (id:100001) has been deleted

[arthursonzogni, 2016-11-23 15:17:25]

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

[commit-bot: I haz the power, 2016-11-23 15:17:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 17:06:32]

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[arthursonzogni, 2016-11-23 17:38:50]

arthursonzogni@chromium.org changed reviewers:
+ alexmos@chromium.org, clamy@chromium.org, creis@chromium.org

[arthursonzogni, 2016-11-23 17:38:51]

Hi,

I would like to continue the efforts of @mkwst on:
https://codereview.chromium.org/2321503002/

I added 3 things:
* Two very minor bugfix. (See patch set 2)
* Support for PlzNavigate. (Patch set 2 + correction on 5)
* Replace error pages with blank pages. (Patch 5)

N.B. #1: I didn't solve the issue with the error pages and the chrome web store
(crbug.com/622385). Instead of error page, a blank page is displayed. By doing
this the problem is hidden (That is the current behavior). We will be able to
switch to error pages once this problem is fixed.

N.B. #2: Some tests still fail with PlzNavigate, it is because
testRunner.dumpResourceLoadCallbacks() doesn't print the same things with and
without PlzNavigate. But the behavior is the correct one.

Please take a look and tell me what do you think of it.
Thanks!

https://codereview.chromium.org/2488743003/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2488743003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2353: }
Some justifications about this:
* I use "" to load a blank page.
* I use GURL("data:text/html,") such that forward/backward navigation reloads a
blank page and not something else. GURL("about:blank") is not a better
alternative because in that case, the parent frame is allowed to access its
children, which is forbidden.

The behavior is the same as the actual one. Once there is no more problems with
error pages, this block should be removed.

[alexmos, 2016-11-30 01:22:19]

This seems reasonable to me.  Loading a blank page will let us land this without
waiting for error pages to be refactored, and it will keep the current behavior
(and also remain consistent with CSP frame-ancestors blocking, which is also
loading a blank page).

clamy@: can you double-check this, especially PlzNavigate support in
navigation_request.cc and the use of loadData (since I think you've worked on
adding data: URL support for PlzNavigate)?

I think you should run this by mkwst@ also, just to see if he agrees this is a
reasonable step to unblock this.

https://codereview.chromium.org/2488743003/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2488743003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:591: state_ =
CANCELING_RESPONSE;
I know this is the same as in mkwst@'s original CL, but I'm curious why this is
CANCELING_RESPONSE rather than CANCELING_REQUEST?

https://codereview.chromium.org/2488743003/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2488743003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2344: // frame-ancestor don't display
error pages but blank pages instead.
I think this CL only moves XFO to the browser process, but not CSP
frame-ancestors, so let's not mention frame-ancestors here until we move it over
as well :)

https://codereview.chromium.org/2488743003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2345: // See crbug.com/622385 if you want
to remove this.
Let's rephrase this as a TODO to remove this once error pages commit in the
right process.  Feel free to include myself, mkwst, and/or yourself as the
TODO() target. :)  Let's also reference crbug.com/588314 in addition to 622385.

https://codereview.chromium.org/2488743003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2349: GURL("data:text/html,"), WebURL(),
replace,
Existing code uses SecurityOrigin::urlWithUniqueSecurityOrigin(), which is
defined as "data:,".  Perhaps we can reuse that URL here?

https://codereview.chromium.org/2488743003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2353: }
On 2016/11/23 17:38:51, arthursonzogni wrote:
> Some justifications about this:
> * I use "" to load a blank page.
> * I use GURL("data:text/html,") such that forward/backward navigation reloads
a
> blank page and not something else. GURL("about:blank") is not a better
> alternative because in that case, the parent frame is allowed to access its
> children, which is forbidden.
> 
> The behavior is the same as the actual one. Once there is no more problems
with
> error pages, this block should be removed.

Ack.  This seems fairly similar to what is currently done by
DocumentLoader::cancelLoadAfterXFrameOptionsOrCSPDenied.

[arthursonzogni, 2016-11-30 13:29:33]

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

[commit-bot: I haz the power, 2016-11-30 13:29:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[arthursonzogni, 2016-11-30 13:49:54]

arthursonzogni@chromium.org changed reviewers:
+ mkwst@chromium.org

[arthursonzogni, 2016-11-30 13:49:55]

Thanks! I agreed with your suggestions.

@mkwst: please see the comments below.

https://codereview.chromium.org/2488743003/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2488743003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:591: state_ =
CANCELING_RESPONSE;
On 2016/11/30 01:22:19, alexmos wrote:
> I know this is the same as in mkwst@'s original CL, but I'm curious why this
is
> CANCELING_RESPONSE rather than CANCELING_REQUEST?

It looks suspicious to me. I think CANCELING_REQUEST is the right state. @mkwst
could you confirm it please?

https://codereview.chromium.org/2488743003/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2488743003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2344: // frame-ancestor don't display
error pages but blank pages instead.
On 2016/11/30 01:22:19, alexmos wrote:
> I think this CL only moves XFO to the browser process, but not CSP
> frame-ancestors, so let's not mention frame-ancestors here until we move it
over
> as well :)

Done.

https://codereview.chromium.org/2488743003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2345: // See crbug.com/622385 if you want
to remove this.
On 2016/11/30 01:22:19, alexmos wrote:
> Let's rephrase this as a TODO to remove this once error pages commit in the
> right process.  Feel free to include myself, mkwst, and/or yourself as the
> TODO() target. :)  Let's also reference crbug.com/588314 in addition to
622385.

Done.

https://codereview.chromium.org/2488743003/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2349: GURL("data:text/html,"), WebURL(),
replace,
On 2016/11/30 01:22:19, alexmos wrote:
> Existing code uses SecurityOrigin::urlWithUniqueSecurityOrigin(), which is
> defined as "data:,".  Perhaps we can reuse that URL here?

Done.

[commit-bot: I haz the power, 2016-11-30 15:05:42]

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

[commit-bot: I haz the power, 2016-11-30 15:05:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[arthursonzogni, 2016-12-01 10:29:32]

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

[commit-bot: I haz the power, 2016-12-01 10:29:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[arthursonzogni, 2016-12-01 10:30:17]

The CQ bit was unchecked by arthursonzogni@chromium.org

[arthursonzogni, 2016-12-01 10:32:05]

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

[commit-bot: I haz the power, 2016-12-01 10:32:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[arthursonzogni, 2016-12-01 10:32:27]

Patchset #7 (id:160001) has been deleted

[commit-bot: I haz the power, 2016-12-01 14:29:29]

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

[commit-bot: I haz the power, 2016-12-01 14:29:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_browser_side_navigation_rel on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[arthursonzogni, 2016-12-05 09:10:11]

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

[commit-bot: I haz the power, 2016-12-05 09:10:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 09:12:56]

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

[commit-bot: I haz the power, 2016-12-05 09:12:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[arthursonzogni, 2016-12-05 09:26:44]

Patchset #6 (id:140001) has been deleted

[arthursonzogni, 2016-12-05 09:45:10]

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

[commit-bot: I haz the power, 2016-12-05 09:45:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 13:38:23]

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

[commit-bot: I haz the power, 2016-12-05 13:38:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_browser_side_navigation_rel on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[arthursonzogni, 2016-12-05 13:54:26]

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

[commit-bot: I haz the power, 2016-12-05 13:54:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[arthursonzogni, 2016-12-05 13:56:05]

The CQ bit was unchecked by arthursonzogni@chromium.org

[arthursonzogni, 2016-12-05 14:16:44]

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

[commit-bot: I haz the power, 2016-12-05 14:17:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[arthursonzogni, 2016-12-05 14:17:03]

Patchset #8 (id:220001) has been deleted

[commit-bot: I haz the power, 2016-12-05 15:57:42]

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

[commit-bot: I haz the power, 2016-12-05 15:57:43]

Dry run: This issue passed the CQ dry run.

[clamy, 2016-12-06 17:09:44]

Thanks! I'm mostly happy with the navigation code, some comments below. Note
that I haven't reviewed the logic of the AncestorThrottle, since I'm not
familiar with it.

https://codereview.chromium.org/2488743003/diff/240001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2488743003/diff/240001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:294: state_ =
CANCELING_REQUEST;
Note: technically the navigation could be cancelled in DEFERRING_RESPONSE state
(DCHECK above is wrong). In that case, would setting the state to
CANCELING_REQUEST still make sense?

https://codereview.chromium.org/2488743003/diff/240001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2488743003/diff/240001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:299: CANCELING_RESPONSE,
What is the difference between CANCELING_REQUEST & CANCELING_RESPONSE?

https://codereview.chromium.org/2488743003/diff/240001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl_unittest.cc (right):

https://codereview.chromium.org/2488743003/diff/240001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl_unittest.cc:105: bool
IsCancelingResponse() {
I don't see this function being used in the tests. It would probably be a good
idea to add a unit test that uses it :) and that checks whether BLOCK_RESPONSE
results in the right state being set.

https://codereview.chromium.org/2488743003/diff/240001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2488743003/diff/240001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:581: OnRequestFailed(false,
net::ERR_BLOCKED_BY_RESPONSE);
Please add some comment mentionning not to add code after this line because the
NavigationRequest will have been destroyed.

https://codereview.chromium.org/2488743003/diff/240001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2488743003/diff/240001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2378: // display error pages but a blank
pages instead.
nit:s/pages/page

[arthursonzogni, 2016-12-07 16:18:52]

Patchset #9 (id:260001) has been deleted

[arthursonzogni, 2016-12-07 16:25:02]

Thanks @clamy!

@mkwst: Are you happy with the change?

https://codereview.chromium.org/2488743003/diff/240001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2488743003/diff/240001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:294: state_ =
CANCELING_REQUEST;
On 2016/12/06 17:09:43, clamy wrote:
> Note: technically the navigation could be cancelled in DEFERRING_RESPONSE
state
> (DCHECK above is wrong). In that case, would setting the state to
> CANCELING_REQUEST still make sense?

I updated the DCHECK.
As I said above, I will merge the two state. Then there is no more problems.

I made a new test with two navigation throttles, one with BLOCK_RESPONSE, the
other one with PROCEED. It checks that the navigation is canceled and the second
throttle is not called.

https://codereview.chromium.org/2488743003/diff/240001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2488743003/diff/240001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:299: CANCELING_RESPONSE,
On 2016/12/06 17:09:43, clamy wrote:
> What is the difference between CANCELING_REQUEST & CANCELING_RESPONSE?

According to me,
* CANCELLING_REQUEST happens when the request is cancelled and no network
request has been made.
* CANCELLING_RESPONSE happens when the network request has been made and the
response has be received. The navigation is canceled at this point.

Why can't we only have one common state? I don't know, I think it could be
useful for test only.
I will merge the two for the moment. @mkwst do you objections?

https://codereview.chromium.org/2488743003/diff/240001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl_unittest.cc (right):

https://codereview.chromium.org/2488743003/diff/240001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl_unittest.cc:105: bool
IsCancelingResponse() {
On 2016/12/06 17:09:44, clamy wrote:
> I don't see this function being used in the tests. It would probably be a good
> idea to add a unit test that uses it :) and that checks whether BLOCK_RESPONSE
> results in the right state being set.

Good idea.
Instead, I merged the two state, this function doesn't exist anymore.
Though, I added one new test with BLOCK_RESPONSE.

https://codereview.chromium.org/2488743003/diff/240001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2488743003/diff/240001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:581: OnRequestFailed(false,
net::ERR_BLOCKED_BY_RESPONSE);
On 2016/12/06 17:09:44, clamy wrote:
> Please add some comment mentionning not to add code after this line because
the
> NavigationRequest will have been destroyed.

Done.

[Mike West, 2016-12-08 09:19:42]

Merging the state of the enum seems reasonable to me.

The //third_party/WebKit and `AncestorThrottle` bits LGTM, though I don't know
how much my vote counts since I'm responsible for at least some of the changes.
I'd appreciate it if other folks would corroborate my approval. :)

[arthursonzogni, 2016-12-08 09:25:59]

Patchset #7 (id:200001) has been deleted

[clamy, 2016-12-09 17:14:02]

Thanks! A few more comments now that I have gone through the AncestorThrottle
itself.

https://codereview.chromium.org/2488743003/diff/280001/components/error_page/...
File components/error_page/common/localized_error.cc (right):

https://codereview.chromium.org/2488743003/diff/280001/components/error_page/...
components/error_page/common/localized_error.cc:317:
{net::ERR_BLOCKED_BY_RESPONSE,
Does this mean net::ERR_BLOCKED_BY_RESPONSE will display an error page? If so,
is that what we want or do we want to display a blank frame?

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
File content/browser/frame_host/ancestor_throttle.cc (right):

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.cc:65:
handle->frame_tree_node()->frame_tree()->root()->current_origin();
@mkwst: sanity check. I see that we're going to the root directly and checking
its origin. Shoudl we walk up the frame tree instead and check each of the
parent frames?

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
File content/browser/frame_host/ancestor_throttle.h (right):

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.h:16: class NavigationHandle;
nit: the forward declaration can be moved below, so that we open the namespace
content/ just once.

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.h:42: explicit
AncestorThrottle(NavigationHandle* handle);
Considering that we have a function for creating the Throttle above, shouldn't
we move the constructor to the private part of the header? (especially since
tests are made friends of this class).

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
File content/browser/frame_host/ancestor_throttle_unittest.cc (right):

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle_unittest.cc:31:
std::replace(header_string.begin(), header_string.end(), '\n', '\0');
If I read this correctly, we're replacing the \n we just set above by \0. Why is
it needed?

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:786: 
nit: no need for empty line here.

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:470: CHECK(result !=
NavigationThrottle::BLOCK_RESPONSE);
nit: this should be changed to DCHECK. Same below.

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:482: // DO NOT ADD CODE after
this. The previous call to OnRequestFailed have
nit:s/have/has

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:584: // DO NOT ADD CODE after
this. The previous call to OnRequestFailed have
nit:s/have/has

[arthursonzogni, 2016-12-13 10:19:50]

Patchset #9 (id:300001) has been deleted

[Mike West, 2016-12-13 13:59:33]

On 2016/12/09 at 17:14:02, clamy wrote:
>
https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
> content/browser/frame_host/ancestor_throttle.cc:65:
handle->frame_tree_node()->frame_tree()->root()->current_origin();
> @mkwst: sanity check. I see that we're going to the root directly and checking
its origin. Shoudl we walk up the frame tree instead and check each of the
parent frames?

Sadly, yes. The web relies on this behavior (I broke docs last time I tried to
change it). `frame-ancestors` has the sane behavior of walking the whole
ancestor tree for matches, but `X-Frame-Options` just checks the top-level.

[arthursonzogni, 2016-12-13 15:16:30]

Patchset #10 (id:340001) has been deleted

[arthursonzogni, 2016-12-13 15:31:41]

Patchset #10 (id:360001) has been deleted

[arthursonzogni, 2016-12-13 15:32:13]

Thanks Camille!

There were counters on the renderer-side that are no more incremented. Thus, I
tried to add in the last patch one histogram that counts these events. Please
@mkwst, I would like to know if you are happy with the values that are recorded
and how it's done.

https://codereview.chromium.org/2488743003/diff/280001/components/error_page/...
File components/error_page/common/localized_error.cc (right):

https://codereview.chromium.org/2488743003/diff/280001/components/error_page/...
components/error_page/common/localized_error.cc:317:
{net::ERR_BLOCKED_BY_RESPONSE,
On 2016/12/09 17:14:01, clamy wrote:
> Does this mean net::ERR_BLOCKED_BY_RESPONSE will display an error page? If so,
> is that what we want or do we want to display a blank frame?

Yes, the intent is to display error pages.
The thing is that after in RenderFrameImpl::LoadNavigationErrorPage, the
net::ERR_BLOCKED_BY_RESPONSE error page is transformed into a blank page because
there is a problem for the moment with how error pages are handled in Chrome.
The URL of the page that fails to load is committed in the browser, which causes
problems.

It would be nice if we could display an error page. That's what we want in the
future. Currently a blank page is displayed. That's the current chrome behavior
and this patch doesn't change this.

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
File content/browser/frame_host/ancestor_throttle.cc (right):

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.cc:65:
handle->frame_tree_node()->frame_tree()->root()->current_origin();
On 2016/12/09 17:14:01, clamy wrote:
> @mkwst: sanity check. I see that we're going to the root directly and checking
> its origin. Shoudl we walk up the frame tree instead and check each of the
> parent frames?

The specification doesn't specify what should be done, it only says that there
is differences between browsers (checking parent frame, top-frame or the whole
chain of ancestors).
Current chrome implementation only checks the top-frame.

There is only one thing to note, before this patch, the whole chain of ancestors
is checked for incrementing a counter:
UseCounter::XFrameOptionsSameOriginWithBadAncestorChain;
Nevertheless, the response is not blocked.

I will add this counter.

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
File content/browser/frame_host/ancestor_throttle.h (right):

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.h:16: class NavigationHandle;
On 2016/12/09 17:14:01, clamy wrote:
> nit: the forward declaration can be moved below, so that we open the namespace
> content/ just once.

Done.

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.h:42: explicit
AncestorThrottle(NavigationHandle* handle);
On 2016/12/09 17:14:01, clamy wrote:
> Considering that we have a function for creating the Throttle above, shouldn't
> we move the constructor to the private part of the header? (especially since
> tests are made friends of this class).

Done.

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
File content/browser/frame_host/ancestor_throttle_unittest.cc (right):

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle_unittest.cc:31:
std::replace(header_string.begin(), header_string.end(), '\n', '\0');
On 2016/12/09 17:14:01, clamy wrote:
> If I read this correctly, we're replacing the \n we just set above by \0. Why
is
> it needed?

It's how the HttpResponseHeaders constructor requires the string to be.

// Parses the given raw_headers.  raw_headers should be formatted thus:
// includes the http status response line, each line is \0-terminated, and
// it's terminated by an empty line (ie, 2 \0s in a row).
// (Note that line continuations should have already been joined;
// see HttpUtil::AssembleRawHeaders)
//
// HttpResponseHeaders does not perform any encoding changes on the input.
//
explicit HttpResponseHeaders(const std::string& raw_headers);

So yes, \n\n is added and then transformed into \0\0.

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:482: // DO NOT ADD CODE after
this. The previous call to OnRequestFailed have
On 2016/12/09 17:14:01, clamy wrote:
> nit:s/have/has
Done

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:584: // DO NOT ADD CODE after
this. The previous call to OnRequestFailed have
On 2016/12/09 17:14:01, clamy wrote:
> nit:s/have/has

Done.

[clamy, 2016-12-16 15:21:43]

Thanks! A few more comments.

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
File content/browser/frame_host/ancestor_throttle.cc (right):

https://codereview.chromium.org/2488743003/diff/280001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.cc:33:
AncestorThrottle::AncestorThrottle(NavigationHandle* handle)
Actually you can keep the constructor here. We generally do an exception to the
rule of ordering functions in the .cc file for constructors & destructors, since
it easier to read if they are all kept at the same place in the .cc file.

https://codereview.chromium.org/2488743003/diff/380001/content/browser/frame_...
File content/browser/frame_host/ancestor_throttle.cc (right):

https://codereview.chromium.org/2488743003/diff/380001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.cc:51:
UMA_HISTOGRAM_ENUMERATION(kXFrameOptionsSameOriginHistogram, TOTAL,
I find the value TOTAL a bit weird here. My suggestion would be to scrap it in
favor of having a value recorded for each of the cases below. This would mean
adding values for CONFLICT, INVALID, DENY, BYPASS & ALLOWALL (& maybe none?).

https://codereview.chromium.org/2488743003/diff/380001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.cc:88:
UMA_HISTOGRAM_ENUMERATION(kXFrameOptionsSameOriginHistogram,
I find it very weird that we're registering this value multiple times. I would
find it more understandable if we registered it in the following way:
- if the parent chain is good: SAME_ORIGIN
- if the top level is wrong and we block: SAME_ORIGIN_BLOCKED
- if the top level is good but the parent chain is wrong:
SAME_ORIGIN_WITH_BAD_ANCESTOR_CHAIN

Ideally: we would always register precisely one value for the histogram when
going in the function. This would make it easier to read and use.

https://codereview.chromium.org/2488743003/diff/380001/content/browser/frame_...
File content/browser/frame_host/ancestor_throttle.h (right):

https://codereview.chromium.org/2488743003/diff/380001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.h:38: enum XFrameOptionsSameOrigin
{
I don't think this is needed outside of the class right? If not, it can be moved
to the anonymous namespace inside the .cc file (this is generally the practice
for histogram enums).

https://codereview.chromium.org/2488743003/diff/380001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.h:43:
XFRAMEOPTIONS_SAMEORIGIN_COUNT
I think this should be XFRAMEOPTIONS_SAMEORIGIN_MAX =
SAME_ORIGIN_WITH_BAD_ANCESTOR_CHAIN. Otherwise, it would be possible to register
a value of 4, which would mean nothing here.

https://codereview.chromium.org/2488743003/diff/380001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2488743003/diff/380001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2377: // Requests blocked by the
X-Frame-Options HTTP response header doesn't
nit: s/doesn't/don't

[arthursonzogni, 2016-12-19 10:28:48]

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

[commit-bot: I haz the power, 2016-12-19 10:28:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[arthursonzogni, 2016-12-19 12:01:19]

Thanks!

I agreed with all you comments. Currently, there is one and only one value
recorded for each sub-frames loaded.

https://codereview.chromium.org/2488743003/diff/380001/content/browser/frame_...
File content/browser/frame_host/ancestor_throttle.cc (right):

https://codereview.chromium.org/2488743003/diff/380001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.cc:51:
UMA_HISTOGRAM_ENUMERATION(kXFrameOptionsSameOriginHistogram, TOTAL,
On 2016/12/16 15:21:43, clamy wrote:
> I find the value TOTAL a bit weird here. My suggestion would be to scrap it in
> favor of having a value recorded for each of the cases below. This would mean
> adding values for CONFLICT, INVALID, DENY, BYPASS & ALLOWALL (& maybe none?).

Done.

https://codereview.chromium.org/2488743003/diff/380001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.cc:88:
UMA_HISTOGRAM_ENUMERATION(kXFrameOptionsSameOriginHistogram,
On 2016/12/16 15:21:43, clamy wrote:
> I find it very weird that we're registering this value multiple times. I would
> find it more understandable if we registered it in the following way:
> - if the parent chain is good: SAME_ORIGIN
> - if the top level is wrong and we block: SAME_ORIGIN_BLOCKED
> - if the top level is good but the parent chain is wrong:
> SAME_ORIGIN_WITH_BAD_ANCESTOR_CHAIN
> 
> Ideally: we would always register precisely one value for the histogram when
> going in the function. This would make it easier to read and use.

Done.

https://codereview.chromium.org/2488743003/diff/380001/content/browser/frame_...
File content/browser/frame_host/ancestor_throttle.h (right):

https://codereview.chromium.org/2488743003/diff/380001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.h:38: enum XFrameOptionsSameOrigin
{
On 2016/12/16 15:21:43, clamy wrote:
> I don't think this is needed outside of the class right? If not, it can be
moved
> to the anonymous namespace inside the .cc file (this is generally the practice
> for histogram enums).

Done.

https://codereview.chromium.org/2488743003/diff/380001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.h:43:
XFRAMEOPTIONS_SAMEORIGIN_COUNT
On 2016/12/16 15:21:43, clamy wrote:
> I think this should be XFRAMEOPTIONS_SAMEORIGIN_MAX =
> SAME_ORIGIN_WITH_BAD_ANCESTOR_CHAIN. Otherwise, it would be possible to
register
> a value of 4, which would mean nothing here.

Done.

https://codereview.chromium.org/2488743003/diff/380001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2488743003/diff/380001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2377: // Requests blocked by the
X-Frame-Options HTTP response header doesn't
On 2016/12/16 15:21:43, clamy wrote:
> nit: s/doesn't/don't

Done.

[commit-bot: I haz the power, 2016-12-19 12:31:35]

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

[commit-bot: I haz the power, 2016-12-19 12:31:36]

Dry run: This issue passed the CQ dry run.

[alexmos, 2016-12-20 01:34:39]

I took another quick look at this before disappearing for the holidays, and this
LGTM with a few requests below, but please wait for clamy@ to be happy with
this.

https://codereview.chromium.org/2488743003/diff/420001/content/browser/frame_...
File content/browser/frame_host/ancestor_throttle.cc (right):

https://codereview.chromium.org/2488743003/diff/420001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.cc:35: // X-Frame-Options:
SAMEORIGIN. The navigation proceeds and every ancestors
nit: s/ancestors have/ancestor has/

https://codereview.chromium.org/2488743003/diff/420001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.cc:91:
UMA_HISTOGRAM_ENUMERATION(kXFrameOptionsSameOriginHistogram, CONFLICT,
In the past, I've been asked to create a helper function to log a histogram like
this.  This is because each macro expands to a lot of code, so it will reduce
binary bloat, and also less repetition and chance of typos in the metric name,
etc.

https://codereview.chromium.org/2488743003/diff/420001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.cc:257: if
(std::count_if(tokens.begin(), tokens.end(), [](std::string token) {
nit: this might be a bit more readable if the condition was assigned to a helper
bool var, which would then be checked in the if.

https://codereview.chromium.org/2488743003/diff/420001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2488743003/diff/420001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:6950:
EXPECT_NE(root->child_at(0)->current_frame_host()->last_successful_url(),
Is this change still necessary?  Committing a blank page should make this work
with the old check again.  (The full discussion about it was at 
https://codereview.chromium.org/2321503002)

[arthursonzogni, 2016-12-20 10:19:54]

Patchset #13 (id:440001) has been deleted

[arthursonzogni, 2016-12-20 10:26:17]

Patchset #13 (id:460001) has been deleted

[arthursonzogni, 2016-12-20 10:29:05]

Patchset #13 (id:480001) has been deleted

[arthursonzogni, 2016-12-20 10:29:43]

Thanks @alexmos!

https://codereview.chromium.org/2488743003/diff/420001/content/browser/frame_...
File content/browser/frame_host/ancestor_throttle.cc (right):

https://codereview.chromium.org/2488743003/diff/420001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.cc:35: // X-Frame-Options:
SAMEORIGIN. The navigation proceeds and every ancestors
On 2016/12/20 01:34:39, alexmos (OOO Dec 20 to Dec 27) wrote:
> nit: s/ancestors have/ancestor has/

Done.

https://codereview.chromium.org/2488743003/diff/420001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.cc:91:
UMA_HISTOGRAM_ENUMERATION(kXFrameOptionsSameOriginHistogram, CONFLICT,
On 2016/12/20 01:34:39, alexmos (OOO Dec 20 to Dec 27) wrote:
> In the past, I've been asked to create a helper function to log a histogram
like
> this.  This is because each macro expands to a lot of code, so it will reduce
> binary bloat, and also less repetition and chance of typos in the metric name,
> etc.

Done. Much better like this.

https://codereview.chromium.org/2488743003/diff/420001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.cc:257: if
(std::count_if(tokens.begin(), tokens.end(), [](std::string token) {
On 2016/12/20 01:34:39, alexmos (OOO Dec 20 to Dec 27) wrote:
> nit: this might be a bit more readable if the condition was assigned to a
helper
> bool var, which would then be checked in the if.

Yes I agree, but instead, I replaced std::count_if and the lambda function by a
loop. Then I move the code that checks if the header contains the
frame-ancestors CSP into a new function: HeaderContainsFrameAncestorsCSP.

https://codereview.chromium.org/2488743003/diff/420001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2488743003/diff/420001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:6950:
EXPECT_NE(root->child_at(0)->current_frame_host()->last_successful_url(),
On 2016/12/20 01:34:39, alexmos (OOO Dec 20 to Dec 27) wrote:
> Is this change still necessary?  Committing a blank page should make this work
> with the old check again.  (The full discussion about it was at 
> https://codereview.chromium.org/2321503002)

You are right.

[clamy, 2016-12-20 14:27:01]

Thanks! Lgtm.

https://codereview.chromium.org/2488743003/diff/500001/content/browser/frame_...
File content/browser/frame_host/ancestor_throttle.cc (right):

https://codereview.chromium.org/2488743003/diff/500001/content/browser/frame_...
content/browser/frame_host/ancestor_throttle.cc:53: // The frame sets multiple
'X-Frame-Options' header with conflicting values.
nit:s/header/headers

[arthursonzogni, 2016-12-20 16:02:10]

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

[arthursonzogni, 2016-12-20 16:02:11]

Hi @edwardjung,

Please could you take a look at:
components/error_page/common/localized_error.cc

It adds a new error page for iframes blocked by an X-Frame-Options policy.

You already have LGTM this code 7 month ago. That's a lot of time, so I didn't
use TBR=edwardjung@chromium.org.

Please note that due to problems with how error pages are handled internally. We
choose to replace temporarily this error page with a blank page when the
renderer navigates to it. FYI, displaying a blank page is already what chrome
does for the moment.

Thanks!

[edwardjung, 2016-12-20 16:17:58]

localized_error.cc lgtm

[arthursonzogni, 2016-12-20 16:35:13]

On 2016/12/20 16:17:58, edwardjung wrote:
> localized_error.cc lgtm

Thanks!

[arthursonzogni, 2016-12-20 16:35:46]

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

[commit-bot: I haz the power, 2016-12-20 16:38:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 20:45:38]

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

[commit-bot: I haz the power, 2016-12-20 20:45:40]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2016-12-21 08:41:50]

The CQ bit was checked by arthursonzogni@chromium.org

[arthursonzogni, 2016-12-21 08:41:51]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2488743003/#ps500001
(title: "Addressed comments (@alexmos #2)")

[commit-bot: I haz the power, 2016-12-21 08:42:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 08:47:20]

CQ is committing da patch.

Bot data: {"patchset_id": 500001, "attempt_start_ts": 1482309710896820,
"parent_rev": "bb36284315ccbb1b31250a9119a5286fd29e2512", "commit_rev":
"c69e469c80e49ce1af14db8eccb81ec1fcae761e"}

[commit-bot: I haz the power, 2016-12-21 08:47:50]

Description was changed from

==========
(Re-)introduce AncestorThrottle to handle 'X-Frame-Options'.

This patch moves ancestor-based blocking behavior up from Blink into the
browser, which gives us some correctness assurances we can't match in
the renderer (for top-level plugins, etc), and ensures that a (malicious?)
renderer isn't responsible for policing itself.

This is a re-land of https://codereview.chromium.org/1617043002, after
clamy@'s heroic refactoring effort to process 'Content-Disposition' and
similar download-triggering mechanisms before handing things off to
this new throttle.

BUG=555418
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel
==========

to

==========
(Re-)introduce AncestorThrottle to handle 'X-Frame-Options'.

This patch moves ancestor-based blocking behavior up from Blink into the
browser, which gives us some correctness assurances we can't match in
the renderer (for top-level plugins, etc), and ensures that a (malicious?)
renderer isn't responsible for policing itself.

This is a re-land of https://codereview.chromium.org/1617043002, after
clamy@'s heroic refactoring effort to process 'Content-Disposition' and
similar download-triggering mechanisms before handing things off to
this new throttle.

BUG=555418
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel

Review-Url: https://codereview.chromium.org/2488743003
==========

[commit-bot: I haz the power, 2016-12-21 08:47:52]

Committed patchset #13 (id:500001)

[commit-bot: I haz the power, 2016-12-21 08:50:27]

Description was changed from

==========
(Re-)introduce AncestorThrottle to handle 'X-Frame-Options'.

This patch moves ancestor-based blocking behavior up from Blink into the
browser, which gives us some correctness assurances we can't match in
the renderer (for top-level plugins, etc), and ensures that a (malicious?)
renderer isn't responsible for policing itself.

This is a re-land of https://codereview.chromium.org/1617043002, after
clamy@'s heroic refactoring effort to process 'Content-Disposition' and
similar download-triggering mechanisms before handing things off to
this new throttle.

BUG=555418
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel

Review-Url: https://codereview.chromium.org/2488743003
==========

to

==========
(Re-)introduce AncestorThrottle to handle 'X-Frame-Options'.

This patch moves ancestor-based blocking behavior up from Blink into the
browser, which gives us some correctness assurances we can't match in
the renderer (for top-level plugins, etc), and ensures that a (malicious?)
renderer isn't responsible for policing itself.

This is a re-land of https://codereview.chromium.org/1617043002, after
clamy@'s heroic refactoring effort to process 'Content-Disposition' and
similar download-triggering mechanisms before handing things off to
this new throttle.

BUG=555418
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel

Committed: https://crrev.com/c5a2f126d6e96ef139e4211ff225169b810490f4
Cr-Commit-Position: refs/heads/master@{#440055}
==========

[commit-bot: I haz the power, 2016-12-21 08:50:29]

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