ThreadableLoader::cancel should be called before loader destruction

third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp

Index: third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
diff --git a/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp b/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
index b7a413c6c6ef3093cd08612e92bf7f046ddec963..6169373cb9fde5827c4d9de390ea99c472c40755 100644
--- a/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
@@ -345,7 +345,7 @@ void DocumentThreadableLoader::makeCrossOriginAccessRequest(const ResourceReques
 
 DocumentThreadableLoader::~DocumentThreadableLoader()
 {
-    m_client = nullptr;
+    DCHECK(!m_client);

[hiroshige, 2016/07/19 06:56:13]

optional: how about making this CHECK() for a while on canary to catch
unexpected violations to this assumption?

[yhirano, 2016/07/19 08:05:03]

Done.

 
     // TODO(oilpan): Remove this once DocumentThreadableLoader is once again a ResourceOwner.
     clearResource();
@@ -937,16 +937,19 @@ void DocumentThreadableLoader::loadRequest(const ResourceRequest& request, Resou
     ResourceError error = resource ? resource->resourceError() : ResourceError();
 
     InspectorInstrumentation::documentThreadableLoaderStartedLoadingForClient(m_document, identifier, m_client);
+    ThreadableLoaderClient* client = m_client;

[hiroshige, 2016/07/19 06:56:14]

IIUC the changes of lines 940--961 are made in order to make |m_client| null
when loading is completed, i.e. didFail.*() is called, right?

[yhirano, 2016/07/19 08:05:03]

Yes, as m_client will never be used after a completion method is called, there
should be no problem.

 
     if (!resource) {
-        m_client->didFail(error);
+        m_client = nullptr;
+        client->didFail(error);
         return;
     }
 
     // No exception for file:/// resources, see <rdar://problem/4962298>.
     // Also, if we have an HTTP response, then it wasn't a network error in fact.
     if (!error.isNull() && !requestURL.isLocalFile() && response.httpStatusCode() <= 0) {
-        m_client->didFail(error);
+        m_client = nullptr;
+        client->didFail(error);
         return;
     }
 
@@ -954,7 +957,8 @@ void DocumentThreadableLoader::loadRequest(const ResourceRequest& request, Resou
     // request and response URLs. This isn't a perfect test though, since a server can serve a redirect to the same URL that was
     // requested. Also comparing the request and response URLs as strings will fail if the requestURL still has its credentials.
     if (requestURL != response.url() && !isAllowedRedirect(response.url())) {
-        m_client->didFailRedirectCheck();
+        m_client = nullptr;
+        client->didFailRedirectCheck();
         return;
     }