DocumentThreadableLoader: Call clearResource() when rejecting redirect

DocumentThreadableLoader: Call clearResource() when rejecting redirect

Also add comments to explain how clearing passed request affects redirect
handling

R=japhet,mkwst
BUG=none

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186399

[tyoshino (SeeGerritForStatus), 2014-10-24 11:07:01]

tyoshino@chromium.org changed reviewers:
+ japhet@chromium.org

[tyoshino (SeeGerritForStatus), 2014-10-24 11:10:58]

FYI, the behavior this cancelling relies on was introduced to WebURLLoaderImpl
by this change.

https://chromium.googlesource.com/chromium/src/+/6568a9e384e0f92422c68d4f31fb...

[Nate Chapin, 2014-10-24 17:09:31]

https://codereview.chromium.org/643733003/diff/20001/Source/core/loader/Docum...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/643733003/diff/20001/Source/core/loader/Docum...
Source/core/loader/DocumentThreadableLoader.cpp:256: clearResource();
If we call clearResource(), can we get ride of clearing |request| ?

I think this is the only place that cancels loads by clearing the
ResourceRequest. If we can remove the "request = ResourceRequest();" lines, we
should also remove the code that handles it in ResourceLoader::willSendRequest.

[tyoshino (SeeGerritForStatus), 2014-10-27 09:04:08]

https://codereview.chromium.org/643733003/diff/20001/Source/core/loader/Docum...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/643733003/diff/20001/Source/core/loader/Docum...
Source/core/loader/DocumentThreadableLoader.cpp:256: clearResource();
On 2014/10/24 17:09:31, Nate Chapin wrote:
> If we call clearResource(), can we get ride of clearing |request| ?

If the resource has only one client (this loader), it'll be cancelled when we
call clearResource(). So, we don't need to do "request = ResourceRequest();".

Otherwise, i.e. if there're other ResourceOwners for the resource, it's possible
that the underlying stack continues loading after following the redirect. Even
in this case, this loader won't receive any callback beyond this point as far as
we call clearResource(). So, no worry about leaking information to the script
that should be protected in terms of CORS algorithm. So, the question is: is it
harmful (regarding CSP, CORS) to have the underlying stack to continue issuing a
request following the redirect.

It seems, ideally, if the client of a Resource want to control redirect, the
Resource shouldn't be shared by multiple clients.

> I think this is the only place that cancels loads by clearing the
> ResourceRequest. If we can remove the "request = ResourceRequest();" lines, we
> should also remove the code that handles it in
ResourceLoader::willSendRequest.

[Nate Chapin, 2014-10-29 23:37:26]

https://codereview.chromium.org/643733003/diff/20001/Source/core/loader/Docum...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/643733003/diff/20001/Source/core/loader/Docum...
Source/core/loader/DocumentThreadableLoader.cpp:256: clearResource();
On 2014/10/27 09:04:08, tyoshino wrote:
> On 2014/10/24 17:09:31, Nate Chapin wrote:
> > If we call clearResource(), can we get ride of clearing |request| ?
> 
> If the resource has only one client (this loader), it'll be cancelled when we
> call clearResource(). So, we don't need to do "request = ResourceRequest();".
> 
> Otherwise, i.e. if there're other ResourceOwners for the resource, it's
possible
> that the underlying stack continues loading after following the redirect. Even
> in this case, this loader won't receive any callback beyond this point as far
as
> we call clearResource(). So, no worry about leaking information to the script
> that should be protected in terms of CORS algorithm. So, the question is: is
it
> harmful (regarding CSP, CORS) to have the underlying stack to continue issuing
a
> request following the redirect.
> 
> It seems, ideally, if the client of a Resource want to control redirect, the
> Resource shouldn't be shared by multiple clients.
> 
> > I think this is the only place that cancels loads by clearing the
> > ResourceRequest. If we can remove the "request = ResourceRequest();" lines,
we
> > should also remove the code that handles it in
> ResourceLoader::willSendRequest.
> 

I think it's probably ok to let the request continue. CORS should certainly be
safe, since we only permit clients to share are resource if their corsEnabled
flags are equal. I think CSP is similarly enforced?

[tyoshino (SeeGerritForStatus), 2014-11-05 13:36:10]

https://codereview.chromium.org/643733003/diff/20001/Source/core/loader/Docum...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/643733003/diff/20001/Source/core/loader/Docum...
Source/core/loader/DocumentThreadableLoader.cpp:256: clearResource();
On 2014/10/29 23:37:25, Nate Chapin wrote:
> I think it's probably ok to let the request continue. CORS should certainly be
> safe, since we only permit clients to share are resource if their corsEnabled
> flags are equal. I think CSP is similarly enforced?

canReuseRequest() currently check only corsEnabled. There's FIXME for mixed
content.

Anyway, as far as there's at least one client that proceeds with redirect, it
sounds fine to me to continue loading even if one client drops.

But I'm worried about forPreload() check and m_allowStaleResources check that
precede canReuseRequest() check in determineRevalidationPolicy().

existingResource->canReuse() check ensures clients on the same Resource shares
the same headers. But allowStoredCredentials() check should be moved to earlier
position? There some Use decisions are made before the allowStoredCredentials()
check.

[Nate Chapin, 2014-11-06 18:06:56]

On 2014/11/05 13:36:10, tyoshino wrote:
>
https://codereview.chromium.org/643733003/diff/20001/Source/core/loader/Docum...
> File Source/core/loader/DocumentThreadableLoader.cpp (right):
> 
>
https://codereview.chromium.org/643733003/diff/20001/Source/core/loader/Docum...
> Source/core/loader/DocumentThreadableLoader.cpp:256: clearResource();
> On 2014/10/29 23:37:25, Nate Chapin wrote:
> > I think it's probably ok to let the request continue. CORS should certainly
be
> > safe, since we only permit clients to share are resource if their
corsEnabled
> > flags are equal. I think CSP is similarly enforced?
> 
> canReuseRequest() currently check only corsEnabled. There's FIXME for mixed
> content.
> 
> Anyway, as far as there's at least one client that proceeds with redirect, it
> sounds fine to me to continue loading even if one client drops.
> 
> But I'm worried about forPreload() check and m_allowStaleResources check that
> precede canReuseRequest() check in determineRevalidationPolicy().
> 
> existingResource->canReuse() check ensures clients on the same Resource shares
> the same headers. But allowStoredCredentials() check should be moved to
earlier
> position? There some Use decisions are made before the
allowStoredCredentials()
> check.

I don't think forPreload() is relevant. I would be very surprised if we ever
preloaded something with security implications. Besides, I don't think (but I'm
not sure) that we ever preload raw resources.

In terms of m_allowStaleResources, I agree it's scary. I would if we should
change that clause to if (m_allowStaleResources &&
m_validatedURLs.contains(existingResource->url())). That should still get us the
right behavior for resources that were already loaded by the current document,
while avoiding arbitrary cache reuse.

Agreed that allowStoredCredentials() be earlier.

[tyoshino (SeeGerritForStatus), 2014-11-10 07:36:34]

On 2014/11/06 18:06:56, Nate Chapin wrote:
> I don't think forPreload() is relevant. I would be very surprised if we ever

HTMLPreloadScanner processes "crossorigin" attribute(s) in script, img and link
elements. A PreloadRequest created by HTMLPreloadScanner and passed to the
specified HTMLResourcePreloader has m_isCORSEnabled and m_allowCredentials set
accordingly. Using them, PreloadRequest::resourceRequest() calls
FetchRequest::setCrossOriginAccessControl() to use CORS.

> preloaded something with security implications. Besides, I don't think (but
I'm
> not sure) that we ever preload raw resources.

Ah, right.
- We call setUrlToLoad only for some m_tagImpl values
(TokenPreloadScanner::StartTagScanner ctor)
- TokenPreloadScanner::StartTagScanner() adjusts m_tagImpl
- TokenPreloadScanner::resourceType() generates Resource::Type only for the
allowed tags

They're Script, Image and CSSStyleSheet. No Raw.

So, forPreload() is not relevant in terms of this change.

[Nate Chapin, 2014-11-10 21:32:57]

On 2014/11/10 07:36:34, tyoshino wrote:
> On 2014/11/06 18:06:56, Nate Chapin wrote:
> > I don't think forPreload() is relevant. I would be very surprised if we ever
> 
> HTMLPreloadScanner processes "crossorigin" attribute(s) in script, img and
link
> elements. A PreloadRequest created by HTMLPreloadScanner and passed to the
> specified HTMLResourcePreloader has m_isCORSEnabled and m_allowCredentials set
> accordingly. Using them, PreloadRequest::resourceRequest() calls
> FetchRequest::setCrossOriginAccessControl() to use CORS.
> 
> > preloaded something with security implications. Besides, I don't think (but
> I'm
> > not sure) that we ever preload raw resources.
> 
> Ah, right.
> - We call setUrlToLoad only for some m_tagImpl values
> (TokenPreloadScanner::StartTagScanner ctor)
> - TokenPreloadScanner::StartTagScanner() adjusts m_tagImpl
> - TokenPreloadScanner::resourceType() generates Resource::Type only for the
> allowed tags
> 
> They're Script, Image and CSSStyleSheet. No Raw.
> 
> So, forPreload() is not relevant in terms of this change.

Anyway, LGTM if you want to submit it this way. I'd slightly prefer dropping the
ResourceRequest nulling, but I'm not going to fight too hard for it :)

[tyoshino (SeeGerritForStatus), 2014-11-13 07:55:32]

On 2014/11/10 21:32:57, Nate Chapin wrote:
> On 2014/11/10 07:36:34, tyoshino wrote:
> > On 2014/11/06 18:06:56, Nate Chapin wrote:
> > > I don't think forPreload() is relevant. I would be very surprised if we
ever
> > 
> > HTMLPreloadScanner processes "crossorigin" attribute(s) in script, img and
> link
> > elements. A PreloadRequest created by HTMLPreloadScanner and passed to the
> > specified HTMLResourcePreloader has m_isCORSEnabled and m_allowCredentials
set
> > accordingly. Using them, PreloadRequest::resourceRequest() calls
> > FetchRequest::setCrossOriginAccessControl() to use CORS.
> > 
> > > preloaded something with security implications. Besides, I don't think
(but
> > I'm
> > > not sure) that we ever preload raw resources.
> > 
> > Ah, right.
> > - We call setUrlToLoad only for some m_tagImpl values
> > (TokenPreloadScanner::StartTagScanner ctor)
> > - TokenPreloadScanner::StartTagScanner() adjusts m_tagImpl
> > - TokenPreloadScanner::resourceType() generates Resource::Type only for the
> > allowed tags
> > 
> > They're Script, Image and CSSStyleSheet. No Raw.
> > 
> > So, forPreload() is not relevant in terms of this change.
> 
> Anyway, LGTM if you want to submit it this way. I'd slightly prefer dropping
the
> ResourceRequest nulling, but I'm not going to fight too hard for it :)

Thanks. Filed a bug to work on later
http://crbug.com/432820

[tyoshino (SeeGerritForStatus), 2014-11-13 07:55:52]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2014-11-13 07:56:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/643733003/60001

[commit-bot: I haz the power, 2014-11-13 08:07:34]

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

[commit-bot: I haz the power, 2014-11-13 08:07:35]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)

[tyoshino (SeeGerritForStatus), 2014-11-13 08:45:47]

Hi Nate,

I forgot to run AssociatedURLLoaderTest. It has invalid expectation. The
TestBlinkWebUnitTestSupport an implementation of BlinkPlatformImpl used for this
test returns the WebURLLoaderMock which behaves differently from
WebURLLoaderImpl. WebURLLoaderImpl stops loading when the WebURLRequest is
cleared, but WebURLLoaderMock doesn't. So, it continues to call the handlers for
successful loading. The EXPECT_-s in
AssociatedURLLoaderTest.RedirectCrossOriginFailure expected that mistakenly.

PTAL.

[tyoshino (SeeGerritForStatus), 2014-11-27 05:29:00]

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

[tyoshino (SeeGerritForStatus), 2014-11-27 05:29:19]

Mike, could you please take a look at AssociatedURLLoaderTest.cpp?

[Mike West, 2014-11-27 08:53:57]

LGTM.

[tyoshino (SeeGerritForStatus), 2014-12-03 08:30:34]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2014-12-03 08:31:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/643733003/100001

[commit-bot: I haz the power, 2014-12-03 09:46:34]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186399