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

third_party/WebKit/Source/core/fetch/ResourceLoader.cpp

 /*
  * Copyright (C) 2006, 2007, 2010, 2011 Apple Inc. All rights reserved.
  *           (C) 2007 Graham Dennis (graham.dennis@gmail.com)
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
@@ skipping 62 matching lines @@
     visitor->trace(m_resource);
 }
 
 void ResourceLoader::releaseResources()
 {
     ASSERT(m_state != ConnectionStateReleased);
     ASSERT(m_notifiedLoadComplete);
     m_fetcher->didLoadResource(m_resource.get());
     if (m_state == ConnectionStateReleased)
         return;
-    m_resource->clearLoader();
     m_resource = nullptr;
 
     ASSERT(m_state != ConnectionStateReleased);
     m_state = ConnectionStateReleased;
     if (m_loader) {
         m_loader->cancel();
         m_loader.clear();
     }
     m_fetcher.clear();
 }
@@ skipping 28 matching lines @@
     RELEASE_ASSERT(m_state == ConnectionStateReceivedResponse);
     m_fetcher->didDownloadData(m_resource.get(), length, encodedDataLength);
     if (m_state == ConnectionStateReleased)
         return;
     m_resource->didDownloadData(length);
 }
 
 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
         // finishing loading the first part.
         m_fetcher->moveResourceLoaderToNonBlocking(this);
 
         m_fetcher->didLoadResource(m_resource.get());
         if (m_state == ConnectionStateReleased)
             return;
     }
 
     if (m_notifiedLoadComplete)
         return;
     m_notifiedLoadComplete = true;
     m_fetcher->didFinishLoading(m_resource.get(), finishTime, encodedDataLength);
 }
 
 void ResourceLoader::didChangePriority(ResourceLoadPriority loadPriority, int intraPriorityValue)
 {
     ASSERT(m_state != ConnectionStateReleased);
     if (m_loader)
         m_loader->didChangePriority(static_cast<WebURLRequest::Priority>(loadPriority), intraPriorityValue);
 }
 
-void ResourceLoader::cancelIfNotFinishing()
-{
-    if (isFinishing())
-        return;
-    cancel();
-}
-
 void ResourceLoader::cancel()
 {
     cancel(ResourceError());
 }
 
 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
     // could start a modal dialog. Normally, a modal dialog would defer loading
     // 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);
 }
 
 void ResourceLoader::willFollowRedirect(WebURLLoader*, WebURLRequest& passedNewRequest, const WebURLResponse& passedRedirectResponse)
 {
     ASSERT(m_state != ConnectionStateReleased);
     ASSERT(!passedNewRequest.isNull());
     ASSERT(!passedRedirectResponse.isNull());
 
     ResourceRequest& newRequest(passedNewRequest.toMutableResourceRequest());
     const ResourceResponse& redirectResponse(passedRedirectResponse.toResourceResponse());
     newRequest.setFollowedRedirect(true);
 
     if (m_fetcher->willFollowRedirect(m_resource.get(), newRequest, redirectResponse)) {
         m_resource->willFollowRedirect(newRequest, redirectResponse);
     } else {
         m_resource->willNotFollowRedirect();
-        cancel(ResourceError::cancelledDueToAccessCheckError(newRequest.url()));
+        if (m_state != ConnectionStateReleased)
+            cancel(ResourceError::cancelledDueToAccessCheckError(newRequest.url()));
     }
 }
 
 void ResourceLoader::didReceiveCachedMetadata(WebURLLoader*, const char* data, int length)
 {
     RELEASE_ASSERT(m_state == ConnectionStateReceivedResponse || m_state == ConnectionStateReceivingData);
     m_resource->setSerializedCachedMetadata(data, length);
 }
 
 void ResourceLoader::didSendData(WebURLLoader*, unsigned long long bytesSent, unsigned long long totalBytesToBeSent)
@@ skipping 30 matching lines @@
                 ResourceRequest request = m_resource->lastResourceRequest();
                 ASSERT(!request.skipServiceWorker());
                 request.setSkipServiceWorker(true);
                 m_loader->loadAsynchronously(WrappedResourceRequest(request), this);
                 return;
             }
         } else {
             if (!m_resource->isCacheValidator() || resourceResponse.httpStatusCode() != 304)
                 m_resource->setResponse(resourceResponse);
             if (!m_fetcher->canAccessResource(m_resource.get(), m_resource->options().securityOrigin.get(), response.url(), ResourceFetcher::ShouldLogAccessControlErrors)) {
                 m_fetcher->didReceiveResponse(m_resource.get(), resourceResponse);

[yhirano, 2016/05/02 12:38:45]

Do we need |if (m_state != Released) | here?

[Nate Chapin, 2016/05/02 20:07:37]

canAccessResource() doesn't cancel() the ResourceLoader, so it's currently safe
without the Released check.

                 cancel(ResourceError::cancelledDueToAccessCheckError(KURL(response.url())));
                 return;
             }
         }
     }
 
     m_resource->responseReceived(resourceResponse, handle.release());
     if (m_state == ConnectionStateReleased)
         return;
 
     m_fetcher->didReceiveResponse(m_resource.get(), resourceResponse);
     if (m_state == ConnectionStateReleased)
         return;
 
     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);
     cancel(ResourceError::cancelledError(resourceResponse.url()));
 }
 
 void ResourceLoader::didReceiveResponse(WebURLLoader* loader, const WebURLResponse& response)
 {
     didReceiveResponse(loader, response, nullptr);
 }
 
 void ResourceLoader::didReceiveData(WebURLLoader*, const char* data, int length, int encodedDataLength)
 {
@@ skipping 10 matching lines @@
     // Could be an issue with a giant local file.
     m_fetcher->didReceiveData(m_resource.get(), data, length, encodedDataLength);
     if (m_state == ConnectionStateReleased)
         return;
     RELEASE_ASSERT(length >= 0);
     m_resource->appendData(data, 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());
-
     m_resource->setLoadFinishTime(finishTime);
     didFinishLoadingOnePart(finishTime, encodedDataLength);
-    if (m_state == ConnectionStateReleased)
-        return;
     m_resource->finish();

[hiroshige, 2016/05/02 06:39:27]

How about adding ASSERT(m_state != ConnectionStateReleased) before and after
this line? Such ASSERT()s might be helpful for preventing the reentrancy from
occurring again.

[Nate Chapin, 2016/05/02 20:07:37]

Done.

[hiroshige, 2016/05/10 10:22:48]

Ack. (memo: ASSERT() after this line is not needed (and not added) because
releaseResources() already has one.)

-
-    // 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;

[hiroshige, 2016/05/02 06:39:27]

How about adding ASSERT(!m_notifiedLoadComplete) before this line?

[Nate Chapin, 2016/05/02 20:07:37]

This isn't always the case as-is. It can be true here if a cancel() occurs after
the first part in a multipart load. I want to get rid of this bit eventually :/

[hiroshige, 2016/05/10 10:22:48]

Hmm, so this CL adds ResourceFetcher::didFailLoading() in such cases. Is it OK?

I feel we should call didFailLoading() because removeResourceLoader() in
didFailLoading() should be called in order to clear ResourceLoader from
ResourceFetcher's loaders list (I made ResourceLoader to remain 
in ResourceFetcher::m_nonBlockingLoaders after the first part loading of a
multipart load).
But not so sure about other parts of didFailLoading().

[Nate Chapin, 2016/05/10 18:26:17]

The only non-test difference I see will be that the failure will appear in
console (if the failure is not a cancellation) and inspector. It seems right to
report a load failure in the console, even if it's after the first part
completes. I'm not absolutely certain it's benign to report the failure to
inspector, but my best guess is yes: the message goes straight to the frontend,
no state is maintained in core/inspector based on it.

[hiroshige, 2016/05/11 09:28:38]

Sounds reasonable.
Please mention briefly about this behavior change in the CL description.

[Nate Chapin, 2016/05/11 18:49:53]

Done.

+    m_fetcher->didFailLoading(m_resource.get(), error);
     m_resource->error(Resource::LoadError);

[hiroshige, 2016/05/02 06:39:27]

How about adding ASSERT(m_state != ConnectionStateReleased) before and after
this line?

[Nate Chapin, 2016/05/02 20:07:37]

Done.

-
-    if (m_state == ConnectionStateReleased)
-        return;
-
     releaseResources();
 }
 
 void ResourceLoader::requestSynchronously(ResourceRequest& request)
 {
     // downloadToFile is not supported for synchronous requests.
     ASSERT(!request.downloadToFile());
     ASSERT(m_loader);
 
     // Synchronous requests should always be max priority, lest they hang the renderer.
@@ skipping 31 matching lines @@
     // empty buffer is a noop in most cases, but is destructive in the case of
     // a 304, where it will overwrite the cached data we should be reusing.
     if (dataOut.size()) {
         m_fetcher->didReceiveData(m_resource.get(), dataOut.data(), dataOut.size(), encodedDataLength);
         m_resource->setResourceBuffer(dataOut);
     }
     didFinishLoading(0, monotonicallyIncreasingTime(), encodedDataLength);
 }
 
 } // namespace blink