Prevent ThreadableLoaderClient methods from being called after failure notification

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 82 matching lines @@
     , m_requestContext(request.requestContext())
     , m_timeoutTimer(this, &DocumentThreadableLoader::didTimeout)
     , m_requestStartedSeconds(0.0)
     , m_corsRedirectLimit(kMaxCORSRedirects)
 {
     ASSERT(client);
     // Setting an outgoing referer is only supported in the async code path.
     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;
     }
 
     m_requestStartedSeconds = monotonicallyIncreasingTime();
 
     // Save any CORS simple headers on the request here. If this request redirects cross-origin, we cancel the old request
     // create a new one, and copy these headers.
     const HTTPHeaderMap& headerMap = request.httpHeaderFields();
     for (const auto& header : headerMap) {
         if (FetchUtils::isSimpleHeader(header.key, header.value))
@@ skipping 37 matching lines @@
     }
 
     ASSERT(m_options.crossOriginRequestPolicy == UseAccessControl);
 
     makeCrossOriginAccessRequest(request);
 }
 
 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;
     }
 
     // We use isSimpleOrForbiddenRequest() here since |request| may have been
     // modified in the process of loading (not from the user's input). For
     // example, referrer. We need to accept them. For security, we must reject
     // forbidden headers/methods at the point we accept user's input. Not here.
     if ((m_options.preflightPolicy == ConsiderPreflight && FetchUtils::isSimpleOrForbiddenRequest(request.httpMethod(), request.httpHeaderFields())) || m_options.preflightPolicy == PreventPreflight) {
         ResourceRequest crossOriginRequest(request);
         ResourceLoaderOptions crossOriginOptions(m_resourceLoaderOptions);
@@ skipping 47 matching lines @@
     }
 }
 
 void DocumentThreadableLoader::cancel()
 {
     cancelWithError(ResourceError());
 }
 
 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;
         if (errorForCallback.isNull()) {
             // FIXME: This error is sent to the client in didFail(), so it should not be an internal one. Use FrameLoaderClient::cancelledError() instead.
             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)
 {
     if (resource())
         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
 // RawResource::didAddClient(), clearing |request| won't be propagated
 // to content::WebURLLoaderImpl. So, this loader must also get detached from
 // the resource by calling clearResource().
 void DocumentThreadableLoader::redirectReceived(Resource* resource, ResourceRequest& request, const ResourceResponse& redirectResponse)
 {
     ASSERT(m_client);
     ASSERT_UNUSED(resource, resource == this->resource());
     ASSERT(m_async);
 
     RefPtr<DocumentThreadableLoader> protect(this);

[hiroshige, 2015/08/06 05:25:03]

Can we remove |protect| here as well? (I'm not so sure though)

[tyoshino (SeeGerritForStatus), 2015/08/26 12:57:32]

Agreed. Removed.

 
     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;
     }
 
     // Allow same origin requests to continue after allowing clients to audit the redirect.
     if (isAllowedRedirect(request.url())) {
         if (m_client->isDocumentThreadableLoaderClient())
             static_cast<DocumentThreadableLoaderClient*>(m_client)->willFollowRedirect(request, redirectResponse);
         return;
     }
 
     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;
 
         InspectorInstrumentation::didReceiveCORSRedirectResponse(m_document.frame(), resource->identifier(), m_document.frame()->loader().documentLoader(), redirectResponse, 0);
 
         bool allowRedirect = false;
         String accessControlErrorDescription;
 
         // Non-simple cross origin requests (both preflight and actual one) are
         // not allowed to follow redirect.
@@ skipping 30 matching lines @@
             request.clearHTTPReferrer();
             request.clearHTTPOrigin();
             request.clearHTTPUserAgent();
             // Add any CORS simple request headers which we previously saved from the original request.
             for (const auto& header : m_simpleRequestHeaders)
                 request.setHTTPHeaderField(header.key, header.value);
             makeCrossOriginAccessRequest(request);
             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)
 {
     ASSERT(m_client);
     ASSERT_UNUSED(resource, resource == this->resource());
     ASSERT(m_async);
 
     m_client->didSendData(bytesSent, totalBytesToBeSent);
 }
@@ skipping 100 matching lines @@
     // loadFallbackRequestForServiceWorker().
     // FIXME: We should use |m_sameOriginRequest| when we will support
     // Suborigins (crbug.com/336894) for Service Worker.
     ASSERT(!m_fallbackRequestForServiceWorker || securityOrigin()->canRequest(m_fallbackRequestForServiceWorker->url()));
     m_fallbackRequestForServiceWorker = nullptr;
 
     if (!m_sameOriginRequest && m_options.crossOriginRequestPolicy == UseAccessControl) {
         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;
         }
     }
 
     m_client->didReceiveResponse(identifier, response, handle);
 }
 
 void DocumentThreadableLoader::setSerializedCachedMetadata(Resource*, const char* data, size_t size)
 {
     if (m_actualRequest)
@@ skipping 24 matching lines @@
 
     m_client->didReceiveData(data, dataLength);
 }
 
 void DocumentThreadableLoader::notifyFinished(Resource* resource)
 {
     ASSERT(m_client);
     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());
+    }
 }
 
 void DocumentThreadableLoader::handleSuccessfulFinish(unsigned long identifier, double finishTime)
 {
     ASSERT(!m_fallbackRequestForServiceWorker);
 
     if (m_actualRequest) {
         ASSERT(!m_sameOriginRequest);
         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;
+        }

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

nit: please add a comment that the code sequence above does the same thing as
clear() except for clearResource() because the resource is needed in
didFinishLoading().

+        client->didFinishLoading(identifier, finishTime);
+        // |this| may be dead here.
     }
 }
 
 void DocumentThreadableLoader::didTimeout(Timer<DocumentThreadableLoader>* timer)
 {
     ASSERT_UNUSED(timer, timer == &m_timeoutTimer);
 
     // Using values from net/base/net_error_list.h ERR_TIMED_OUT,
     // Same as existing FIXME above - this error should be coming from FrameLoaderClient to be identifiable.
     static const int timeoutError = -7;
@@ skipping 23 matching lines @@
     loadRequest(*actualRequest, *actualOptions);
 }
 
 void DocumentThreadableLoader::handlePreflightFailure(const String& url, const String& errorDescription)
 {
     ResourceError error(errorDomainBlinkInternal, 0, url, errorDescription);
 
     // 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)
 {
     // Any credential should have been removed from the cross-site requests.
     const KURL& requestURL = request.url();
     ASSERT(m_sameOriginRequest || requestURL.user().isEmpty());
     ASSERT(m_sameOriginRequest || requestURL.pass().isEmpty());
 
     // Update resourceLoaderOptions with enforced values.
@@ skipping 47 matching lines @@
     // 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() && (!isAllowedByContentSecurityPolicy(response.url(), ContentSecurityPolicy::DidRedirect) || !isAllowedRedirect(response.url()))) {
         m_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.
+    if (!m_client)
+        return;
+
     SharedBuffer* data = resource->resourceBuffer();
     if (data)
         handleReceivedData(data->data(), data->size());
 

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

Should we also check |m_client|?
e.g. for the case where a user calls ThreadableLoader::cancel() from
didReceiveData()?

     handleSuccessfulFinish(identifier, 0.0);
 }
 
 bool DocumentThreadableLoader::isAllowedRedirect(const KURL& url) const
 {
     if (m_options.crossOriginRequestPolicy == AllowCrossOriginRequests)
         return true;
 
     return m_sameOriginRequest && securityOrigin()->canRequest(url);
 }
@@ skipping 11 matching lines @@
         return DoNotAllowStoredCredentials;
     return m_resourceLoaderOptions.allowCredentials;
 }
 
 SecurityOrigin* DocumentThreadableLoader::securityOrigin() const
 {
     return m_securityOrigin ? m_securityOrigin.get() : m_document.securityOrigin();
 }
 
 } // namespace blink