ThreadableLoader::cancel should be called before loader destruction

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

 /*
  * Copyright (C) 2011, 2012 Google Inc. All rights reserved.
  * Copyright (C) 2013, Intel Corporation
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
@@ skipping 327 matching lines @@
             ResourceLoaderOptions preflightOptions = m_actualOptions;
             preflightOptions.allowCredentials = DoNotAllowStoredCredentials;
             loadRequest(preflightRequest, preflightOptions);
             // |this| may be dead here in async mode.
         }
     }
 }
 
 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();
 }
 
 void DocumentThreadableLoader::overrideTimeout(unsigned long timeoutMilliseconds)
 {
     ASSERT(m_async);
 
     // |m_requestStartedSeconds| == 0.0 indicates loading is already finished
@@ skipping 571 matching lines @@
 
     FetchRequest fetchRequest(request, m_options.initiator, resourceLoaderOptions);
     if (m_options.crossOriginRequestPolicy == AllowCrossOriginRequests)
         fetchRequest.setOriginRestriction(FetchRequest::NoOriginRestriction);
     Resource* resource = RawResource::fetchSynchronously(fetchRequest, document().fetcher());
     ResourceResponse response = resource ? resource->response() : ResourceResponse();
     unsigned long identifier = resource ? resource->identifier() : std::numeric_limits<unsigned long>::max();
     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;
     }
 
     // FIXME: A synchronous request does not tell us whether a redirect happened or not, so we guess by comparing the
     // 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;
     }
 
     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.
@@ skipping 32 matching lines @@
     return m_securityOrigin ? m_securityOrigin.get() : document().getSecurityOrigin();
 }
 
 Document& DocumentThreadableLoader::document() const
 {
     ASSERT(m_document);
     return *m_document;
 }
 
 } // namespace blink