Clear Resource::m_loader in Resource::finish() and Resource::error()

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 048e1018c6cec0784fa2077e199aeb2a61249805..1551551045de5ed01d11c9e22a31b230ff292f98 100644
--- a/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp
+++ b/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp
@@ -80,7 +80,6 @@ void ResourceLoader::releaseResources()
     m_fetcher->didLoadResource(m_resource.get());
     if (m_state == ConnectionStateReleased)
         return;
-    m_resource->clearLoader();
     m_resource = nullptr;
 
     ASSERT(m_state != ConnectionStateReleased);
@@ -129,7 +128,7 @@ void ResourceLoader::didDownloadData(WebURLLoader*, int length, int encodedDataL
 void ResourceLoader::didFinishLoadingOnePart(double finishTime, int64_t encodedDataLength)
 {
     ASSERT(m_state != ConnectionStateReleased);
-    if (isFinishing()) {
+    if (m_state == ConnectionStateFinishedLoading) {
         m_fetcher->removeResourceLoader(this);
     } else {
         // When loading a multipart resource, make the loader non-block when
@@ -154,13 +153,6 @@ void ResourceLoader::didChangePriority(ResourceLoadPriority loadPriority, int in
         m_loader->didChangePriority(static_cast<WebURLRequest::Priority>(loadPriority), intraPriorityValue);
 }
 
-void ResourceLoader::cancelIfNotFinishing()
-{
-    if (isFinishing())
-        return;
-    cancel();
-}
-
 void ResourceLoader::cancel()
 {
     cancel(ResourceError());
@@ -168,19 +160,8 @@ void ResourceLoader::cancel()
 
 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_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);
+    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
@@ -188,21 +169,12 @@ void ResourceLoader::cancel(const ResourceError& error)
     // 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 ConnectionStateCanceled, and ASSERT or break in other ways.
+    // is ConnectionStateFinishedLoading, and ASSERT or break in other ways.
     if (m_loader) {
         m_loader->cancel();
         m_loader.clear();
     }
-
-    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();
+    didFail(nullptr, error.isNull() ? ResourceError::cancelledError(m_resource->lastResourceRequest().url()) : error);

[Nate Chapin, 2016/04/27 19:48:30]

Most of cancel() is the same as didFail(). Delegate to didFail() instead.

 }
 
 void ResourceLoader::willFollowRedirect(WebURLLoader*, WebURLRequest& passedNewRequest, const WebURLResponse& passedRedirectResponse)
@@ -219,7 +191,8 @@ void ResourceLoader::willFollowRedirect(WebURLLoader*, WebURLRequest& passedNewR
         m_resource->willFollowRedirect(newRequest, redirectResponse);
     } else {
         m_resource->willNotFollowRedirect();
-        cancel(ResourceError::cancelledDueToAccessCheckError(newRequest.url()));
+        if (m_state != ConnectionStateReleased)
+            cancel(ResourceError::cancelledDueToAccessCheckError(newRequest.url()));
     }
 }
 
@@ -287,14 +260,6 @@ void ResourceLoader::didReceiveResponse(WebURLLoader*, const WebURLResponse& res
 
     if (m_resource->response().httpStatusCode() < 400 || m_resource->shouldIgnoreHTTPStatusCodeErrors())
         return;
-
-    if (!m_notifiedLoadComplete) {
-        m_notifiedLoadComplete = true;
-        m_fetcher->didFailLoading(m_resource.get(), ResourceError::cancelledError(resourceResponse.url()));
-    }
-
-    ASSERT(m_state != ConnectionStateReleased);
-    m_resource->error(Resource::LoadError);

[Nate Chapin, 2016/04/27 19:48:30]

This block is also redundant with cancel/didFail.

     cancel(ResourceError::cancelledError(resourceResponse.url()));
 }
 
@@ -325,45 +290,23 @@ void ResourceLoader::didReceiveData(WebURLLoader*, const char* data, int length,
 
 void ResourceLoader::didFinishLoading(WebURLLoader*, double finishTime, int64_t encodedDataLength)
 {
-
     RELEASE_ASSERT(m_state == ConnectionStateReceivedResponse || m_state == ConnectionStateReceivingData);
     m_state = ConnectionStateFinishedLoading;
-    WTF_LOG(ResourceLoading, "Received '%s'.", m_resource->url().getString().latin1().data());

[Nate Chapin, 2016/04/27 19:48:30]

I don't even know where these log to, or if anyone has used them post-fork.

-
     m_resource->setLoadFinishTime(finishTime);
     didFinishLoadingOnePart(finishTime, encodedDataLength);
-    if (m_state == ConnectionStateReleased)
-        return;
     m_resource->finish();
-
-    // If the load has been cancelled by a delegate in response to didFinishLoad(), do not release
-    // the resources a second time, they have been released by cancel.
-    if (m_state == ConnectionStateReleased)
-        return;
     releaseResources();
 }
 
 void ResourceLoader::didFail(WebURLLoader*, const WebURLError& error)
 {
-
+    ASSERT(m_state != ConnectionStateFinishedLoading);
     ASSERT(m_state != ConnectionStateReleased);
-    m_state = ConnectionStateFailed;
-    WTF_LOG(ResourceLoading, "Failed to load '%s'.\n", m_resource->url().getString().latin1().data());
-
+    m_state = ConnectionStateFinishedLoading;
     m_resource->setResourceError(error);
-
-    if (!m_notifiedLoadComplete) {
-        m_notifiedLoadComplete = true;
-        m_fetcher->didFailLoading(m_resource.get(), error);
-    }
-    if (m_state == ConnectionStateReleased)
-        return;
-
+    m_notifiedLoadComplete = true;
+    m_fetcher->didFailLoading(m_resource.get(), error);

[Nate Chapin, 2016/04/27 19:48:30]

Unconditionally call didFailLoading here. The only case where
m_notifiedLoadComplete will be set to true before finishing is for multipart
loads, which set it when the first payload part completes.
didFinishLoadingOnePart() ensures ResourceFetcher is properly notified if the
load completes successfully even when m_notifiedLoadComplete is true. However,
if the load fails or cancels after m_notifiedLoadComplete is set to true, we
still need to call didFailLoading to ensure that ResourceFetcher drops its
reference to this ResourceLoader.

     m_resource->error(Resource::LoadError);
-
-    if (m_state == ConnectionStateReleased)
-        return;
-
     releaseResources();
 }