- Revision
- 86720
- Author
- [email protected]
- Date
- 2011-05-17 16:42:36 -0700 (Tue, 17 May 2011)
Log Message
2011-05-17 Brady Eidson <[email protected]>
Reviewed by Darin Adler.
<rdar://problem/9366728> and https://webkit.org/b/60796
Crash when code inside a ResourceLoadDelegate method calls [WebView stopLoading:]
Break up ResourceLoader::didCancel() into willCancel() and didCancel(), and making them pure virtual.
This change has the following benefits:
- Managing ResourceLoader state can be in the base class; Subclasses no longer need to protect
themselves, check these variables as often, or ASSERT them.
- ResourceLoader subclasses no longer have to call the base class ::didCancel
- ResourceLoader::cancel becomes more capable of handling reentrancy with the design that the
cancellation is completed inside the last call.
No new tests - No change in behavior for previous tests, and new test would require API usage outside
the scope of DumpRenderTree.
* loader/ResourceLoader.cpp:
(WebCore::ResourceLoader::ResourceLoader):
(WebCore::ResourceLoader::cancel): Moved from ResourceLoader::didCancel, and does all of that same work
except it interposes calls to "willCancel" and "didCancel" as required to maintain the same behavior.
* loader/ResourceLoader.h: Added pure virtual didCancel() and willCancel().
Split-up into willCancel() and didCancel(), based on when the base class didCancel() used to be called:
* loader/MainResourceLoader.cpp:
(WebCore::MainResourceLoader::willCancel):
(WebCore::MainResourceLoader::didCancel):
* loader/MainResourceLoader.h:
Split-up into willCancel() and didCancel(), based on when the "reached terminal state" flag used to be checked:
* loader/NetscapePlugInStreamLoader.cpp:
(WebCore::NetscapePlugInStreamLoader::didReceiveResponse): Call the entry point cancel() instead of the old didCancel()
(WebCore::NetscapePlugInStreamLoader::willCancel):
(WebCore::NetscapePlugInStreamLoader::didCancel):
* loader/NetscapePlugInStreamLoader.h:
Split-up into willCancel() and didCancel(), based on when the "reached terminal state" flag used to be checked:
* loader/SubresourceLoader.cpp:
(WebCore::SubresourceLoader::willCancel):
(WebCore::SubresourceLoader::didCancel):
* loader/SubresourceLoader.h:
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (86719 => 86720)
--- trunk/Source/WebCore/ChangeLog 2011-05-17 23:39:57 UTC (rev 86719)
+++ trunk/Source/WebCore/ChangeLog 2011-05-17 23:42:36 UTC (rev 86720)
@@ -1,3 +1,46 @@
+2011-05-17 Brady Eidson <[email protected]>
+
+ Reviewed by Darin Adler.
+
+ <rdar://problem/9366728> and https://webkit.org/b/60796
+ Crash when code inside a ResourceLoadDelegate method calls [WebView stopLoading:]
+
+ Break up ResourceLoader::didCancel() into willCancel() and didCancel(), and making them pure virtual.
+ This change has the following benefits:
+ - Managing ResourceLoader state can be in the base class; Subclasses no longer need to protect
+ themselves, check these variables as often, or ASSERT them.
+ - ResourceLoader subclasses no longer have to call the base class ::didCancel
+ - ResourceLoader::cancel becomes more capable of handling reentrancy with the design that the
+ cancellation is completed inside the last call.
+
+ No new tests - No change in behavior for previous tests, and new test would require API usage outside
+ the scope of DumpRenderTree.
+
+ * loader/ResourceLoader.cpp:
+ (WebCore::ResourceLoader::ResourceLoader):
+ (WebCore::ResourceLoader::cancel): Moved from ResourceLoader::didCancel, and does all of that same work
+ except it interposes calls to "willCancel" and "didCancel" as required to maintain the same behavior.
+ * loader/ResourceLoader.h: Added pure virtual didCancel() and willCancel().
+
+ Split-up into willCancel() and didCancel(), based on when the base class didCancel() used to be called:
+ * loader/MainResourceLoader.cpp:
+ (WebCore::MainResourceLoader::willCancel):
+ (WebCore::MainResourceLoader::didCancel):
+ * loader/MainResourceLoader.h:
+
+ Split-up into willCancel() and didCancel(), based on when the "reached terminal state" flag used to be checked:
+ * loader/NetscapePlugInStreamLoader.cpp:
+ (WebCore::NetscapePlugInStreamLoader::didReceiveResponse): Call the entry point cancel() instead of the old didCancel()
+ (WebCore::NetscapePlugInStreamLoader::willCancel):
+ (WebCore::NetscapePlugInStreamLoader::didCancel):
+ * loader/NetscapePlugInStreamLoader.h:
+
+ Split-up into willCancel() and didCancel(), based on when the "reached terminal state" flag used to be checked:
+ * loader/SubresourceLoader.cpp:
+ (WebCore::SubresourceLoader::willCancel):
+ (WebCore::SubresourceLoader::didCancel):
+ * loader/SubresourceLoader.h:
+
2011-05-17 Nat Duca <[email protected]>
Reviewed by James Robinson.
Modified: trunk/Source/WebCore/loader/MainResourceLoader.cpp (86719 => 86720)
--- trunk/Source/WebCore/loader/MainResourceLoader.cpp 2011-05-17 23:39:57 UTC (rev 86719)
+++ trunk/Source/WebCore/loader/MainResourceLoader.cpp 2011-05-17 23:42:36 UTC (rev 86720)
@@ -99,24 +99,23 @@
ASSERT(reachedTerminalState());
}
-void MainResourceLoader::didCancel(const ResourceError& error)
+void MainResourceLoader::willCancel(const ResourceError&)
{
m_dataLoadTimer.stop();
- // FrameLoader won't be reachable after calling ResourceLoader::didCancel().
- FrameLoader* frameLoader = this->frameLoader();
-
if (m_waitingForContentPolicy) {
- frameLoader->policyChecker()->cancelCheck();
+ frameLoader()->policyChecker()->cancelCheck();
ASSERT(m_waitingForContentPolicy);
m_waitingForContentPolicy = false;
deref(); // balances ref in didReceiveResponse
}
- ResourceLoader::didCancel(error);
+}
+void MainResourceLoader::didCancel(const ResourceError& error)
+{
// We should notify the frame loader after fully canceling the load, because it can do complicated work
// like calling DOMWindow::print(), during which a half-canceled load could try to finish.
- frameLoader->receivedMainResourceError(error, true);
+ frameLoader()->receivedMainResourceError(error, true);
}
ResourceError MainResourceLoader::interruptionForPolicyChangeError() const
Modified: trunk/Source/WebCore/loader/MainResourceLoader.h (86719 => 86720)
--- trunk/Source/WebCore/loader/MainResourceLoader.h 2011-05-17 23:39:57 UTC (rev 86719)
+++ trunk/Source/WebCore/loader/MainResourceLoader.h 2011-05-17 23:42:36 UTC (rev 86720)
@@ -74,6 +74,7 @@
private:
MainResourceLoader(Frame*);
+ virtual void willCancel(const ResourceError&);
virtual void didCancel(const ResourceError&);
bool loadNow(ResourceRequest&);
Modified: trunk/Source/WebCore/loader/NetscapePlugInStreamLoader.cpp (86719 => 86720)
--- trunk/Source/WebCore/loader/NetscapePlugInStreamLoader.cpp 2011-05-17 23:39:57 UTC (rev 86719)
+++ trunk/Source/WebCore/loader/NetscapePlugInStreamLoader.cpp 2011-05-17 23:42:36 UTC (rev 86720)
@@ -89,7 +89,7 @@
return;
if (response.httpStatusCode() < 100 || response.httpStatusCode() >= 400)
- didCancel(frameLoader()->fileDoesNotExistError(response));
+ cancel(frameLoader()->fileDoesNotExistError(response));
}
void NetscapePlugInStreamLoader::didReceiveData(const char* data, int length, long long encodedDataLength, bool allAtOnce)
@@ -119,23 +119,17 @@
ResourceLoader::didFail(error);
}
-void NetscapePlugInStreamLoader::didCancel(const ResourceError& error)
+void NetscapePlugInStreamLoader::willCancel(const ResourceError& error)
{
- RefPtr<NetscapePlugInStreamLoader> protect(this);
-
m_client->didFail(this, error);
+}
- // If calling didFail spins the run loop the stream loader can reach the terminal state.
- // If that's the case we just return early.
- if (reachedTerminalState())
- return;
-
+void NetscapePlugInStreamLoader::didCancel(const ResourceError&)
+{
// We need to remove the stream loader after the call to didFail, since didFail can
// spawn a new run loop and if the loader has been removed it won't be deferred when
// the document loader is asked to defer loading.
m_documentLoader->removePlugInStreamLoader(this);
-
- ResourceLoader::didCancel(error);
}
}
Modified: trunk/Source/WebCore/loader/NetscapePlugInStreamLoader.h (86719 => 86720)
--- trunk/Source/WebCore/loader/NetscapePlugInStreamLoader.h 2011-05-17 23:39:57 UTC (rev 86719)
+++ trunk/Source/WebCore/loader/NetscapePlugInStreamLoader.h 2011-05-17 23:42:36 UTC (rev 86720)
@@ -65,7 +65,8 @@
NetscapePlugInStreamLoader(Frame*, NetscapePlugInStreamLoaderClient*);
- virtual void didCancel(const ResourceError& error);
+ virtual void willCancel(const ResourceError&);
+ virtual void didCancel(const ResourceError&);
NetscapePlugInStreamLoaderClient* m_client;
};
Modified: trunk/Source/WebCore/loader/ResourceLoader.cpp (86719 => 86720)
--- trunk/Source/WebCore/loader/ResourceLoader.cpp 2011-05-17 23:39:57 UTC (rev 86719)
+++ trunk/Source/WebCore/loader/ResourceLoader.cpp 2011-05-17 23:42:36 UTC (rev 86720)
@@ -63,6 +63,7 @@
, m_documentLoader(frame->loader()->activeDocumentLoader())
, m_identifier(0)
, m_reachedTerminalState(false)
+ , m_calledWillCancel(false)
, m_cancelled(false)
, m_calledDidFinishLoad(false)
, m_sendResourceLoadCallbacks(sendResourceLoadCallbacks)
@@ -337,35 +338,6 @@
releaseResources();
}
-void ResourceLoader::didCancel(const ResourceError& error)
-{
- ASSERT(!m_cancelled);
- ASSERT(!m_reachedTerminalState);
-
- if (FormData* data = ""
- data->removeGeneratedFilesIfNeeded();
-
- // This flag prevents bad behavior when loads that finish cause the
- // load itself to be cancelled (which could happen with a _javascript_ that
- // changes the window location). This is used to prevent both the body
- // of this method and the body of connectionDidFinishLoading: running
- // for a single delegate. Canceling wins.
- m_cancelled = true;
-
- if (m_handle)
- m_handle->clearAuthentication();
-
- m_documentLoader->cancelPendingSubstituteLoad(this);
- if (m_handle) {
- m_handle->cancel();
- m_handle = 0;
- }
- if (m_sendResourceLoadCallbacks && m_identifier && !m_calledDidFinishLoad)
- frameLoader()->notifier()->didFailToLoad(this, error);
-
- releaseResources();
-}
-
void ResourceLoader::cancel()
{
cancel(ResourceError());
@@ -373,12 +345,53 @@
void ResourceLoader::cancel(const ResourceError& error)
{
+ // If the load has already completed - succeeded, failed, or previously cancelled - do nothing.
if (m_reachedTerminalState)
return;
- if (!error.isNull())
- didCancel(error);
- else
- didCancel(cancelledError());
+
+ ResourceError nonNullError = error.isNull() ? cancelledError() : error;
+
+ // willCancel() and didFailToLoad() both call out to clients that might do
+ // something causing the last reference to this object to go away.
+ RefPtr<ResourceLoader> protector(this);
+
+ // 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;
+
+ 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 = ""
+ data->removeGeneratedFilesIfNeeded();
+
+ if (m_handle)
+ m_handle->clearAuthentication();
+
+ m_documentLoader->cancelPendingSubstituteLoad(this);
+ if (m_handle) {
+ m_handle->cancel();
+ m_handle = 0;
+ }
+
+ if (m_sendResourceLoadCallbacks && m_identifier && !m_calledDidFinishLoad)
+ frameLoader()->notifier()->didFailToLoad(this, nonNullError);
+ }
+
+ // If cancel() completed from within the call to willCancel() or didFailToLoad(),
+ // we don't want to redo didCancel() or releasesResources().
+ if (m_reachedTerminalState)
+ return;
+
+ didCancel(nonNullError);
+
+ releaseResources();
}
const ResourceResponse& ResourceLoader::response() const
Modified: trunk/Source/WebCore/loader/ResourceLoader.h (86719 => 86720)
--- trunk/Source/WebCore/loader/ResourceLoader.h 2011-05-17 23:39:57 UTC (rev 86719)
+++ trunk/Source/WebCore/loader/ResourceLoader.h 2011-05-17 23:42:36 UTC (rev 86720)
@@ -150,7 +150,6 @@
// deferred) and should only be called by ResourceLoadScheduler or setDefersLoading().
void start();
- virtual void didCancel(const ResourceError&);
void didFinishLoadingOnePart(double finishTime);
const ResourceRequest& request() const { return m_request; }
@@ -163,12 +162,16 @@
ResourceResponse m_response;
private:
+ virtual void willCancel(const ResourceError&) = 0;
+ virtual void didCancel(const ResourceError&) = 0;
+
ResourceRequest m_request;
RefPtr<SharedBuffer> m_resourceData;
unsigned long m_identifier;
bool m_reachedTerminalState;
+ bool m_calledWillCancel;
bool m_cancelled;
bool m_calledDidFinishLoad;
Modified: trunk/Source/WebCore/loader/SubresourceLoader.cpp (86719 => 86720)
--- trunk/Source/WebCore/loader/SubresourceLoader.cpp 2011-05-17 23:39:57 UTC (rev 86719)
+++ trunk/Source/WebCore/loader/SubresourceLoader.cpp 2011-05-17 23:42:36 UTC (rev 86720)
@@ -224,27 +224,15 @@
ResourceLoader::didFail(error);
}
-void SubresourceLoader::didCancel(const ResourceError& error)
+void SubresourceLoader::willCancel(const ResourceError& error)
{
- ASSERT(!reachedTerminalState());
-
- // Calling removeSubresourceLoader will likely result in a call to deref, so we must protect ourselves.
- RefPtr<SubresourceLoader> protect(this);
-
if (m_client)
m_client->didFail(this, error);
-
- if (cancelled())
- return;
-
- // The only way the subresource loader can reach the terminal state here is if the run loop spins when calling
- // m_client->didFail. This should in theory not happen which is why the assert is here.
- ASSERT(!reachedTerminalState());
- if (reachedTerminalState())
- return;
-
+}
+
+void SubresourceLoader::didCancel(const ResourceError&)
+{
m_documentLoader->removeSubresourceLoader(this);
- ResourceLoader::didCancel(error);
}
bool SubresourceLoader::shouldUseCredentialStorage()
Modified: trunk/Source/WebCore/loader/SubresourceLoader.h (86719 => 86720)
--- trunk/Source/WebCore/loader/SubresourceLoader.h 2011-05-17 23:39:57 UTC (rev 86719)
+++ trunk/Source/WebCore/loader/SubresourceLoader.h 2011-05-17 23:42:36 UTC (rev 86720)
@@ -59,6 +59,7 @@
virtual bool shouldUseCredentialStorage();
virtual void didReceiveAuthenticationChallenge(const AuthenticationChallenge&);
virtual void receivedCancellation(const AuthenticationChallenge&);
+ virtual void willCancel(const ResourceError&);
virtual void didCancel(const ResourceError&);
#if HAVE(CFNETWORK_DATA_ARRAY_CALLBACK)
