DocumentThreadableLoader: Call clearResource() when rejecting redirect

Source/core/loader/DocumentThreadableLoader.cpp

Index: Source/core/loader/DocumentThreadableLoader.cpp
diff --git a/Source/core/loader/DocumentThreadableLoader.cpp b/Source/core/loader/DocumentThreadableLoader.cpp
index 3cdd83fde895b57f08203a6de78b8e26fdb8ef44..674d6bbac9ba750ae5a6cc288b4bedb456e810bb 100644
--- a/Source/core/loader/DocumentThreadableLoader.cpp
+++ b/Source/core/loader/DocumentThreadableLoader.cpp
@@ -236,6 +236,12 @@ void DocumentThreadableLoader::setDefersLoading(bool value)
         resource()->setDefersLoading(value);
 }
 
+// In this method, we can clear |request| to tell content::WebURLLoaderImpl of
+// Chromium not to follow the redirect. This works only when this method is
+// called by RawResource::willSendRequest(). If called by
+// RawResource::didAddClient(), clearing |request| won't be propagated
+// to content::WebURLLoaderImpl. So, this loader must also get detached from
+// the resource by calling clearResource().
 void DocumentThreadableLoader::redirectReceived(Resource* resource, ResourceRequest& request, const ResourceResponse& redirectResponse)
 {
     ASSERT(m_client);
@@ -246,13 +252,19 @@ void DocumentThreadableLoader::redirectReceived(Resource* resource, ResourceRequ
     // FIXME: Support redirect in Fetch API.
     if (resource->resourceRequest().requestContext() == blink::WebURLRequest::RequestContextFetch) {
         m_client->didFailRedirectCheck();
+
+        clearResource();

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

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]

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.

[Nate Chapin, 2014/10/29 23:37:25]

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:09]

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.

         request = ResourceRequest();
+
         return;
     }
 
     if (!isAllowedByPolicy(request.url())) {
         m_client->didFailRedirectCheck();
+
+        clearResource();
         request = ResourceRequest();
+
         m_requestStartedSeconds = 0.0;
         return;
     }
@@ -319,7 +331,10 @@ void DocumentThreadableLoader::redirectReceived(Resource* resource, ResourceRequ
     } else {
         m_client->didFailRedirectCheck();
     }
+
+    clearResource();
     request = ResourceRequest();
+
     m_requestStartedSeconds = 0.0;
 }