Clean up ResourceLoader finish more, remove ConnectionState enum

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

Index: third_party/WebKit/Source/core/fetch/ResourceLoader.cpp
diff --git a/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp b/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp
index 3d2564384a97e906a533cfaa436805f83aa4831e..cbbe0f644aa1feed84e8f13b7338d46f405076b2 100644
--- a/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp
+++ b/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp
@@ -54,9 +54,7 @@ ResourceLoader* ResourceLoader::create(ResourceFetcher* fetcher, Resource* resou
 
 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);
@@ -64,7 +62,7 @@ ResourceLoader::ResourceLoader(ResourceFetcher* fetcher, Resource* resource)
 
 ResourceLoader::~ResourceLoader()
 {
-    ASSERT(m_state == ConnectionStateReleased);
+    ASSERT(!m_loader);
 }
 
 DEFINE_TRACE(ResourceLoader)
@@ -73,29 +71,11 @@ DEFINE_TRACE(ResourceLoader)
     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);
@@ -115,68 +95,23 @@ void ResourceLoader::setDefersLoading(bool 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());
 
@@ -188,14 +123,13 @@ void ResourceLoader::willFollowRedirect(WebURLLoader*, WebURLRequest& passedNewR
         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);
 }
 
@@ -215,19 +149,12 @@ void ResourceLoader::didReceiveResponse(WebURLLoader*, const WebURLResponse& res
     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();
@@ -241,23 +168,23 @@ void ResourceLoader::didReceiveResponse(WebURLLoader*, const WebURLResponse& res
                 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)
@@ -267,44 +194,26 @@ void ResourceLoader::didReceiveResponse(WebURLLoader* loader, const WebURLRespon
 
 void ResourceLoader::didReceiveData(WebURLLoader*, const char* data, int length, int encodedDataLength)
 {
-    RELEASE_ASSERT(m_state == ConnectionStateReceivedResponse || m_state == ConnectionStateReceivingData);
-    m_state = ConnectionStateReceivingData;
-
-    // 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_fetcher->didReceiveData(m_resource.get(), data, length, encodedDataLength);
     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)
@@ -327,18 +236,17 @@ void ResourceLoader::requestSynchronously(ResourceRequest& request)
     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;