Crash in WebCore::SubresourceLoader::releaseResources when connection fails
authorjaphet@chromium.org <japhet@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 29 May 2013 05:29:54 +0000 (05:29 +0000)
committerjaphet@chromium.org <japhet@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 29 May 2013 05:29:54 +0000 (05:29 +0000)
https://bugs.webkit.org/show_bug.cgi?id=87743

Don't do anything complicated in SubresourceLoader::releaseResources(),
just clear variables. With this patch, releaseResources() will still
assert in debug builds if it is called twice, but it will safely execute
in release.

Reviewed by Darin Adler.

* loader/ResourceLoader.cpp:
(WebCore::ResourceLoader::cleanupForError): Pull shared cleanup code out of didFail()
    and cancel() into a helper.
(WebCore::ResourceLoader::cancel): Merge a couple variables into an enum, check for
    reentrancy from within didCancel().
* loader/ResourceLoader.h: Replace m_calledWillCancel and m_cancelled with an enum.
* loader/SubresourceLoader.cpp:
(WebCore::SubresourceLoader::didFinishLoading): Don't call ResourceLoader::didFinishLoading(),
    put finish() in the middle of the process.
(WebCore::SubresourceLoader::didFail): Don't call ResourceLoader::didFail(), put finish()
    in the middle of the process.
(WebCore::SubresourceLoader::didCancel):
(WebCore::SubresourceLoader::notifyDone): Do the non-trivial work previous done in releaseResources(),
    most importantly calling loadDone().
(WebCore::SubresourceLoader::releaseResources): Only do simple variable clearing here.
* loader/SubresourceLoader.h:
(SubresourceLoader):
* loader/cache/CachedResource.cpp: Split stopLoading() into cancelLoad() (which notifies clients)
    and clearLoader() (which just nulls m_loader).
* loader/cache/CachedResource.h:
* loader/chromium/ResourceLoaderChromium.cpp:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@150867 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/loader/ResourceLoader.cpp
Source/WebCore/loader/ResourceLoader.h
Source/WebCore/loader/SubresourceLoader.cpp
Source/WebCore/loader/SubresourceLoader.h
Source/WebCore/loader/cache/CachedResource.cpp
Source/WebCore/loader/cache/CachedResource.h

index 09d64b4..ab216b9 100644 (file)
@@ -1,3 +1,37 @@
+2013-05-28  Nate Chapin  <japhet@chromium.org>
+
+        Crash in WebCore::SubresourceLoader::releaseResources when connection fails
+        https://bugs.webkit.org/show_bug.cgi?id=87743
+
+        Don't do anything complicated in SubresourceLoader::releaseResources(),
+        just clear variables. With this patch, releaseResources() will still
+        assert in debug builds if it is called twice, but it will safely execute
+        in release.
+
+        Reviewed by Darin Adler.
+
+        * loader/ResourceLoader.cpp:
+        (WebCore::ResourceLoader::cleanupForError): Pull shared cleanup code out of didFail()
+            and cancel() into a helper.
+        (WebCore::ResourceLoader::cancel): Merge a couple variables into an enum, check for
+            reentrancy from within didCancel().
+        * loader/ResourceLoader.h: Replace m_calledWillCancel and m_cancelled with an enum.
+        * loader/SubresourceLoader.cpp:
+        (WebCore::SubresourceLoader::didFinishLoading): Don't call ResourceLoader::didFinishLoading(),
+            put finish() in the middle of the process.
+        (WebCore::SubresourceLoader::didFail): Don't call ResourceLoader::didFail(), put finish()
+            in the middle of the process.
+        (WebCore::SubresourceLoader::didCancel):
+        (WebCore::SubresourceLoader::notifyDone): Do the non-trivial work previous done in releaseResources(),
+            most importantly calling loadDone().
+        (WebCore::SubresourceLoader::releaseResources): Only do simple variable clearing here.
+        * loader/SubresourceLoader.h:
+        (SubresourceLoader):
+        * loader/cache/CachedResource.cpp: Split stopLoading() into cancelLoad() (which notifies clients)
+            and clearLoader() (which just nulls m_loader).
+        * loader/cache/CachedResource.h:
+        * loader/chromium/ResourceLoaderChromium.cpp:
+
 2013-05-28  Seokju Kwon  <seokju.kwon@gmail.com>
 
         [GTK] Build fix after r150837
index dc083c2..45b3a30 100644 (file)
@@ -62,9 +62,8 @@ ResourceLoader::ResourceLoader(Frame* frame, ResourceLoaderOptions options)
     , m_documentLoader(frame->loader()->activeDocumentLoader())
     , m_identifier(0)
     , m_reachedTerminalState(false)
-    , m_calledWillCancel(false)
-    , m_cancelled(false)
     , m_notifiedLoadComplete(false)
+    , m_cancellationStatus(NotCancelled)
     , m_defersLoading(frame->page()->defersLoading())
     , m_options(options)
 {
@@ -332,7 +331,7 @@ void ResourceLoader::didFinishLoading(double finishTime)
 
     // 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_cancelled)
+    if (wasCancelled())
         return;
     releaseResources();
 }
@@ -341,7 +340,7 @@ void ResourceLoader::didFinishLoadingOnePart(double finishTime)
 {
     // If load has been cancelled after finishing (which could happen with a
     // JavaScript that changes the window location), do nothing.
-    if (m_cancelled)
+    if (wasCancelled())
         return;
     ASSERT(!m_reachedTerminalState);
 
@@ -354,7 +353,7 @@ void ResourceLoader::didFinishLoadingOnePart(double finishTime)
 
 void ResourceLoader::didFail(const ResourceError& error)
 {
-    if (m_cancelled)
+    if (wasCancelled())
         return;
     ASSERT(!m_reachedTerminalState);
 
@@ -362,16 +361,20 @@ void ResourceLoader::didFail(const ResourceError& error)
     // anything including possibly derefing this; one example of this is Radar 3266216.
     RefPtr<ResourceLoader> protector(this);
 
+    cleanupForError(error);
+    releaseResources();
+}
+
+void ResourceLoader::cleanupForError(const ResourceError& error)
+{
     if (FormData* data = m_request.httpBody())
         data->removeGeneratedFilesIfNeeded();
 
-    if (!m_notifiedLoadComplete) {
-        m_notifiedLoadComplete = true;
-        if (m_options.sendLoadCallbacks == SendCallbacks)
-            frameLoader()->notifier()->didFailToLoad(this, error);
-    }
-
-    releaseResources();
+    if (m_notifiedLoadComplete)
+        return;
+    m_notifiedLoadComplete = true;
+    if (m_options.sendLoadCallbacks == SendCallbacks && m_identifier)
+        frameLoader()->notifier()->didFailToLoad(this, error);
 }
 
 void ResourceLoader::didChangePriority(ResourceLoadPriority loadPriority)
@@ -401,19 +404,16 @@ void ResourceLoader::cancel(const ResourceError& error)
     
     // If we re-enter cancel() from inside willCancel(), we want to pick up from where we left 
     // off without re-running willCancel()
-    if (!m_calledWillCancel) {
-        m_calledWillCancel = true;
+    if (m_cancellationStatus == NotCancelled) {
+        m_cancellationStatus = CalledWillCancel;
         
         willCancel(nonNullError);
     }
 
     // If we re-enter cancel() from inside didFailToLoad(), we want to pick up from where we 
     // left off without redoing any of this work.
-    if (!m_cancelled) {
-        m_cancelled = true;
-        
-        if (FormData* data = m_request.httpBody())
-            data->removeGeneratedFilesIfNeeded();
+    if (m_cancellationStatus == CalledWillCancel) {
+        m_cancellationStatus = Cancelled;
 
         if (m_handle)
             m_handle->clearAuthentication();
@@ -423,9 +423,7 @@ void ResourceLoader::cancel(const ResourceError& error)
             m_handle->cancel();
             m_handle = 0;
         }
-
-        if (m_options.sendLoadCallbacks == SendCallbacks && m_identifier && !m_notifiedLoadComplete)
-            frameLoader()->notifier()->didFailToLoad(this, nonNullError);
+        cleanupForError(nonNullError);
     }
 
     // If cancel() completed from within the call to willCancel() or didFailToLoad(),
@@ -434,7 +432,11 @@ void ResourceLoader::cancel(const ResourceError& error)
         return;
 
     didCancel(nonNullError);
-            
+
+    if (m_cancellationStatus == FinishedCancel)
+        return;
+    m_cancellationStatus = FinishedCancel;
+
     releaseResources();
 }
 
index 86cdb4e..f530d08 100644 (file)
@@ -154,8 +154,9 @@ protected:
     void start();
 
     void didFinishLoadingOnePart(double finishTime);
+    void cleanupForError(const ResourceError&);
 
-    bool cancelled() const { return m_cancelled; }
+    bool wasCancelled() const { return m_cancellationStatus >= Cancelled; }
 
     void didReceiveDataOrBuffer(const char*, int, PassRefPtr<SharedBuffer>, long long encodedDataLength, DataPayloadType);
 
@@ -177,10 +178,16 @@ private:
     unsigned long m_identifier;
 
     bool m_reachedTerminalState;
-    bool m_calledWillCancel;
-    bool m_cancelled;
     bool m_notifiedLoadComplete;
 
+    enum CancellationStatus {
+        NotCancelled,
+        CalledWillCancel,
+        Cancelled,
+        FinishedCancel
+    };
+    CancellationStatus m_cancellationStatus;
+
     bool m_defersLoading;
     ResourceRequest m_deferredRequest;
     ResourceLoaderOptions m_options;
index f4d318d..24bc637 100644 (file)
@@ -280,8 +280,16 @@ void SubresourceLoader::didFinishLoading(double finishTime)
     m_state = Finishing;
     m_resource->setLoadFinishTime(finishTime);
     m_resource->data(resourceData(), true);
+
+    if (wasCancelled())
+        return;
     m_resource->finish();
-    ResourceLoader::didFinishLoading(finishTime);
+    ASSERT(!reachedTerminalState());
+    didFinishLoadingOnePart(finishTime);
+    notifyDone();
+    if (reachedTerminalState())
+        return;
+    releaseResources();
 }
 
 void SubresourceLoader::didFail(const ResourceError& error)
@@ -297,10 +305,14 @@ void SubresourceLoader::didFail(const ResourceError& error)
     if (m_resource->resourceToRevalidate())
         memoryCache()->revalidationFailed(m_resource);
     m_resource->setResourceError(error);
-    m_resource->error(CachedResource::LoadError);
     if (!m_resource->isPreloaded())
         memoryCache()->remove(m_resource);
-    ResourceLoader::didFail(error);
+    m_resource->error(CachedResource::LoadError);
+    cleanupForError(error);
+    notifyDone();
+    if (reachedTerminalState())
+        return;
+    releaseResources();
 }
 
 void SubresourceLoader::willCancel(const ResourceError& error)
@@ -318,17 +330,30 @@ void SubresourceLoader::willCancel(const ResourceError& error)
     memoryCache()->remove(m_resource);
 }
 
+void SubresourceLoader::didCancel(const ResourceError&)
+{
+    m_resource->cancelLoad();
+    notifyDone();
+}
+
+void SubresourceLoader::notifyDone()
+{
+    if (reachedTerminalState())
+        return;
+
+    m_requestCountTracker.clear();
+    m_documentLoader->cachedResourceLoader()->loadDone(m_resource);
+    if (reachedTerminalState())
+        return;
+    m_documentLoader->removeSubresourceLoader(this);
+}
+
 void SubresourceLoader::releaseResources()
 {
     ASSERT(!reachedTerminalState());
-    if (m_state != Uninitialized) {
-        m_requestCountTracker.clear();
-        m_documentLoader->cachedResourceLoader()->loadDone(m_resource);
-        if (reachedTerminalState())
-            return;
-        m_resource->stopLoading();
-        m_documentLoader->removeSubresourceLoader(this);
-    }
+    if (m_state != Uninitialized)
+        m_resource->clearLoader();
+    m_resource = 0;
     ResourceLoader::releaseResources();
 }
 
index 6cb6cbc..4e5ecdc 100644 (file)
@@ -64,7 +64,7 @@ private:
     virtual void didFinishLoading(double finishTime) OVERRIDE;
     virtual void didFail(const ResourceError&) OVERRIDE;
     virtual void willCancel(const ResourceError&) OVERRIDE;
-    virtual void didCancel(const ResourceError&) OVERRIDE { }
+    virtual void didCancel(const ResourceError&) OVERRIDE;
 
 #if USE(NETWORK_CFDATA_ARRAY_CALLBACK)
     virtual bool supportsDataArray() OVERRIDE { return true; }
@@ -77,6 +77,8 @@ private:
 
     void didReceiveDataOrBuffer(const char*, int, PassRefPtr<SharedBuffer>, long long encodedDataLength, DataPayloadType);
 
+    void notifyDone();
+
     enum SubresourceLoaderState {
         Uninitialized,
         Initialized,
index 3b2f713..ed38da8 100644 (file)
@@ -380,6 +380,16 @@ void CachedResource::error(CachedResource::Status status)
     setLoading(false);
     checkNotify();
 }
+    
+void CachedResource::cancelLoad()
+{
+    if (!isLoading())
+        return;
+
+    setStatus(LoadError);
+    setLoading(false);
+    checkNotify();
+}
 
 void CachedResource::finish()
 {
@@ -450,21 +460,10 @@ void CachedResource::responseReceived(const ResourceResponse& response)
         setEncoding(encoding);
 }
 
-void CachedResource::stopLoading()
+void CachedResource::clearLoader()
 {
-    ASSERT(m_loader);            
+    ASSERT(m_loader);
     m_loader = 0;
-
-    CachedResourceHandle<CachedResource> protect(this);
-
-    // All loads finish with data(allDataReceived = true) or error(), except for
-    // canceled loads, which silently set our request to 0. Be sure to notify our
-    // client in that case, so we don't seem to continue loading forever.
-    if (isLoading()) {
-        setLoading(false);
-        setStatus(LoadError);
-        checkNotify();
-    }
 }
 
 void CachedResource::addClient(CachedResourceClient* client)
index c55b2fc..32a588e 100644 (file)
@@ -185,7 +185,7 @@ public:
     
     bool inLiveDecodedResourcesList() { return m_inLiveDecodedResourcesList; }
     
-    void stopLoading();
+    void clearLoader();
 
     ResourceBuffer* resourceBuffer() const { ASSERT(!m_purgeableData); return m_data.get(); }
 
@@ -204,6 +204,7 @@ public:
     String accept() const { return m_accept; }
     void setAccept(const String& accept) { m_accept = accept; }
 
+    void cancelLoad();
     bool wasCanceled() const { return m_error.isCancellation(); }
     bool errorOccurred() const { return m_status == LoadError || m_status == DecodeError; }
     bool loadFailedOrCanceled() { return !m_error.isNull(); }