Clean up response handling in ResourceLoader/ResourceFetcher

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

 /*
  * Copyright (C) 2006, 2007, 2010, 2011 Apple Inc. All rights reserved.
  *           (C) 2007 Graham Dennis (graham.dennis@gmail.com)
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
@@ skipping 75 matching lines @@
     m_loader->setDefersLoading(defersLoading);
     ASSERT(m_loader);
     m_loader->setLoadingTaskRunner(loadingTaskRunner);
 
     if (m_resource->options().synchronousPolicy == RequestSynchronously)
         requestSynchronously(request);
     else
         m_loader->loadAsynchronously(WrappedResourceRequest(request), this);
 }
 
+void ResourceLoader::restart(const ResourceRequest& request)

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

I think the name of |restart| is too general, because this function cannot
restart loading of any Resource, and thus need renaming and/or comments.
(In this case, restart() is called before Resource::responseReceived() (and
before/without Resource::willFollowRedirect()?) and thus we can restart loading
by simply restarting WebURLLoader, not touching Resource's state.)

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

Renamed to restartForServiceWorkerFallback.

+{
+    m_loader.reset();
+    m_loader = wrapUnique(Platform::current()->createURLLoader());
+    DCHECK(m_loader);
+    m_loader->loadAsynchronously(WrappedResourceRequest(request), this);
+}
+
 void ResourceLoader::setDefersLoading(bool defers)
 {
     ASSERT(m_loader);
     m_loader->setDefersLoading(defers);
 }
 
 void ResourceLoader::didDownloadData(WebURLLoader*, int length, int encodedDataLength)
 {
     m_fetcher->didDownloadData(m_resource.get(), length, encodedDataLength);
     m_resource->didDownloadData(length);
@@ skipping 31 matching lines @@
 void ResourceLoader::didReceiveCachedMetadata(WebURLLoader*, const char* data, int length)
 {
     m_resource->setSerializedCachedMetadata(data, length);
 }
 
 void ResourceLoader::didSendData(WebURLLoader*, unsigned long long bytesSent, unsigned long long totalBytesToBeSent)
 {
     m_resource->didSendData(bytesSent, totalBytesToBeSent);
 }
 
-bool ResourceLoader::responseNeedsAccessControlCheck() const
-{
-    // If the fetch was (potentially) CORS enabled, an access control check of the response is required.
-    return m_resource->options().corsEnabled == IsCORSEnabled;
-}
-
 void ResourceLoader::didReceiveResponse(WebURLLoader*, const WebURLResponse& response, WebDataConsumerHandle* rawHandle)
 {
     DCHECK(!response.isNull());
     // |rawHandle|'s ownership is transferred to the callee.
     std::unique_ptr<WebDataConsumerHandle> handle = wrapUnique(rawHandle);
     const ResourceResponse& resourceResponse = response.toResourceResponse();
-
-    if (responseNeedsAccessControlCheck()) {
-        if (response.wasFetchedViaServiceWorker()) {
-            if (response.wasFallbackRequiredByServiceWorker()) {
-                m_loader.reset();
-                m_loader = wrapUnique(Platform::current()->createURLLoader());
-                DCHECK(m_loader);
-                ResourceRequest request = m_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);
-                m_loader->loadAsynchronously(WrappedResourceRequest(request), this);
-                return;
-            }
-        } else {
-            if (!m_resource->isCacheValidator() || resourceResponse.httpStatusCode() != 304)
-                m_resource->setResponse(resourceResponse);

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

This CL removes this setResponse(). It's probably OK because
Resource::responseReceived() sets response.

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

Mostly correct. The only reason it needed to be here was because
canAccessResource() expected Resource::response() to be valid. I refactored
canAccessResource() and renamed it to canAccessResponse(), and removed this
expectation.

-            if (!m_fetcher->canAccessResource(m_resource.get(), m_resource->options().securityOrigin.get(), response.url(), ResourceFetcher::ShouldLogAccessControlErrors)) {
-                m_fetcher->didReceiveResponse(m_resource.get(), resourceResponse);
-                didFail(nullptr, ResourceError::cancelledDueToAccessCheckError(KURL(response.url())));
-                return;
-            }
-        }
-    }
-
+    if (!m_fetcher->didReceiveResponse(m_resource.get(), resourceResponse))
+        return;
     m_resource->responseReceived(resourceResponse, std::move(handle));
-    if (!m_loader)
-        return;
-
-    m_fetcher->didReceiveResponse(m_resource.get(), resourceResponse);
-    if (!m_loader)
-        return;
-
-    if (m_resource->response().httpStatusCode() < 400 || m_resource->shouldIgnoreHTTPStatusCodeErrors())
-        return;
-    didFail(nullptr, ResourceError::cancelledError(resourceResponse.url()));
+    if (response.httpStatusCode() >= 400 && !m_resource->shouldIgnoreHTTPStatusCodeErrors())

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

Do we need |if (!m_loader) return;| before this?

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

This moved to ResourceFetcher, but add a resource->loader() nullcheck there.

+        didFail(nullptr, ResourceError::cancelledError(resourceResponse.url()));
 }
 
 void ResourceLoader::didReceiveResponse(WebURLLoader* loader, const WebURLResponse& response)
 {
     didReceiveResponse(loader, response, nullptr);
 }
 
 void ResourceLoader::didReceiveData(WebURLLoader*, const char* data, int length, int encodedDataLength, int encodedBodyLength)
 {
     RELEASE_ASSERT(length >= 0);
@@ skipping 53 matching lines @@
     // empty buffer is a noop in most cases, but is destructive in the case of
     // a 304, where it will overwrite the cached data we should be reusing.
     if (dataOut.size()) {
         m_fetcher->didReceiveData(m_resource.get(), dataOut.data(), dataOut.size(), encodedDataLength);
         m_resource->setResourceBuffer(dataOut);
     }
     didFinishLoading(0, monotonicallyIncreasingTime(), encodedDataLength);
 }
 
 } // namespace blink