(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

[Mike West, 2016-09-07 12:33:06]

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

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

[Mike West, 2016-09-08 07:39:26]

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

[commit-bot: I haz the power, 2016-09-08 07:50:14]

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-09-08 07:50:49]

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

[commit-bot: I haz the power, 2016-09-08 07:50:49]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Mike West, 2016-09-08 13:11:26]

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

[commit-bot: I haz the power, 2016-09-08 13:11:37]

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-09-08 13:23:41]

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

[commit-bot: I haz the power, 2016-09-08 13:23:42]

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

[Mike West, 2016-09-09 10:15:46]

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

[commit-bot: I haz the power, 2016-09-09 10:15:57]

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-09-09 11:04:32]

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

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

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Mike West, 2016-09-12 09:30:04]

mkwst@chromium.org changed reviewers:
+ alexmos@chromium.org, clamy@chromium.org

[Mike West, 2016-09-12 09:30:05]

alexmos@, clamy@: I'm in the process of rebasing the XFO->Browser changes, and
running into an issue with cancellation behavior. In particular, it looks like
the URL which asserts the XFO header (and which looks to be successfully
cancelled) still commits a URL to the current frame host (see
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_iso...).
The rest of that test passes if I comment out the specific EXPECT_NE that's
failing...

It doesn't look like committing the blocked URL should be possible based on my
reading of the cancellation code, so I'm obviously missing something. Can y'all
point me in the right direction? :)

[Mike West, 2016-09-12 09:30:12]

Patchset #1 (id:1) has been deleted

[Mike West, 2016-09-12 09:30:17]

Patchset #1 (id:20001) has been deleted

[alexmos, 2016-09-12 22:17:15]

Thanks, this looks great but it seems there's a failing test,
SitePerProcessBrowserTest.CrossSiteIframeBlockedByXFrameOptionsOrCSP, which I've
added in https://codereview.chromium.org/2096453002 soon after your original
attempt to land this.  Looks like it's complaining that the blocked page commits
the URL that was blocked.  Can you try the repro steps in
https://crbug.com/622385 to check that this CL doesn't bring back that kill? 
And/or, is the test wrong now (e.g., should it be checking last_successful_url()
instead)?

[Mike West, 2016-09-13 13:25:09]

On 2016/09/12 at 22:17:15, alexmos wrote:
> Thanks, this looks great but it seems there's a failing test,
SitePerProcessBrowserTest.CrossSiteIframeBlockedByXFrameOptionsOrCSP

Yeah. That's what I was asking about in
https://codereview.chromium.org/2321503002#msg15. :) Changing the test to use
`last_successful_url()` works, but it's not clear to me why it's correct. That
is, why does the cancelled request show up as committed in the current frame
host? Shouldn't cancelling the request mean that it doesn't commit?

[Mike West, 2016-09-13 13:25:18]

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

[commit-bot: I haz the power, 2016-09-13 13:25:35]

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

[Mike West, 2016-09-13 13:26:45]

> Can you try the repro steps in https://crbug.com/622385 to check that this CL
doesn't bring back that kill?

I ran through the steps, and see an XFO error in the console (as expected), but
the renderer isn't killed. So that's good.

[commit-bot: I haz the power, 2016-09-13 16:48:44]

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

[commit-bot: I haz the power, 2016-09-13 16:48:48]

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

[Mike West, 2016-09-13 18:02:35]

Hrm. That specific test passes under PlzNavigate, but crashes in status quo
(https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium....
probably because we're in CANCELING when the host is requested, and not
WILL_PROCESS_RESPONSE. Not sure if it's an artifact of the way the test is put
together or not...

[alexmos, 2016-09-13 19:14:58]

On 2016/09/13 18:02:35, Mike West wrote:
> Hrm. That specific test passes under PlzNavigate, but crashes in status quo
>
(https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium....
> probably because we're in CANCELING when the host is requested, and not
> WILL_PROCESS_RESPONSE. Not sure if it's an artifact of the way the test is put
> together or not...

Hmm, did you mean the reverse?  It seems to be crashing with PlzNavigate but is
fine without it.  Looks like it's hitting CHECK_GE(state_,
WILL_PROCESS_RESPONSE) in GetRenderFrameHost.

Also, I looked at the blocking logic for CWS from issue 622385 [1], and it
didn't make sense to me why the current behavior wouldn't lead to a renderer
kill.  So I tried patching in the current CL on ToT, and I actually do get a
renderer kill with repro steps from 622385.  I'm really curious why you didn't
get it when you tried it.  What's also interesting is if I follow these repro
steps with --enable-browser-side-navigation, the browser crashes with the above
CHECK_GE that the test is also hitting.  So I don't think this is the test's
fault, and it looks like that this might actually reintroduce those renderer
kills...

'm not that familiar with all the error behavior; I noticed that with this CL,
we now commit an error page with the sad tab for blocked pages (yay!), but that
sends the blocked page's URL as the committed URL.  Is there a way for these
kinds of errors to instead commit a safer URL, like the data:, URL that it used
to commit?  (Also, I thought some errors commit chromewebdata URLs, but
apparently not this one?)  I briefly chatted with Charlie/Nasko about this, and
we don't want the blocked URL as the last committed URL for that subframe, as
the error commits in the same process, and someone could later find a way to
reload something like CWS in-process, or incorrectly assume that the frame
committed there has the CWS origin for a security check.

[1]
https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_content...

[Mike West, 2016-09-14 07:45:06]

On 2016/09/13 at 19:14:58, alexmos wrote:
> Hmm, did you mean the reverse?  It seems to be crashing with PlzNavigate but
is fine without it.  Looks like it's hitting CHECK_GE(state_,
WILL_PROCESS_RESPONSE) in GetRenderFrameHost.

Yes. I meant the opposite of what I typed. :)

> Also, I looked at the blocking logic for CWS from issue 622385 [1], and it
didn't make sense to me why the current behavior wouldn't lead to a renderer
kill.

Ah. Right. I ran this in content shell, which, of course, doesn't pull in
anything from //chrome. Like the webstore URL and associated behaviors. *cough*
Sorry about that. Running in `chrome` gives me the renderer kill you're seeing.

> I'm not that familiar with all the error behavior; I noticed that with this
CL, we now commit an error page with the sad tab for blocked pages (yay!), but
that sends the blocked page's URL as the committed URL.

Yeah, this looks like where the problem is introduced. I'll dig into this a bit
today.

[Mike West, 2016-09-14 09:12:57]

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

[commit-bot: I haz the power, 2016-09-14 09:13:11]

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

[Mike West, 2016-09-14 09:21:25]

On 2016/09/14 at 07:45:06, Mike West wrote:
> > I'm not that familiar with all the error behavior; I noticed that with this
CL, we now commit an error page with the sad tab for blocked pages (yay!), but
that sends the blocked page's URL as the committed URL.
> 
> Yeah, this looks like where the problem is introduced. I'll dig into this a
bit today.

Splitting `CANCELING` into `CANCELING_REQUEST` and `CANCELING_RESPONSE` allows
us to cleanly address the DCHECK in the test. However, the response isn't
actually cancelled, as you'll see when the bots finish running.

Is it possible that none of the infrastructure actually works under PlzNavigate?
:) I now remember that clamy@ suggested in
https://codereview.chromium.org/1616943003#msg9 that the initial implementation
of response-driven cancelation needed some ~complicated changes to work with
browser-side navigation. Perhaps we never actually made those changes?

Putting in a dummy "Cancel-everything!" throttle seems to confirm that things
don't seem to work quite the way they should. :/

https://codereview.chromium.org/2321503002/diff/80001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2321503002/diff/80001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1038: 
This is ugly. But without some ~large refactoring in the way error pages are put
together, it's going to be pretty hard to satisfy the constraint that the
committed URL must differ from the actual URL. My vague understanding of error
pages (based on an hour of digging around this morning) is that they
intentionally use the failed URL so that reload works the way it's expected to
work. *shrug*

https://codereview.chromium.org/2321503002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2321503002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:245: CANCELING_RESPONSE,
We have a host at this point, whereas we don't have a response during
`CANCELING_REQUEST`.

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

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

[commit-bot: I haz the power, 2016-09-14 10:01:13]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[clamy, 2016-09-14 11:19:07]

On 2016/09/14 09:21:25, Mike West wrote:
> On 2016/09/14 at 07:45:06, Mike West wrote:
> > > I'm not that familiar with all the error behavior; I noticed that with
this
> CL, we now commit an error page with the sad tab for blocked pages (yay!), but
> that sends the blocked page's URL as the committed URL.
> > 
> > Yeah, this looks like where the problem is introduced. I'll dig into this a
> bit today.
> 
> Splitting `CANCELING` into `CANCELING_REQUEST` and `CANCELING_RESPONSE` allows
> us to cleanly address the DCHECK in the test. However, the response isn't
> actually cancelled, as you'll see when the bots finish running.
> 
> Is it possible that none of the infrastructure actually works under
PlzNavigate?
> :) I now remember that clamy@ suggested in
> https://codereview.chromium.org/1616943003#msg9 that the initial
implementation
> of response-driven cancelation needed some ~complicated changes to work with
> browser-side navigation. Perhaps we never actually made those changes?
> 
> Putting in a dummy "Cancel-everything!" throttle seems to confirm that things
> don't seem to work quite the way they should. :/

Weird. Is it canceling or blocking? I'm pretty sure we did not introduce support
for blocking (ie in PlzNavigate it's just going to go through), but support for
cancelling and pausing the request at WillProcessResponse stage was introduced
in https://codereview.chromium.org/1811913003.

> 
>
https://codereview.chromium.org/2321503002/diff/80001/chrome/renderer/chrome_...
> File chrome/renderer/chrome_content_renderer_client.cc (right):
> 
>
https://codereview.chromium.org/2321503002/diff/80001/chrome/renderer/chrome_...
> chrome/renderer/chrome_content_renderer_client.cc:1038: 
> This is ugly. But without some ~large refactoring in the way error pages are
put
> together, it's going to be pretty hard to satisfy the constraint that the
> committed URL must differ from the actual URL. My vague understanding of error
> pages (based on an hour of digging around this morning) is that they
> intentionally use the failed URL so that reload works the way it's expected to
> work. *shrug*
> 
>
https://codereview.chromium.org/2321503002/diff/80001/content/browser/frame_h...
> File content/browser/frame_host/navigation_handle_impl.h (right):
> 
>
https://codereview.chromium.org/2321503002/diff/80001/content/browser/frame_h...
> content/browser/frame_host/navigation_handle_impl.h:245: CANCELING_RESPONSE,
> We have a host at this point, whereas we don't have a response during
> `CANCELING_REQUEST`.

[Mike West, 2016-09-14 14:40:17]

On 2016/09/14 at 11:19:07, clamy wrote:
> On 2016/09/14 09:21:25, Mike West wrote:
> > On 2016/09/14 at 07:45:06, Mike West wrote:
> > > > I'm not that familiar with all the error behavior; I noticed that with
this
> > CL, we now commit an error page with the sad tab for blocked pages (yay!),
but
> > that sends the blocked page's URL as the committed URL.
> > > 
> > > Yeah, this looks like where the problem is introduced. I'll dig into this
a
> > bit today.
> > 
> > Splitting `CANCELING` into `CANCELING_REQUEST` and `CANCELING_RESPONSE`
allows
> > us to cleanly address the DCHECK in the test. However, the response isn't
> > actually cancelled, as you'll see when the bots finish running.
> > 
> > Is it possible that none of the infrastructure actually works under
PlzNavigate?
> > :) I now remember that clamy@ suggested in
> > https://codereview.chromium.org/1616943003#msg9 that the initial
implementation
> > of response-driven cancelation needed some ~complicated changes to work with
> > browser-side navigation. Perhaps we never actually made those changes?
> > 
> > Putting in a dummy "Cancel-everything!" throttle seems to confirm that
things
> > don't seem to work quite the way they should. :/
> 
> Weird. Is it canceling or blocking? I'm pretty sure we did not introduce
support for blocking (ie in PlzNavigate it's just going to go through), but
support for cancelling and pausing the request at WillProcessResponse stage was
introduced in https://codereview.chromium.org/1811913003.

For this CL, I'm interested in blocking the response. That is, take a look at
the results for
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium....
The AncestorThrottle is returning `BLOCK_RESPONSE`, triggering an error message,
etc, but the framed page is still rendered.

[clamy, 2016-09-14 14:43:10]

On 2016/09/14 14:40:17, Mike West wrote:
> On 2016/09/14 at 11:19:07, clamy wrote:
> > On 2016/09/14 09:21:25, Mike West wrote:
> > > On 2016/09/14 at 07:45:06, Mike West wrote:
> > > > > I'm not that familiar with all the error behavior; I noticed that with
> this
> > > CL, we now commit an error page with the sad tab for blocked pages (yay!),
> but
> > > that sends the blocked page's URL as the committed URL.
> > > > 
> > > > Yeah, this looks like where the problem is introduced. I'll dig into
this
> a
> > > bit today.
> > > 
> > > Splitting `CANCELING` into `CANCELING_REQUEST` and `CANCELING_RESPONSE`
> allows
> > > us to cleanly address the DCHECK in the test. However, the response isn't
> > > actually cancelled, as you'll see when the bots finish running.
> > > 
> > > Is it possible that none of the infrastructure actually works under
> PlzNavigate?
> > > :) I now remember that clamy@ suggested in
> > > https://codereview.chromium.org/1616943003#msg9 that the initial
> implementation
> > > of response-driven cancelation needed some ~complicated changes to work
with
> > > browser-side navigation. Perhaps we never actually made those changes?
> > > 
> > > Putting in a dummy "Cancel-everything!" throttle seems to confirm that
> things
> > > don't seem to work quite the way they should. :/
> > 
> > Weird. Is it canceling or blocking? I'm pretty sure we did not introduce
> support for blocking (ie in PlzNavigate it's just going to go through), but
> support for cancelling and pausing the request at WillProcessResponse stage
was
> introduced in https://codereview.chromium.org/1811913003.
> 
> For this CL, I'm interested in blocking the response. That is, take a look at
> the results for
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium....
> The AncestorThrottle is returning `BLOCK_RESPONSE`, triggering an error
message,
> etc, but the framed page is still rendered.

Ok so in this case it will not work with Plznavigate. I haven't implemented the
proper handling of BLOCK_RESPONSE yet.

[Mike West, 2016-09-14 15:48:14]

On 2016/09/14 at 14:43:10, clamy wrote:
> > For this CL, I'm interested in blocking the response. That is, take a look
at
> > the results for
> >
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium....
> > The AncestorThrottle is returning `BLOCK_RESPONSE`, triggering an error
message,
> > etc, but the framed page is still rendered.
> 
> Ok so in this case it will not work with Plznavigate. I haven't implemented
the proper handling of BLOCK_RESPONSE yet.

Ok. Should that block landing this patch? I don't know the timelines for
PlzNavigate, and I don't want to add things to your critical path unnecessarily.

[clamy, 2016-09-14 15:52:53]

On 2016/09/14 15:48:14, Mike West wrote:
> On 2016/09/14 at 14:43:10, clamy wrote:
> > > For this CL, I'm interested in blocking the response. That is, take a look
> at
> > > the results for
> > >
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium....
> > > The AncestorThrottle is returning `BLOCK_RESPONSE`, triggering an error
> message,
> > > etc, but the framed page is still rendered.
> > 
> > Ok so in this case it will not work with Plznavigate. I haven't implemented
> the proper handling of BLOCK_RESPONSE yet.
> 
> Ok. Should that block landing this patch? I don't know the timelines for
> PlzNavigate, and I don't want to add things to your critical path
unnecessarily.

This is not an issue. I was waiting for a usage of BLOCK_RESPONSE before working
on the PlzNavigate support. In the meantime, we'll just update our test
exclusion files.

[alexmos, 2016-09-14 20:25:11]

https://codereview.chromium.org/2321503002/diff/80001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2321503002/diff/80001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1038: 
On 2016/09/14 09:21:24, Mike West wrote:
> This is ugly. But without some ~large refactoring in the way error pages are
put
> together, it's going to be pretty hard to satisfy the constraint that the
> committed URL must differ from the actual URL. My vague understanding of error
> pages (based on an hour of digging around this morning) is that they
> intentionally use the failed URL so that reload works the way it's expected to
> work. *shrug*

Yeah, a bit unfortunate that we have to do this, but I guess I'm ok with it.

But here is a question for Charlie about the CWS kill.  The new error page sets
the blocked URL as the last committed URL, but the last committed origin is
actually "null" (since it's a data:text/html,chromewebdata page).  I wonder if
we could switch the CWS kill to work off of the committed origin instead of URL?
 (I.e., instead of RFHI::CanCommitURL doing this check on the URL,  move the
check to only happen via CanCommitOrigin.  Looks like CanCommitOrigin already
calls CanCommitURL with the GURL constructed from the origin, so we're
effectively checking this twice today.)  That also seems better for cases like
blob:/filesystem: URLs that we've been fighting through in recent bugs.  One
concern though would be if you tried to put CWS in a sandboxed iframe, which
would also commit a null origin.  Charlie, what do you think?

[Charlie Reis, 2016-09-14 21:39:04]

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

[Charlie Reis, 2016-09-14 21:39:05]

Note: I haven't looked at the rest, but I wanted to reply to Alex's question. 
Some concerns below.

https://codereview.chromium.org/2321503002/diff/80001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2321503002/diff/80001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1038: 
On 2016/09/14 20:25:10, alexmos wrote:
> On 2016/09/14 09:21:24, Mike West wrote:
> > This is ugly. But without some ~large refactoring in the way error pages are
> put
> > together, it's going to be pretty hard to satisfy the constraint that the
> > committed URL must differ from the actual URL. My vague understanding of
error
> > pages (based on an hour of digging around this morning) is that they
> > intentionally use the failed URL so that reload works the way it's expected
to
> > work. *shrug*
> 
> Yeah, a bit unfortunate that we have to do this, but I guess I'm ok with it.


Hmm, I'm not ok with this change.  This check isn't going to be specific to the
CWS for long.  We need to be able to enforce the process model for things like
--isolate-extensions and Site Isolation by killing the renderer if it claims to
commit a page it's not allowed to.  We can't suppress error pages in all those
cases.


> But here is a question for Charlie about the CWS kill.  The new error page
sets
> the blocked URL as the last committed URL, but the last committed origin is
> actually "null" (since it's a data:text/html,chromewebdata page).  I wonder if
> we could switch the CWS kill to work off of the committed origin instead of
URL?
>  (I.e., instead of RFHI::CanCommitURL doing this check on the URL,  move the
> check to only happen via CanCommitOrigin.  Looks like CanCommitOrigin already
> calls CanCommitURL with the GURL constructed from the origin, so we're
> effectively checking this twice today.)  That also seems better for cases like
> blob:/filesystem: URLs that we've been fighting through in recent bugs.  One
> concern though would be if you tried to put CWS in a sandboxed iframe, which
> would also commit a null origin.  Charlie, what do you think?

I'm not sure if that's safe.  Ideally we'd enforce it for both URL and origin,
rather than only checking origin.  The reason is that we end up with a record
that a URL committed in a process when it really couldn't have, and that
NavigationEntry ends up with a SiteInstance for a process that isn't allowed to
load the URL.  Mike mentioned the reload case, and we had exactly that bug in
https://crbug.com/560511, where reloading an error page would skip a process
swap when one was needed.  It's not clear that an origin check would always
catch that, given the possibility of a null origin in a sandboxed iframe that
Alex mentions.

At some point soon, I think we need to revisit how error pages work so that they
don't have this problem (and hopefully so that we don't have 5 variations of
error page strategies with different URL/origin/process properties, across net,
blob, filesystem, XFO, FilterURL, and probably others).

I realize we'll need something for the short term, but I'm not sure what that is
yet.  (It's possible that moving the check from CanCommitURL to CanCommitOrigin
might not be a big regression in the short term, but I'm concerned about it for
the reason above.)

[alexmos, 2016-09-17 01:43:17]

https://codereview.chromium.org/2321503002/diff/80001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2321503002/diff/80001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1038: 
On 2016/09/14 21:39:05, Charlie Reis (slow) wrote:
> On 2016/09/14 20:25:10, alexmos wrote:
> > On 2016/09/14 09:21:24, Mike West wrote:
> > > This is ugly. But without some ~large refactoring in the way error pages
are
> > put
> > > together, it's going to be pretty hard to satisfy the constraint that the
> > > committed URL must differ from the actual URL. My vague understanding of
> error
> > > pages (based on an hour of digging around this morning) is that they
> > > intentionally use the failed URL so that reload works the way it's
expected
> to
> > > work. *shrug*
> > 
> > Yeah, a bit unfortunate that we have to do this, but I guess I'm ok with it.
> 
> 
> Hmm, I'm not ok with this change.  This check isn't going to be specific to
the
> CWS for long.  We need to be able to enforce the process model for things like
> --isolate-extensions and Site Isolation by killing the renderer if it claims
to
> commit a page it's not allowed to.  We can't suppress error pages in all those
> cases.
> 
> 
> > But here is a question for Charlie about the CWS kill.  The new error page
> sets
> > the blocked URL as the last committed URL, but the last committed origin is
> > actually "null" (since it's a data:text/html,chromewebdata page).  I wonder
if
> > we could switch the CWS kill to work off of the committed origin instead of
> URL?
> >  (I.e., instead of RFHI::CanCommitURL doing this check on the URL,  move the
> > check to only happen via CanCommitOrigin.  Looks like CanCommitOrigin
already
> > calls CanCommitURL with the GURL constructed from the origin, so we're
> > effectively checking this twice today.)  That also seems better for cases
like
> > blob:/filesystem: URLs that we've been fighting through in recent bugs.  One
> > concern though would be if you tried to put CWS in a sandboxed iframe, which
> > would also commit a null origin.  Charlie, what do you think?
> 
> I'm not sure if that's safe.  Ideally we'd enforce it for both URL and origin,
> rather than only checking origin.  The reason is that we end up with a record
> that a URL committed in a process when it really couldn't have, and that
> NavigationEntry ends up with a SiteInstance for a process that isn't allowed
to
> load the URL.  Mike mentioned the reload case, and we had exactly that bug in
> https://crbug.com/560511, where reloading an error page would skip a process
> swap when one was needed.  It's not clear that an origin check would always
> catch that, given the possibility of a null origin in a sandboxed iframe that
> Alex mentions.
> 
> At some point soon, I think we need to revisit how error pages work so that
they
> don't have this problem (and hopefully so that we don't have 5 variations of
> error page strategies with different URL/origin/process properties, across
net,
> blob, filesystem, XFO, FilterURL, and probably others).
> 
> I realize we'll need something for the short term, but I'm not sure what that
is
> yet.  (It's possible that moving the check from CanCommitURL to
CanCommitOrigin
> might not be a big regression in the short term, but I'm concerned about it
for
> the reason above.)

Given Charlie's comments, and as a way to make progress on this in the short
term, I wonder if it's possible to reland this without committing error pages
for XFO blocks at first, and then add them in a followup that would handle all
the complications we're discussing?  I don't know how complicated that would be
though (e.g., looks like XFO blocks might not be able to use
ERR_BLOCKED_BY_CLIENT since that's used for other blocks that might want error
pages).  What do you all think?

(Also, just to point out, there's another issue related to errors and CWS kills,
https://crbug.com/588314 - we could also run into trouble if a redirect points
to a page that is blocked by XFO.)

[Mike West, 2016-10-26 13:03:17]

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

[commit-bot: I haz the power, 2016-10-26 13:03:38]

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-10-26 13:43:02]

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

[commit-bot: I haz the power, 2016-10-26 13:43:03]

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

[arthursonzogni, 2016-11-09 15:54:06]

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

[arthursonzogni, 2016-11-09 15:54:07]

Hi there

I tried to help.
https://codereview.chromium.org/2488743003/

This is a patch that makes PlzNavigate handle the BLOCK_RESPONSE enum value
(introduced by this CL). It results that the denied iframe request response is
blocked with PlzNavigate too.
There is also some quick fix that makes bots green.

FYI: There is still some PlzNavigate tests related to X-Frame-Options that
doesn't pass. But it's not really related to it. The thing is that requests
doesn't follow the same path with PlzNavigate. Thus
testRunner.dumpResourceLoadCallbacks() doesn't print the same things.
WillSendRequest is called two times per renderer-initiated request with
PlzNavigate instead of one :(

[arthursonzogni, 2016-11-09 15:57:23]

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

[arthursonzogni, 2016-11-22 16:02:35]

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