Clean up ResourceLoader finish more, remove ConnectionState enum

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 36 matching lines @@
 
 namespace blink {
 
 ResourceLoader* ResourceLoader::create(ResourceFetcher* fetcher, Resource* resource)
 {
     return new ResourceLoader(fetcher, resource);
 }
 
 ResourceLoader::ResourceLoader(ResourceFetcher* fetcher, Resource* resource)
     : m_fetcher(fetcher)
-    , m_notifiedLoadComplete(false)

[Nate Chapin, 2016/05/12 20:51:12]

Like didFailLoading(), didFinishLoading() should now be resilient to being
called after the first part of a multipart response completes, so this bit isn't
needed.

     , m_resource(resource)
-    , m_state(ConnectionStateNew)
 {
     ASSERT(m_resource);
     ASSERT(m_fetcher);
 }
 
 ResourceLoader::~ResourceLoader()
 {
-    ASSERT(m_state == ConnectionStateReleased);
+    ASSERT(!m_loader);
 }
 
 DEFINE_TRACE(ResourceLoader)
 {
     visitor->trace(m_fetcher);
     visitor->trace(m_resource);
 }
 
-void ResourceLoader::releaseResources()

[Nate Chapin, 2016/05/12 20:51:12]

didLoadResource() moved to ResourceFetcher's didFinishLoading/didFailLoading.

m_loader is now cleared at the start of finish/fail.

Everything else isn't strictly necessary.

-{
-    ASSERT(m_state != ConnectionStateReleased);
-    ASSERT(m_notifiedLoadComplete);
-    m_fetcher->didLoadResource(m_resource.get());
-    ASSERT(m_state != ConnectionStateReleased);
-    m_resource = nullptr;
-    m_state = ConnectionStateReleased;
-    if (m_loader) {
-        m_loader->cancel();
-        m_loader.clear();
-    }
-    m_fetcher.clear();
-}
-
 void ResourceLoader::start(ResourceRequest& request)
 {
     ASSERT(!m_loader);
 
     m_fetcher->willStartLoadingResource(m_resource.get(), this, 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_resource->options().synchronousPolicy == RequestSynchronously)
         requestSynchronously(request);
     else
         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)
 {
-    RELEASE_ASSERT(m_state == ConnectionStateReceivedResponse);

[Nate Chapin, 2016/05/12 20:51:12]

If you're not ok with dropping all these ASSERTs, i'd suggest that we replace
them with m_loader asserts. AFAIK, the only way the state machine has ever
gotten corrupted is when a load is cancelled but the WebURLLoader isn't notified
promptly. Becasue WebURLLoader is now immediately cleared when we don't want any
more messages from it, this should suffice.

     m_fetcher->didDownloadData(m_resource.get(), length, encodedDataLength);
-    if (m_state == ConnectionStateReleased)
-        return;
     m_resource->didDownloadData(length);
 }
 
-void ResourceLoader::didFinishLoadingOnePart(double finishTime, int64_t encodedDataLength)
-{
-    ASSERT(m_state != ConnectionStateReleased);
-    if (m_state == ConnectionStateFinishedLoading) {

[Nate Chapin, 2016/05/12 20:51:12]

This block is in ResourceFetcher::didFinishLoading now, allowing
removeResourceLoader() and moveResourceLoaderToNonBlocking() private.

-        m_fetcher->removeResourceLoader(this);
-    } else {
-        // When loading a multipart resource, make the loader non-block when
-        // finishing loading the first part.
-        m_fetcher->moveResourceLoaderToNonBlocking(this);
-
-        m_fetcher->didLoadResource(m_resource.get());
-        if (m_state == ConnectionStateReleased)
-            return;
-    }
-
-    if (m_notifiedLoadComplete)
-        return;
-    m_notifiedLoadComplete = true;
-    m_fetcher->didFinishLoading(m_resource.get(), finishTime, encodedDataLength);
-}
-
 void ResourceLoader::didChangePriority(ResourceLoadPriority loadPriority, int intraPriorityValue)
 {
-    ASSERT(m_state != ConnectionStateReleased);
     if (m_loader)
         m_loader->didChangePriority(static_cast<WebURLRequest::Priority>(loadPriority), intraPriorityValue);
 }
 
 void ResourceLoader::cancel()
 {
-    cancel(ResourceError());
-}
-
-void ResourceLoader::cancel(const ResourceError& error)
-{
-    ASSERT(m_state != ConnectionStateFinishedLoading);
-    ASSERT(m_state != ConnectionStateReleased);
-
-    // If we don't immediately clear m_loader when cancelling, we might get
-    // unexpected reentrancy. m_resource->error() can trigger JS events, which
-    // could start a modal dialog. Normally, a modal dialog would defer loading
-    // and prevent receiving messages for a cancelled ResourceLoader, but
-    // m_fetcher->didFailLoading() severs the connection by which all of a
-    // page's loads are deferred. A response can then arrive, see m_state
-    // is ConnectionStateFinishedLoading, and ASSERT or break in other ways.
-    if (m_loader) {

[Nate Chapin, 2016/05/12 20:51:12]

Because didFail() now starts by clearing m_loader, this isn't necessary.

-        m_loader->cancel();
-        m_loader.clear();
-    }
-    didFail(nullptr, error.isNull() ? ResourceError::cancelledError(m_resource->lastResourceRequest().url()) : error);
+    didFail(nullptr, ResourceError::cancelledError(m_resource->lastResourceRequest().url()));
 }
 
 void ResourceLoader::willFollowRedirect(WebURLLoader*, WebURLRequest& passedNewRequest, const WebURLResponse& passedRedirectResponse)
 {
-    ASSERT(m_state != ConnectionStateReleased);
     ASSERT(!passedNewRequest.isNull());
     ASSERT(!passedRedirectResponse.isNull());
 
     ResourceRequest& newRequest(passedNewRequest.toMutableResourceRequest());
     const ResourceResponse& redirectResponse(passedRedirectResponse.toResourceResponse());
     newRequest.setFollowedRedirect(true);
 
     if (m_fetcher->willFollowRedirect(m_resource.get(), newRequest, redirectResponse)) {
         m_resource->willFollowRedirect(newRequest, redirectResponse);
     } else {
         m_resource->willNotFollowRedirect();
-        if (m_state != ConnectionStateReleased)
-            cancel(ResourceError::cancelledDueToAccessCheckError(newRequest.url()));
+        if (m_loader)
+            didFail(nullptr, ResourceError::cancelledDueToAccessCheckError(newRequest.url()));
     }
 }
 
 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_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 isValidStateTransition = (m_state == ConnectionStateStarted || m_state == ConnectionStateReceivedResponse);
-    RELEASE_ASSERT(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);
                 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_resource->options().securityOrigin.get(), response.url(), ResourceFetcher::ShouldLogAccessControlErrors)) {
                 m_fetcher->didReceiveResponse(m_resource.get(), resourceResponse);
-                cancel(ResourceError::cancelledDueToAccessCheckError(KURL(response.url())));
+                didFail(nullptr, ResourceError::cancelledDueToAccessCheckError(KURL(response.url())));
                 return;
             }
         }
     }
 
     m_resource->responseReceived(resourceResponse, handle.release());
-    if (m_state == ConnectionStateReleased)
+    if (!m_loader)
         return;
 
     m_fetcher->didReceiveResponse(m_resource.get(), resourceResponse);
-    if (m_state == ConnectionStateReleased)
+    if (!m_loader)
         return;
 
     if (m_resource->response().httpStatusCode() < 400 || m_resource->shouldIgnoreHTTPStatusCodeErrors())
         return;
-    cancel(ResourceError::cancelledError(resourceResponse.url()));
+    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)
 {
-    RELEASE_ASSERT(m_state == ConnectionStateReceivedResponse || m_state == ConnectionStateReceivingData);
-    m_state = ConnectionStateReceivingData;
+    RELEASE_ASSERT(length >= 0);
+    m_fetcher->didReceiveData(m_resource.get(), data, length, encodedDataLength);
+    m_resource->appendData(data, length);
+}
 
-    // It is possible to receive data on uninitialized resources if it had an error status code, and we are running a nested message
-    // loop. When this occurs, ignoring the data is the correct action.
-    if (m_resource->response().httpStatusCode() >= 400 && !m_resource->shouldIgnoreHTTPStatusCodeErrors())

[Nate Chapin, 2016/05/12 20:51:12]

This exists to defend against nested loops causing data to be received when we
call didFail() in didReceiveResponse() and a nested message loop starts while
handling the error. Because the loader gets cancelled at the start of didFail(),
this reeentrancy should no longer be possible.

-        return;
-
-    // FIXME: If we get a resource with more than 2B bytes, this code won't do the right thing.
-    // However, with today's computers and networking speeds, this won't happen in practice.
-    // Could be an issue with a giant local file.
-    m_fetcher->didReceiveData(m_resource.get(), data, length, encodedDataLength);
-    if (m_state == ConnectionStateReleased)
-        return;
-    RELEASE_ASSERT(length >= 0);
-    m_resource->appendData(data, length);
+void ResourceLoader::didFinishLoadingFirstPartInMultipart()
+{
+    m_fetcher->didFinishLoading(m_resource.get(), 0, WebURLLoaderClient::kUnknownEncodedDataLength, true);
 }
 
 void ResourceLoader::didFinishLoading(WebURLLoader*, double finishTime, int64_t encodedDataLength)
 {
-    RELEASE_ASSERT(m_state == ConnectionStateReceivedResponse || m_state == ConnectionStateReceivingData);
-    m_state = ConnectionStateFinishedLoading;
-    didFinishLoadingOnePart(finishTime, encodedDataLength);
-    ASSERT(m_state != ConnectionStateReleased);
-    m_resource->finish(finishTime);

[Nate Chapin, 2016/05/12 20:51:12]

Call finish/error from ResourceFetcher, so that it still happens between calls
to FetchContext's didFinishLoading/didFailLoading and didLoadResource.

-    releaseResources();
+    m_loader.clear();
+    m_fetcher->didFinishLoading(m_resource.get(), finishTime, encodedDataLength, false);
 }
 
 void ResourceLoader::didFail(WebURLLoader*, const WebURLError& error)
 {
-    ASSERT(m_state != ConnectionStateFinishedLoading);
-    ASSERT(m_state != ConnectionStateReleased);
-    m_state = ConnectionStateFinishedLoading;
-    m_notifiedLoadComplete = true;
+    m_loader.clear();
     m_fetcher->didFailLoading(m_resource.get(), error);
-    ASSERT(m_state != ConnectionStateReleased);
-    m_resource->error(error);
-    releaseResources();
 }
 
 void ResourceLoader::requestSynchronously(ResourceRequest& request)
 {
     // downloadToFile is not supported for synchronous requests.
     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;
     }
 
     WrappedResourceRequest requestIn(request);
     WebURLResponse responseOut;
     responseOut.initialize();
     WebURLError errorOut;
     WebData dataOut;
     m_loader->loadSynchronously(requestIn, responseOut, errorOut, dataOut);
+
+    // A message dispatched while synchronously fetching the resource
+    // can bring about the cancellation of this load.
+    if (!m_loader)
+        return;
     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);
-            return;
-        }
         didFail(0, errorOut);
         return;
     }
     didReceiveResponse(0, responseOut);
-    if (m_state == ConnectionStateReleased)
+    if (!m_loader)
         return;
     RefPtr<ResourceLoadInfo> resourceLoadInfo = responseOut.toResourceResponse().resourceLoadInfo();
     int64_t encodedDataLength = resourceLoadInfo ? resourceLoadInfo->encodedDataLength : WebURLLoaderClient::kUnknownEncodedDataLength;
 
     // Follow the async case convention of not calling didReceiveData or
     // appending data to m_resource if the response body is empty. Copying the
     // 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