Do not trigger CrossSiteResourceHandler for streams or user certs.

Do not trigger CrossSiteResourceHandler for streams or user certs.

When a resource request gets pre-empted in favor of either a stream or user
certificate installation, do not trigger CrossSiteResourceHandler behavior.

Add a regression test to ensure the DCHECK does not fire.

BUG=342999
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251132

[davidben, 2014-02-12 19:57:56]

creis: Main review.
kalman: chrome/browser/extensions/

creis, do you think we'll want to merge this? The DCHECK isn't in release
builds, but I imagine confusing CSRH and friends into thinking there's a
cross-site transition in progress when there isn't would muck things up.

https://codereview.chromium.org/161003002/diff/1/content/browser/loader/cross...
File content/browser/loader/cross_site_resource_handler.cc (right):

https://codereview.chromium.org/161003002/diff/1/content/browser/loader/cross...
content/browser/loader/cross_site_resource_handler.cc:164: if (!swap_needed ||
info->IsDownload() || info->is_stream() ||
It's possible we should just set the IsDownload bit on streams too. I went with
this just because ResourceLoader::CancelRequestInternal also checks both.

[davidben, 2014-02-12 19:58:56]

https://codereview.chromium.org/161003002/diff/1/content/browser/loader/buffe...
File content/browser/loader/buffered_resource_handler.cc (left):

https://codereview.chromium.org/161003002/diff/1/content/browser/loader/buffe...
content/browser/loader/buffered_resource_handler.cc:375: // TODO(davidben):
These DCHECKs do actually trigger.
(Removed this comment and replaced it with the other since I added it in
response to hitting the x-x509-user-cert case. Forgot to look into it until
now.)

[not at google - send to devlin, 2014-02-12 20:47:42]

could you make zork@ the reviewer for the extensions portion of this? I don't
know the code.

[davidben, 2014-02-12 20:54:31]

On 2014/02/12 20:47:42, kalman wrote:
> could you make zork@ the reviewer for the extensions portion of this? I don't
> know the code.

I picked you because you're an OWNER (doesn't look like zork@ is), but if you'll
OWNERS-stamp it afterwards, sure. I just copied one of the unit tests and added
an extra navigation to it.

[Zachary Kuznia, 2014-02-12 22:04:48]

lgtm

https://codereview.chromium.org/161003002/diff/1/content/browser/loader/cross...
File content/browser/loader/cross_site_resource_handler.cc (right):

https://codereview.chromium.org/161003002/diff/1/content/browser/loader/cross...
content/browser/loader/cross_site_resource_handler.cc:164: if (!swap_needed ||
info->IsDownload() || info->is_stream() ||
On 2014/02/12 19:57:56, David Benjamin wrote:
> It's possible we should just set the IsDownload bit on streams too. I went
with
> this just because ResourceLoader::CancelRequestInternal also checks both.

IsDownload and is_stream should be mutually exclusive, so this should be the
right approach.

[davidben, 2014-02-12 22:18:55]

https://codereview.chromium.org/161003002/diff/1/content/browser/loader/cross...
File content/browser/loader/cross_site_resource_handler.cc (right):

https://codereview.chromium.org/161003002/diff/1/content/browser/loader/cross...
content/browser/loader/cross_site_resource_handler.cc:164: if (!swap_needed ||
info->IsDownload() || info->is_stream() ||
On 2014/02/12 22:04:48, Zachary Kuznia wrote:
> On 2014/02/12 19:57:56, David Benjamin wrote:
> > It's possible we should just set the IsDownload bit on streams too. I went
> with
> > this just because ResourceLoader::CancelRequestInternal also checks both.
> 
> IsDownload and is_stream should be mutually exclusive, so this should be the
> right approach.

Right, but the question is whether consumers of the ResourceRequestInfo should
care about the distinction. Maybe we should remove is_stream() and always set
IsDownload() when the BRH detaches the handler. The current approach is
certainly more error-prone, as this bug demonstrates.

Checking on codesearch, everything that checks for is_stream() also checks for
IsDownload(). Everything that checks for IsDownload() also checks for
is_stream() except this code (a bug) and AppUrlRedirector which... couldn't
check is_stream() if it wanted to because it's not exported. But it doesn't care
either way because it only queries when a request starts. Haven't looked closely
at what it does and whether it should care.

[Charlie Reis, 2014-02-13 01:45:07]

Nice find!  LGTM, though I'd like to find a way to unify is_stream() and
IsDownload if possible.

> creis, do you think we'll want to merge this? The DCHECK isn't in release
> builds, but I imagine confusing CSRH and friends into thinking there's a
> cross-site transition in progress when there isn't would muck things up.

It does seem like it could land us in some nasty situations, but merging late in
the game is a high bar.  Let's land this first and then ask laforge with some
examples of what currently fails.

https://codereview.chromium.org/161003002/diff/1/content/browser/loader/cross...
File content/browser/loader/cross_site_resource_handler.cc (right):

https://codereview.chromium.org/161003002/diff/1/content/browser/loader/cross...
content/browser/loader/cross_site_resource_handler.cc:164: if (!swap_needed ||
info->IsDownload() || info->is_stream() ||
On 2014/02/12 22:18:56, David Benjamin wrote:
> On 2014/02/12 22:04:48, Zachary Kuznia wrote:
> > On 2014/02/12 19:57:56, David Benjamin wrote:
> > > It's possible we should just set the IsDownload bit on streams too. I went
> > with
> > > this just because ResourceLoader::CancelRequestInternal also checks both.
> > 
> > IsDownload and is_stream should be mutually exclusive, so this should be the
> > right approach.
> 
> Right, but the question is whether consumers of the ResourceRequestInfo should
> care about the distinction. Maybe we should remove is_stream() and always set
> IsDownload() when the BRH detaches the handler. The current approach is
> certainly more error-prone, as this bug demonstrates.
> 
> Checking on codesearch, everything that checks for is_stream() also checks for
> IsDownload(). Everything that checks for IsDownload() also checks for
> is_stream() except this code (a bug) and AppUrlRedirector which... couldn't
> check is_stream() if it wanted to because it's not exported. But it doesn't
care
> either way because it only queries when a request starts. Haven't looked
closely
> at what it does and whether it should care.

I agree that the current approach is error prone.  It might be nice to have a
method that covers these similar cases if there's a concept that be used to
describe all of them (or their inverse, something like "IsDocument" or
"WillCommit").

In this case, whether the network response will commit as a new navigation is
the distinction the code is looking for.  Do the other callers of IsDownload()
and is_stream() need to care about 204 responses, since they lead to a similar
no-new-commit outcome?

[not at google - send to devlin, 2014-02-13 02:48:46]

rubberstamp lgtm for extensions

zork could you add yourself as an OWNER in the streams private directory?

[davidben, 2014-02-13 03:48:27]

https://codereview.chromium.org/161003002/diff/1/content/browser/loader/cross...
File content/browser/loader/cross_site_resource_handler.cc (right):

https://codereview.chromium.org/161003002/diff/1/content/browser/loader/cross...
content/browser/loader/cross_site_resource_handler.cc:164: if (!swap_needed ||
info->IsDownload() || info->is_stream() ||
On 2014/02/13 01:45:07, Charlie Reis wrote:
> On 2014/02/12 22:18:56, David Benjamin wrote:
> > On 2014/02/12 22:04:48, Zachary Kuznia wrote:
> > > On 2014/02/12 19:57:56, David Benjamin wrote:
> > > > It's possible we should just set the IsDownload bit on streams too. I
went
> > > with
> > > > this just because ResourceLoader::CancelRequestInternal also checks
both.
> > > 
> > > IsDownload and is_stream should be mutually exclusive, so this should be
the
> > > right approach.
> > 
> > Right, but the question is whether consumers of the ResourceRequestInfo
should
> > care about the distinction. Maybe we should remove is_stream() and always
set
> > IsDownload() when the BRH detaches the handler. The current approach is
> > certainly more error-prone, as this bug demonstrates.
> > 
> > Checking on codesearch, everything that checks for is_stream() also checks
for
> > IsDownload(). Everything that checks for IsDownload() also checks for
> > is_stream() except this code (a bug) and AppUrlRedirector which... couldn't
> > check is_stream() if it wanted to because it's not exported. But it doesn't
> care
> > either way because it only queries when a request starts. Haven't looked
> closely
> > at what it does and whether it should care.
> 
> I agree that the current approach is error prone.  It might be nice to have a
> method that covers these similar cases if there's a concept that be used to
> describe all of them (or their inverse, something like "IsDocument" or
> "WillCommit").
> 
> In this case, whether the network response will commit as a new navigation is
> the distinction the code is looking for.  Do the other callers of IsDownload()
> and is_stream() need to care about 204 responses, since they lead to a similar
> no-new-commit outcome?

The other callers seem to be more interested in the detaching aspect. There's
logic to allow them to outlive the route, and logic to ignore the renderer's
cancellations. And there's AppUrlRedirector which I still need to look at.

Calling a stream a "download" seems fairly sound to me. I'm already lumping
x-x509-user-cert here under "download" on grounds that it's a download with a
funny handler. Could we claim a stream is one too? Maybe Download isn't the best
word... "detached"? Detached is already used for DetachableResourceHandler, but
it's pretty similar actually. Maybe we could unifying those codepaths too, I
dunno. "Redirected" is the other word that comes to mind, but that also already
means something else. :-)

[davidben, 2014-02-13 18:08:13]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-02-13 18:09:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidben@chromium.org/161003002/190001

[Charlie Reis, 2014-02-13 19:33:04]

https://codereview.chromium.org/161003002/diff/1/content/browser/loader/cross...
File content/browser/loader/cross_site_resource_handler.cc (right):

https://codereview.chromium.org/161003002/diff/1/content/browser/loader/cross...
content/browser/loader/cross_site_resource_handler.cc:164: if (!swap_needed ||
info->IsDownload() || info->is_stream() ||
On 2014/02/13 03:48:28, David Benjamin wrote:
> On 2014/02/13 01:45:07, Charlie Reis wrote:
> > On 2014/02/12 22:18:56, David Benjamin wrote:
> > > On 2014/02/12 22:04:48, Zachary Kuznia wrote:
> > > > On 2014/02/12 19:57:56, David Benjamin wrote:
> > > > > It's possible we should just set the IsDownload bit on streams too. I
> went
> > > > with
> > > > > this just because ResourceLoader::CancelRequestInternal also checks
> both.
> > > > 
> > > > IsDownload and is_stream should be mutually exclusive, so this should be
> the
> > > > right approach.
> > > 
> > > Right, but the question is whether consumers of the ResourceRequestInfo
> should
> > > care about the distinction. Maybe we should remove is_stream() and always
> set
> > > IsDownload() when the BRH detaches the handler. The current approach is
> > > certainly more error-prone, as this bug demonstrates.
> > > 
> > > Checking on codesearch, everything that checks for is_stream() also checks
> for
> > > IsDownload(). Everything that checks for IsDownload() also checks for
> > > is_stream() except this code (a bug) and AppUrlRedirector which...
couldn't
> > > check is_stream() if it wanted to because it's not exported. But it
doesn't
> > care
> > > either way because it only queries when a request starts. Haven't looked
> > closely
> > > at what it does and whether it should care.
> > 
> > I agree that the current approach is error prone.  It might be nice to have
a
> > method that covers these similar cases if there's a concept that be used to
> > describe all of them (or their inverse, something like "IsDocument" or
> > "WillCommit").
> > 
> > In this case, whether the network response will commit as a new navigation
is
> > the distinction the code is looking for.  Do the other callers of
IsDownload()
> > and is_stream() need to care about 204 responses, since they lead to a
similar
> > no-new-commit outcome?
> 
> The other callers seem to be more interested in the detaching aspect. There's
> logic to allow them to outlive the route, and logic to ignore the renderer's
> cancellations. And there's AppUrlRedirector which I still need to look at.
> 
> Calling a stream a "download" seems fairly sound to me. I'm already lumping
> x-x509-user-cert here under "download" on grounds that it's a download with a
> funny handler. Could we claim a stream is one too? Maybe Download isn't the
best
> word... "detached"? Detached is already used for DetachableResourceHandler,
but
> it's pretty similar actually. Maybe we could unifying those codepaths too, I
> dunno. "Redirected" is the other word that comes to mind, but that also
already
> means something else. :-)

I'll defer to networking folks on that decision, since I don't have a great
sense for the behavior for streams, certificates, or what detaching means.  I'm
ok with lumping it all as downloads unless there's a better name for it. 
Certainly makes sense to do that as a followup CL.

[commit-bot: I haz the power, 2014-02-13 19:55:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidben@chromium.org/161003002/190001

[commit-bot: I haz the power, 2014-02-13 21:38:09]

Change committed as 251132