Clean up response handling in ResourceLoader/ResourceFetcher

third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp

Index: third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
diff --git a/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp b/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
index dd07563a5e11cf9cc61916fcb27957720a7dd178..6264bb2ad99d32653b4ac7e6869ff9deff729a7e 100644
--- a/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
+++ b/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
@@ -239,25 +239,28 @@ Resource* ResourceFetcher::cachedResource(const KURL& resourceURL) const
     return resource.get();
 }
 
-bool ResourceFetcher::canAccessResource(Resource* resource, SecurityOrigin* sourceOrigin, const KURL& url, AccessControlLoggingDecision logErrorsDecision) const

[hiroshige, 2016/07/28 09:56:42]

This CL removes AccessControlLoggingDecision because it is always
ShouldLogAccessControlErrors.
Can we split changes related to this to a separate CL?

[Nate Chapin, 2016/07/28 22:29:35]

Ok, will do.

+bool ResourceFetcher::canAccessResponse(Resource* resource, const ResourceResponse& response) const
 {
     // Redirects can change the response URL different from one of request.
     bool forPreload = resource->isUnusedPreload();
-    if (!context().canRequest(resource->getType(), resource->resourceRequest(), url, resource->options(), forPreload, FetchRequest::UseDefaultOriginRestrictionForType))
+    if (!context().canRequest(resource->getType(), resource->resourceRequest(), response.url(), resource->options(), forPreload, FetchRequest::UseDefaultOriginRestrictionForType))
         return false;
 
+    SecurityOrigin* sourceOrigin = resource->options().securityOrigin.get();
     if (!sourceOrigin)
         sourceOrigin = context().getSecurityOrigin();
 
-    if (sourceOrigin->canRequestNoSuborigin(url))
+    if (sourceOrigin->canRequestNoSuborigin(response.url()))
         return true;
 
+    // Use the original response instead of the 304 response for a successful revaldiation.
+    const ResourceResponse& responseForAccessControl = response.httpStatusCode() == 304 ? resource->response() : response;

[hiroshige, 2016/07/28 09:56:42]

We need to check m_resource->isCacheValidator() here, because a server can
return 304 for non-revalidating request (such server is not well behaving
though).

[Nate Chapin, 2016/07/28 22:29:35]

Oops, I don't remember dropping that. Done.

     String errorDescription;
-    if (!resource->passesAccessControlCheck(sourceOrigin, errorDescription)) {
+    if (!passesAccessControlCheck(responseForAccessControl, resource->options().allowCredentials, sourceOrigin, errorDescription, resource->lastResourceRequest().requestContext())) {

[hiroshige, 2016/07/28 09:56:42]

This replaces ResourceRequest::allowStoredCredentials() with
ResourceLoaderOptions::allowCredentials. I think it's good because they are
duplicated. Are we going to merge them further? (If so, let's create a tracking
crbug)

Placing DCHECK_EQ(resource->options().allowCredentials,
resource->lastResourceRequest().allowStoredCredentials() ?
AllowStoredCredentials : DoNotAllowStoredCredentials) is good for catching
potential bugs.

[hiroshige, 2016/07/28 09:56:42]

Memo: This inlines Resource::passesAccessControlCheck().

[Nate Chapin, 2016/07/28 22:29:35]

Also, see comment about setReponse() call in
ResourceLoader::didReceiveResponse(). The old variant implicitly uses
Resource::m_response, which is not yet set in the non-304 case.

[Nate Chapin, 2016/07/28 22:29:35]

That may make sense, but some places don't use the StoredCredentials enum at
all, and it's not exposed outside of blink. ResourceFetcher always holds them to
be equivalent, so it seemed correct in this specific scenario.

         resource->setCORSFailed();
-        if (!forPreload && (logErrorsDecision == ShouldLogAccessControlErrors)) {
+        if (!forPreload) {
             String resourceType = Resource::resourceTypeToString(resource->getType(), resource->options().initiatorInfo);
-            context().addConsoleMessage(resourceType + " from origin '" + SecurityOrigin::create(url)->toString() + "' has been blocked from loading by Cross-Origin Resource Sharing policy: " + errorDescription);
+            context().addConsoleMessage(resourceType + " from origin '" + SecurityOrigin::create(response.url())->toString() + "' has been blocked from loading by Cross-Origin Resource Sharing policy: " + errorDescription);
         }
         return false;
     }
@@ -945,21 +948,39 @@ void ResourceFetcher::didFailLoading(Resource* resource, const ResourceError& er
     context().didLoadResource(resource);
 }
 
-void ResourceFetcher::didReceiveResponse(Resource* resource, const ResourceResponse& response)
+bool ResourceFetcher::didReceiveResponse(Resource* resource, const ResourceResponse& response)
 {
-    // If the response is fetched via ServiceWorker, the original URL of the response could be different from the URL of the request.
-    // We check the URL not to load the resources which are forbidden by the page CSP.
-    // https://w3c.github.io/webappsec-csp/#should-block-response
+    context().dispatchDidReceiveResponse(resource->identifier(), response, resource->resourceRequest().frameType(), resource->resourceRequest().requestContext(), resource);
+
     if (response.wasFetchedViaServiceWorker()) {
+        if (resource->options().corsEnabled == IsCORSEnabled && response.wasFallbackRequiredByServiceWorker()) {

[hiroshige, 2016/07/28 09:56:42]

memo for myself: this block is moved from the callee:
ResourceLoader::didReceiveResponse().

+            ResourceRequest request = resource->lastResourceRequest();
+            DCHECK_EQ(request.skipServiceWorker(), WebURLRequest::SkipServiceWorker::None);
+            // This code handles the case when a regular controlling service worker
+            // doesn't handle a cross origin request. When this happens we still
+            // want to give foreign fetch a chance to handle the request, so
+            // only skip the controlling service worker for the fallback request.
+            // This is currently safe because of http://crbug.com/604084 the
+            // wasFallbackRequiredByServiceWorker flag is never set when foreign fetch
+            // handled a request.
+            request.setSkipServiceWorker(WebURLRequest::SkipServiceWorker::Controlling);
+            resource->loader()->restart(request);
+            return false;
+        }
+
+        // If the response is fetched via ServiceWorker, the original URL of the response could be different from the URL of the request.
+        // We check the URL not to load the resources which are forbidden by the page CSP.
+        // https://w3c.github.io/webappsec-csp/#should-block-response
         const KURL& originalURL = response.originalURLViaServiceWorker();
         if (!originalURL.isEmpty() && !context().allowResponse(resource->getType(), resource->resourceRequest(), originalURL, resource->options())) {
-            resource->loader()->cancel();
-            bool isInternalRequest = resource->options().initiatorInfo.name == FetchInitiatorTypeNames::internal;
-            context().dispatchDidFail(resource->identifier(), ResourceError(errorDomainBlinkInternal, 0, originalURL.getString(), "Unsafe attempt to load URL " + originalURL.elidedString() + " fetched by a ServiceWorker."), isInternalRequest);
-            return;
+            resource->loader()->didFail(nullptr, ResourceError::cancelledDueToAccessCheckError(response.url()));
+            return false;
         }
+    } else if (resource->options().corsEnabled == IsCORSEnabled && !canAccessResponse(resource, response)) {
+        resource->loader()->didFail(nullptr, ResourceError::cancelledDueToAccessCheckError(response.url()));
+        return false;
     }
-    context().dispatchDidReceiveResponse(resource->identifier(), response, resource->resourceRequest().frameType(), resource->resourceRequest().requestContext(), resource);
+    return true;
 }
 
 void ResourceFetcher::didReceiveData(const Resource* resource, const char* data, int dataLength, int encodedDataLength)