Don't duplicate ResourceRequests and ResourceLoaderOptions on ResourceLoader

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 38 matching lines @@
 
 namespace {
 
 bool isManualRedirectFetchRequest(const ResourceRequest& request)
 {
     return request.fetchRedirectMode() == WebURLRequest::FetchRedirectModeManual && request.requestContext() == WebURLRequest::RequestContextFetch;
 }
 
 } // namespace
 
-ResourceLoader* ResourceLoader::create(ResourceFetcher* fetcher, Resource* resource, const ResourceRequest& request, const ResourceLoaderOptions& options)
+ResourceLoader* ResourceLoader::create(ResourceFetcher* fetcher, Resource* resource)
 {
-    ResourceLoader* loader = new ResourceLoader(fetcher, resource, options);
-    loader->init(request);
-    return loader;
+    return new ResourceLoader(fetcher, resource);
 }
 
-ResourceLoader::ResourceLoader(ResourceFetcher* fetcher, Resource* resource, const ResourceLoaderOptions& options)
+ResourceLoader::ResourceLoader(ResourceFetcher* fetcher, Resource* resource)
     : m_fetcher(fetcher)
     , m_notifiedLoadComplete(false)
     , m_loadingMultipartContent(false)
-    , m_options(options)
     , m_resource(resource)
     , m_state(ConnectionStateNew)
 {
     ASSERT(m_resource);
     ASSERT(m_fetcher);
+    m_fetcher->didInitializeResourceLoader(this);
 }
 
 ResourceLoader::~ResourceLoader()
 {
     ASSERT(m_state == ConnectionStateReleased);
 }
 
 DEFINE_TRACE(ResourceLoader)
 {
     visitor->trace(m_fetcher);
@@ skipping 12 matching lines @@
 
     ASSERT(m_state != ConnectionStateReleased);
     m_state = ConnectionStateReleased;
     if (m_loader) {
         m_loader->cancel();
         m_loader.clear();
     }
     m_fetcher.clear();
 }
 
-void ResourceLoader::init(const ResourceRequest& passedRequest)
-{
-    ResourceRequest request(passedRequest);
-    m_fetcher->willSendRequest(m_resource->identifier(), request, ResourceResponse(), m_options.initiatorInfo);
-    m_originalRequest = m_request = applyOptions(request);
-    m_resource->updateRequest(request);

[Nate Chapin, 2016/03/09 00:38:20]

This call of updateRequest() is a no-op. When a ResourceLoader is initialized,
there are typically no clients to listen to the notification.

[hiroshige, 2016/03/09 02:01:56]

I suspect this might not be a case when delayed loading is done in FontResource
etc., but Resource::updateRequest() is no-op except for RawResource, so
I think this is still true.

optional: we can add ASSERT() that m_resource is not RawResource or
!hasClients().

[Nate Chapin, 2016/03/09 22:35:54]

Agreed. I had reached the same conclusion, but in my rush to post this for
review yesterday, neglected to put it in writing.
If we are going to ASSERT that, I'd prefer to do it in Resource::load(). I'd
like to avoid teaching ResourceLoader more about Resource::Type, since yhirano@
is about to remove the last mention of them in his multipart cleanups :)

-    m_fetcher->didInitializeResourceLoader(this);
-}
-
-void ResourceLoader::start()
+void ResourceLoader::start(ResourceRequest& request)
 {
     ASSERT(!m_loader);
-    ASSERT(!m_request.isNull());
 
-    m_fetcher->willStartLoadingResource(m_resource.get(), m_request);
+    m_fetcher->willStartLoadingResource(m_resource.get(), request);
+    m_fetcher->willSendRequest(m_resource->identifier(), request, ResourceResponse(), m_resource->options().initiatorInfo);

[hiroshige, 2016/03/09 02:01:56]

This CL changes the order from:
- willSendRequest()
- didInitializeResourceLoader()
- willStartLoadingResource()
- willSendRequest() (for each redirect leg, if any)
to:
- didInitializeResourceLoader()
- willStartLoadingResource()
- willSendRequest()
- willSendRequest() (for each redirect leg, if any)
which is cleaner.
It is worth mentioned in the CL description.

optional: can we merge didInitializeResourceLoader() into
willStartLoadingResource()?
pros: fewer callbacks. no callbacks in the constructor.

[Nate Chapin, 2016/03/09 22:35:54]

I've done a bit more refactoring than that. ResourceLoader::start() and
ResourceLoader::willFollowRedirect() now have one main m_fetcher call each.

+    applyOptions(request);
     RELEASE_ASSERT(m_state == ConnectionStateNew);
     m_state = ConnectionStateStarted;
 
     m_loader = adoptPtr(Platform::current()->createURLLoader());
     m_loader->setDefersLoading(m_fetcher->defersLoading());
     ASSERT(m_loader);
     m_loader->setLoadingTaskRunner(m_fetcher->loadingTaskRunner());
 
-    if (m_options.synchronousPolicy == RequestSynchronously)
-        requestSynchronously();
+    if (m_resource->options().synchronousPolicy == RequestSynchronously)
+        requestSynchronously(request);
     else
-        m_loader->loadAsynchronously(WrappedResourceRequest(m_request), this);
-}
-
-void ResourceLoader::changeToSynchronous()
-{
-    ASSERT(m_options.synchronousPolicy == RequestAsynchronously);
-    ASSERT(m_loader);
-    m_loader->cancel();
-    m_loader.clear();
-    m_loader = adoptPtr(Platform::current()->createURLLoader());
-    ASSERT(m_loader);
-    m_request.setPriority(ResourceLoadPriorityHighest);
-    m_state = ConnectionStateStarted;
-    requestSynchronously();
+        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)
 {
@@ skipping 39 matching lines @@
 void ResourceLoader::cancel(const ResourceError& error)
 {
     // If the load has already completed - succeeded, failed, or previously cancelled - do nothing.
     if (m_state == ConnectionStateReleased)
         return;
     if (isFinishing()) {
         releaseResources();
         return;
     }
 
-    ResourceError nonNullError = error.isNull() ? ResourceError::cancelledError(m_request.url()) : error;
+    ResourceError nonNullError = error.isNull() ? ResourceError::cancelledError(m_resource->lastResourceRequest().url()) : error;
 
     WTF_LOG(ResourceLoading, "Cancelled load of '%s'.\n", m_resource->url().getString().latin1().data());
     m_state = ConnectionStateCanceled;
     m_resource->setResourceError(nonNullError);
 
     if (!m_notifiedLoadComplete) {
         m_notifiedLoadComplete = true;
         m_fetcher->didFailLoading(m_resource.get(), nonNullError);
     }
 
     if (m_state != ConnectionStateReleased)
         m_resource->error(Resource::LoadError);
     if (m_state != ConnectionStateReleased)
         releaseResources();
 }
 
 void ResourceLoader::willFollowRedirect(WebURLLoader*, WebURLRequest& passedNewRequest, const WebURLResponse& passedRedirectResponse)
 {
     ASSERT(m_state != ConnectionStateReleased);
 
     ResourceRequest& newRequest(applyOptions(passedNewRequest.toMutableResourceRequest()));
+    ResourceLoaderOptions options = m_resource->options();

[Nate Chapin, 2016/03/09 00:38:20]

m_resource->options() is const, hence the copy. Is it worth making it non-const
to avoid this and the second copy below where we call m_resource->setOptions()?

 
     ASSERT(!newRequest.isNull());
     const ResourceResponse& redirectResponse(passedRedirectResponse.toResourceResponse());
     ASSERT(!redirectResponse.isNull());
     newRequest.setFollowedRedirect(true);
-    if (!isManualRedirectFetchRequest(m_resource->resourceRequest()) && !m_fetcher->canAccessRedirect(m_resource.get(), newRequest, redirectResponse, m_options)) {
+    if (!isManualRedirectFetchRequest(m_resource->resourceRequest()) && !m_fetcher->canAccessRedirect(m_resource.get(), newRequest, redirectResponse, options)) {
         cancel(ResourceError::cancelledDueToAccessCheckError(newRequest.url()));
         return;
     }
     ASSERT(m_state != ConnectionStateReleased);
 
-    applyOptions(newRequest); // canAccessRedirect() can modify m_options so we should re-apply it.
+    m_resource->setOptions(options);
+    applyOptions(newRequest); // canAccessRedirect() can modify options so we should re-apply it.
     m_fetcher->redirectReceived(m_resource.get(), redirectResponse);
-    ASSERT(m_state != ConnectionStateReleased);
+    m_fetcher->willSendRequest(m_resource->identifier(), newRequest, redirectResponse, options.initiatorInfo);
     m_resource->willFollowRedirect(newRequest, redirectResponse);

[Nate Chapin, 2016/03/09 00:38:20]

By doing m_resource->willFollowRedirect() last, clients get the real, final
version of the ResourceRequest that will be sent, removing the need for the
updateRequest() call. Also, this is the function that might cancel the load
synchronously, so doing it last removes the need for early exits.

-    if (newRequest.isNull() || m_state == ConnectionStateReleased)
-        return;
-
-    m_fetcher->willSendRequest(m_resource->identifier(), newRequest, redirectResponse, m_options.initiatorInfo);
-    ASSERT(m_state != ConnectionStateReleased);
-    ASSERT(!newRequest.isNull());
-    m_resource->updateRequest(newRequest);
-    m_request = newRequest;
 }
 
 void ResourceLoader::didReceiveCachedMetadata(WebURLLoader*, const char* data, int length)
 {
     RELEASE_ASSERT(m_state == ConnectionStateReceivedResponse || m_state == ConnectionStateReceivingData);
     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_options.corsEnabled == IsCORSEnabled;
+    return m_resource->options().corsEnabled == IsCORSEnabled;
 }
 
 void ResourceLoader::didReceiveResponse(WebURLLoader*, const WebURLResponse& response, WebDataConsumerHandle* rawHandle)
 {
     ASSERT(!response.isNull());
     // |rawHandle|'s ownership is transferred to the callee.
     OwnPtr<WebDataConsumerHandle> handle = adoptPtr(rawHandle);
 
     bool isMultipartPayload = response.isMultipartPayload();
     bool isValidStateTransition = (m_state == ConnectionStateStarted || m_state == ConnectionStateReceivedResponse);
     // In the case of multipart loads, calls to didReceiveData & didReceiveResponse can be interleaved.
     RELEASE_ASSERT(isMultipartPayload || isValidStateTransition);
     m_state = ConnectionStateReceivedResponse;
 
     const ResourceResponse& resourceResponse = response.toResourceResponse();
 
     if (responseNeedsAccessControlCheck()) {
         if (response.wasFetchedViaServiceWorker()) {
             if (response.wasFallbackRequiredByServiceWorker()) {
                 m_loader->cancel();
                 m_loader.clear();
                 m_state = ConnectionStateStarted;
                 m_loader = adoptPtr(Platform::current()->createURLLoader());
                 ASSERT(m_loader);
-                ASSERT(!m_request.skipServiceWorker());
-                m_request.setSkipServiceWorker(true);
-                WrappedResourceRequest wrappedRequest(m_request);
-                m_loader->loadAsynchronously(wrappedRequest, this);
+                ResourceRequest request = m_resource->lastResourceRequest();
+                ASSERT(!request.skipServiceWorker());
+                request.setSkipServiceWorker(true);
+                m_loader->loadAsynchronously(WrappedResourceRequest(request), this);
                 return;
             }
         } else {
             if (!m_resource->isCacheValidator() || resourceResponse.httpStatusCode() != 304)
                 m_resource->setResponse(resourceResponse);
-            if (!m_fetcher->canAccessResource(m_resource.get(), m_options.securityOrigin.get(), response.url(), ResourceFetcher::ShouldLogAccessControlErrors)) {
+            if (!m_fetcher->canAccessResource(m_resource.get(), m_resource->options().securityOrigin.get(), response.url(), ResourceFetcher::ShouldLogAccessControlErrors)) {
                 m_fetcher->didReceiveResponse(m_resource.get(), resourceResponse);
                 cancel(ResourceError::cancelledDueToAccessCheckError(KURL(response.url())));
                 return;
             }
         }
     }
 
     m_resource->responseReceived(resourceResponse, handle.release());
     if (m_state == ConnectionStateReleased)
         return;
 
     m_fetcher->didReceiveResponse(m_resource.get(), resourceResponse);
     if (m_state == ConnectionStateReleased)
         return;
 
     if (response.toResourceResponse().isMultipart()) {
         // We only support multipart for images, though the image may be loaded
         // as a main resource that we end up displaying through an ImageDocument.
         if (!m_resource->isImage() && m_resource->getType() != Resource::MainResource) {
-            cancel();
+            cancel(ResourceError::cancelledError(resourceResponse.url()));
             return;
         }
         m_loadingMultipartContent = true;
     } else if (isMultipartPayload) {
         // Since a subresource loader does not load multipart sections progressively, data was delivered to the loader all at once.
         // After the first multipart section is complete, signal to delegates that this load is "finished"
         m_fetcher->subresourceLoaderFinishedLoadingOnePart(this);
         didFinishLoadingOnePart(0, WebURLLoaderClient::kUnknownEncodedDataLength);
     }
     if (m_state == ConnectionStateReleased)
         return;
 
     if (m_resource->response().httpStatusCode() < 400 || m_resource->shouldIgnoreHTTPStatusCodeErrors())
         return;
 
     if (!m_notifiedLoadComplete) {
         m_notifiedLoadComplete = true;
-        m_fetcher->didFailLoading(m_resource.get(), ResourceError::cancelledError(m_request.url()));
+        m_fetcher->didFailLoading(m_resource.get(), ResourceError::cancelledError(resourceResponse.url()));
     }
 
     ASSERT(m_state != ConnectionStateReleased);
     m_resource->error(Resource::LoadError);
-    cancel();
+    cancel(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)
 {
     RELEASE_ASSERT(m_state == ConnectionStateReceivedResponse || m_state == ConnectionStateReceivingData);
@@ skipping 53 matching lines @@
         return;
 
     m_resource->error(Resource::LoadError);
 
     if (m_state == ConnectionStateReleased)
         return;
 
     releaseResources();
 }
 
-void ResourceLoader::requestSynchronously()
+void ResourceLoader::requestSynchronously(ResourceRequest& request)
 {
     // downloadToFile is not supported for synchronous requests.
-    ASSERT(!m_request.downloadToFile());
+    ASSERT(!request.downloadToFile());
     ASSERT(m_loader);
 
+    // Synchronous requests should always be max priority, lest they hang the renderer.
+    request.setPriority(ResourceLoadPriorityHighest);
+
     if (m_fetcher->defersLoading()) {
         cancel();
         return;
     }
 
     RefPtrWillBeRawPtr<Resource> protectResource(m_resource.get());
-    WrappedResourceRequest requestIn(m_request);
+    WrappedResourceRequest requestIn(request);
     WebURLResponse responseOut;
     responseOut.initialize();
     WebURLError errorOut;
     WebData dataOut;
     m_loader->loadSynchronously(requestIn, responseOut, errorOut, dataOut);
     if (errorOut.reason) {
         if (m_state == ConnectionStateReleased) {
             // A message dispatched while synchronously fetching the resource
             // can bring about the cancellation of this load.
             ASSERT(!m_resource);
@@ skipping 14 matching lines @@
     // 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);
 }
 
 ResourceRequest& ResourceLoader::applyOptions(ResourceRequest& request) const
 {
-    request.setAllowStoredCredentials(m_options.allowCredentials == AllowStoredCredentials);
+    request.setAllowStoredCredentials(m_resource->options().allowCredentials == AllowStoredCredentials);
     return request;
 }
 
 } // namespace blink