Prevent ThreadableLoaderClient methods from being called after failure notification

Source/core/loader/DocumentThreadableLoader.cpp

Index: Source/core/loader/DocumentThreadableLoader.cpp
diff --git a/Source/core/loader/DocumentThreadableLoader.cpp b/Source/core/loader/DocumentThreadableLoader.cpp
index 74bf523ccfafd5dd70bdf8d83fee6171c90a7c4e..fd200356acbd5ff3f971bdcdd06654072ba7a22a 100644
--- a/Source/core/loader/DocumentThreadableLoader.cpp
+++ b/Source/core/loader/DocumentThreadableLoader.cpp
@@ -100,7 +100,10 @@ DocumentThreadableLoader::DocumentThreadableLoader(Document& document, Threadabl
     ASSERT(m_async || request.httpReferrer().isEmpty());
 
     if (!m_sameOriginRequest && m_options.crossOriginRequestPolicy == DenyCrossOriginRequests) {
-        m_client->didFail(ResourceError(errorDomainBlinkInternal, 0, request.url().string(), "Cross origin requests are not supported."));
+        ThreadableLoaderClient* client = m_client;
+        clear();
+        client->didFail(ResourceError(errorDomainBlinkInternal, 0, request.url().string(), "Cross origin requests are not supported."));
+        // |this| may be dead here.
         return;
     }
 
@@ -158,13 +161,17 @@ void DocumentThreadableLoader::dispatchInitialRequest(const ResourceRequest& req
 void DocumentThreadableLoader::makeCrossOriginAccessRequest(const ResourceRequest& request)
 {
     ASSERT(m_options.crossOriginRequestPolicy == UseAccessControl);
+    ASSERT(!resource());
 
     // Cross-origin requests are only allowed certain registered schemes.
     // We would catch this when checking response headers later, but there
     // is no reason to send a request, preflighted or not, that's guaranteed
     // to be denied.
     if (!SchemeRegistry::shouldTreatURLSchemeAsCORSEnabled(request.url().protocol())) {
-        m_client->didFailAccessControlCheck(ResourceError(errorDomainBlinkInternal, 0, request.url().string(), "Cross origin requests are only supported for protocol schemes: " + SchemeRegistry::listOfCORSEnabledURLSchemes() + "."));
+        ThreadableLoaderClient* client = m_client;
+        clear();
+        client->didFailAccessControlCheck(ResourceError(errorDomainBlinkInternal, 0, request.url().string(), "Cross origin requests are only supported for protocol schemes: " + SchemeRegistry::listOfCORSEnabledURLSchemes() + "."));
+        // |this| may be dead here.
         return;
     }
 
@@ -232,8 +239,6 @@ void DocumentThreadableLoader::cancel()
 
 void DocumentThreadableLoader::cancelWithError(const ResourceError& error)
 {
-    RefPtr<DocumentThreadableLoader> protect(this);
-
     // Cancel can re-enter and m_resource might be null here as a result.
     if (m_client && resource()) {
         ResourceError errorForCallback = error;
@@ -242,11 +247,14 @@ void DocumentThreadableLoader::cancelWithError(const ResourceError& error)
             errorForCallback = ResourceError(errorDomainBlinkInternal, 0, resource()->url().string(), "Load cancelled");
             errorForCallback.setIsCancellation(true);
         }
-        m_client->didFail(errorForCallback);
+
+        ThreadableLoaderClient* client = m_client;
+        clear();
+        client->didFail(errorForCallback);
+        // |this| may be dead here.
+    } else {
+        clear();
     }
-    clearResource();
-    m_client = 0;
-    m_requestStartedSeconds = 0.0;
 }
 
 void DocumentThreadableLoader::setDefersLoading(bool value)
@@ -255,6 +263,18 @@ void DocumentThreadableLoader::setDefersLoading(bool value)
         resource()->setDefersLoading(value);
 }
 
+void DocumentThreadableLoader::clear()
+{
+    m_client = 0;
+
+    if (!m_async)
+        return;
+
+    clearResource();
+    m_timeoutTimer.stop();
+    m_requestStartedSeconds = 0.0;
+}
+
 // In this method, we can clear |request| to tell content::WebURLLoaderImpl of
 // Chromium not to follow the redirect. This works only when this method is
 // called by RawResource::willSendRequest(). If called by
@@ -270,12 +290,13 @@ void DocumentThreadableLoader::redirectReceived(Resource* resource, ResourceRequ
     RefPtr<DocumentThreadableLoader> protect(this);
 
     if (!isAllowedByContentSecurityPolicy(request.url(), ContentSecurityPolicy::DidRedirect)) {
-        m_client->didFailRedirectCheck();
+        ThreadableLoaderClient* client = m_client;
+        clear();
+        client->didFailRedirectCheck();
+        // |this| may be dead here.
 
-        clearResource();
         request = ResourceRequest();
 
-        m_requestStartedSeconds = 0.0;
         return;
     }
 
@@ -287,7 +308,10 @@ void DocumentThreadableLoader::redirectReceived(Resource* resource, ResourceRequ
     }
 
     if (m_corsRedirectLimit <= 0) {
-        m_client->didFailRedirectCheck();
+        ThreadableLoaderClient* client = m_client;
+        clear();
+        client->didFailRedirectCheck();
+        // |this| may be dead here.
     } else if (m_options.crossOriginRequestPolicy == UseAccessControl) {
         --m_corsRedirectLimit;
 
@@ -338,16 +362,18 @@ void DocumentThreadableLoader::redirectReceived(Resource* resource, ResourceRequ
             return;
         }
 
-        ResourceError error(errorDomainBlinkInternal, 0, redirectResponse.url().string(), accessControlErrorDescription);
-        m_client->didFailAccessControlCheck(error);
+        ThreadableLoaderClient* client = m_client;
+        clear();
+        client->didFailAccessControlCheck(ResourceError(errorDomainBlinkInternal, 0, redirectResponse.url().string(), accessControlErrorDescription));
+        // |this| may be dead here.
     } else {
-        m_client->didFailRedirectCheck();
+        ThreadableLoaderClient* client = m_client;
+        clear();
+        client->didFailRedirectCheck();
+        // |this| may be dead here.
     }
 
-    clearResource();
     request = ResourceRequest();
-
-    m_requestStartedSeconds = 0.0;
 }
 
 void DocumentThreadableLoader::dataSent(Resource* resource, unsigned long long bytesSent, unsigned long long totalBytesToBeSent)
@@ -468,7 +494,11 @@ void DocumentThreadableLoader::handleResponse(unsigned long identifier, const Re
         String accessControlErrorDescription;
         if (!passesAccessControlCheck(response, effectiveAllowCredentials(), securityOrigin(), accessControlErrorDescription)) {
             reportResponseReceived(identifier, response);
-            m_client->didFailAccessControlCheck(ResourceError(errorDomainBlinkInternal, 0, response.url().string(), accessControlErrorDescription));
+
+            ThreadableLoaderClient* client = m_client;
+            clear();
+            client->didFailAccessControlCheck(ResourceError(errorDomainBlinkInternal, 0, response.url().string(), accessControlErrorDescription));
+            // |this| may be dead here.
             return;
         }
     }
@@ -513,12 +543,15 @@ void DocumentThreadableLoader::notifyFinished(Resource* resource)
     ASSERT(resource == this->resource());
     ASSERT(m_async);
 
-    m_timeoutTimer.stop();
-
-    if (resource->errorOccurred())
-        m_client->didFail(resource->resourceError());
-    else
+    if (resource->errorOccurred()) {
+        ThreadableLoaderClient* client = m_client;
+        ResourceError error = resource->resourceError();
+        clear();
+        client->didFail(error);
+        // |this| may be dead here.
+    } else {
         handleSuccessfulFinish(resource->identifier(), resource->loadFinishTime());

[hiroshige, 2015/08/03 13:22:19]

m_timeoutTimer.stop() is not executed in this branch?

[tyoshino (SeeGerritForStatus), 2015/08/04 08:32:05]

handleSuccessfulFinish does it. If it's just completion of the preflight
request, we need to keep the timer active. So, handleSuccessfulFinish doesn't
call stop() when m_actualRequest is set.

[hiroshige, 2015/08/04 10:20:16]

I see, thanks.
If so, does this CL also fix a bug of stopping m_timeoutTimer here?

But the timer is activated again in loadRequest() called from
loadActualRequest(), and thus we don't need to keep the timer active here?
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

+    }
 }
 
 void DocumentThreadableLoader::handleSuccessfulFinish(unsigned long identifier, double finishTime)
@@ -530,9 +563,14 @@ void DocumentThreadableLoader::handleSuccessfulFinish(unsigned long identifier,
         ASSERT(m_options.crossOriginRequestPolicy == UseAccessControl);
         loadActualRequest();
     } else {
-        // FIXME: Should prevent timeout from being overridden after finished loading, without
-        // resetting m_requestStartedSeconds to 0.0
-        m_client->didFinishLoading(identifier, finishTime);
+        ThreadableLoaderClient* client = m_client;
+        m_client = 0;
+        if (m_async) {
+            m_timeoutTimer.stop();
+            m_requestStartedSeconds = 0.0;
+        }
+        client->didFinishLoading(identifier, finishTime);
+        // |this| may be dead here.
     }
 }
 
@@ -576,9 +614,10 @@ void DocumentThreadableLoader::handlePreflightFailure(const String& url, const S
     // Prevent handleSuccessfulFinish() from bypassing access check.
     m_actualRequest = nullptr;
 
-    // FIXME: Should prevent timeout from being overridden after preflight failure, without
-    // resetting m_requestStartedSeconds to 0.0
-    m_client->didFailAccessControlCheck(error);
+    ThreadableLoaderClient* client = m_client;
+    clear();
+    client->didFailAccessControlCheck(error);
+    // |this| may be dead here.
 }
 
 void DocumentThreadableLoader::loadRequest(const ResourceRequest& request, ResourceLoaderOptions resourceLoaderOptions)
@@ -646,6 +685,14 @@ void DocumentThreadableLoader::loadRequest(const ResourceRequest& request, Resou
 
     handleResponse(identifier, response, nullptr);
 
+    // handleResponse() may detect an error. In such a case (check |m_client|
+    // as it gets reset by clear() call), skip the rest.
+    //
+    // |this| is alive here since loadResourceSynchronously() keeps it alive
+    // until the end of the function.
+    if (!m_client)
+        return;
+
     SharedBuffer* data = resource->resourceBuffer();
     if (data)
         handleReceivedData(data->data(), data->size());