DocumentThreadableLoader: Call clearResource() when rejecting redirect

Source/core/loader/DocumentThreadableLoader.cpp

 /*
  * Copyright (C) 2011, 2012 Google Inc. All rights reserved.
  * Copyright (C) 2013, Intel Corporation
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
@@ skipping 218 matching lines @@
     m_client = 0;
     m_requestStartedSeconds = 0.0;
 }
 
 void DocumentThreadableLoader::setDefersLoading(bool value)
 {
     if (resource())
         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);
     ASSERT_UNUSED(resource, resource == this->resource());
 
     RefPtr<DocumentThreadableLoader> protect(this);
 
     // 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;
     }
 
     // Allow same origin requests to continue after allowing clients to audit the redirect.
     if (isAllowedRedirect(request.url())) {
         if (m_client->isDocumentThreadableLoaderClient())
             static_cast<DocumentThreadableLoaderClient*>(m_client)->willSendRequest(request, redirectResponse);
         return;
     }
@@ skipping 46 matching lines @@
             }
             makeCrossOriginAccessRequest(request);
             return;
         }
 
         ResourceError error(errorDomainBlinkInternal, 0, redirectResponse.url().string(), accessControlErrorDescription);
         m_client->didFailAccessControlCheck(error);
     } else {
         m_client->didFailRedirectCheck();
     }
+
+    clearResource();
     request = ResourceRequest();
+
     m_requestStartedSeconds = 0.0;
 }
 
 void DocumentThreadableLoader::dataSent(Resource* resource, unsigned long long bytesSent, unsigned long long totalBytesToBeSent)
 {
     ASSERT(m_client);
     ASSERT_UNUSED(resource, resource == this->resource());
     m_client->didSendData(bytesSent, totalBytesToBeSent);
 }
 
@@ skipping 263 matching lines @@
         return DoNotAllowStoredCredentials;
     return m_resourceLoaderOptions.allowCredentials;
 }
 
 SecurityOrigin* DocumentThreadableLoader::securityOrigin() const
 {
     return m_securityOrigin ? m_securityOrigin.get() : m_document.securityOrigin();
 }
 
 } // namespace blink