Source/WebCore: Don't re-enter CachedResource::removeClient() if an XHR
authorjaphet@chromium.org <japhet@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Jun 2012 18:42:51 +0000 (18:42 +0000)
committerjaphet@chromium.org <japhet@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Jun 2012 18:42:51 +0000 (18:42 +0000)
is canceled and restarted multiple times.
https://bugs.webkit.org/show_bug.cgi?id=89378

Reviewed by Eric Seidel.

Test: http/tests/xmlhttprequest/reentrant-cancel.html

* loader/DocumentThreadableLoader.cpp:
(WebCore::DocumentThreadableLoader::cancel):
(WebCore::DocumentThreadableLoader::clearResource): Save off a copy of m_resource
   then clear it, so we don't call clearResource() multiple times for the same resource.

LayoutTests: Test for https://bugs.webkit.org/show_bug.cgi?id=89378.
Reviewed by Eric Seidel.

* http/tests/xmlhttprequest/reentrant-cancel-expected.txt: Added.
* http/tests/xmlhttprequest/reentrant-cancel.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/xmlhttprequest/reentrant-cancel.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/loader/DocumentThreadableLoader.cpp

index dbe0acf..19e8c81 100644 (file)
@@ -1,3 +1,11 @@
+2012-06-20  Nate Chapin  <japhet@chromium.org>
+
+        Test for https://bugs.webkit.org/show_bug.cgi?id=89378.
+        Reviewed by Eric Seidel.
+
+        * http/tests/xmlhttprequest/reentrant-cancel-expected.txt: Added.
+        * http/tests/xmlhttprequest/reentrant-cancel.html: Added.
+
 2012-06-20  Robert Hogan  <robert@webkit.org>
 
         Negative margin block doesn't properly clear a float enclosed by a previous sibling
diff --git a/LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-expected.txt b/LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-expected.txt
new file mode 100644 (file)
index 0000000..53a8068
--- /dev/null
@@ -0,0 +1 @@
+XThis tests that when we re-entrantly create and cancel XHRs, we don't try to disconnect the same CachedResourceClient multiple times from its CachedResource. We pass if we don't crash. XX
diff --git a/LayoutTests/http/tests/xmlhttprequest/reentrant-cancel.html b/LayoutTests/http/tests/xmlhttprequest/reentrant-cancel.html
new file mode 100644 (file)
index 0000000..3ea5536
--- /dev/null
@@ -0,0 +1,21 @@
+<script>
+if (window.layoutTestController)
+    layoutTestController.dumpAsText();
+
+function addElement() {
+    document.documentElement.appendChild(document.createTextNode('X'));
+}
+document.addEventListener("DOMContentLoaded", addElement, false);
+window.onload = addElement;
+
+var xhr = new XMLHttpRequest;
+function sendXHR()
+{
+    xhr.open("GET", "", true);
+    xhr.send();
+}
+window.addEventListener("DOMSubtreeModified", sendXHR);
+addElement();
+</script>
+This tests that when we re-entrantly create and cancel XHRs, we don't try to disconnect the same CachedResourceClient
+multiple times from its CachedResource. We pass if we don't crash.
index 18565e8..43344de 100644 (file)
@@ -1,3 +1,18 @@
+2012-06-20  Nate Chapin  <japhet@chromium.org>
+
+        Don't re-enter CachedResource::removeClient() if an XHR
+        is canceled and restarted multiple times.
+        https://bugs.webkit.org/show_bug.cgi?id=89378
+
+        Reviewed by Eric Seidel.
+
+        Test: http/tests/xmlhttprequest/reentrant-cancel.html
+
+        * loader/DocumentThreadableLoader.cpp:
+        (WebCore::DocumentThreadableLoader::cancel):
+        (WebCore::DocumentThreadableLoader::clearResource): Save off a copy of m_resource
+           then clear it, so we don't call clearResource() multiple times for the same resource.
+
 2012-06-20  Robert Hogan  <robert@webkit.org>
 
         Negative margin block doesn't properly clear a float enclosed by a previous sibling
index 1c22bb4..aeb4c69 100644 (file)
@@ -146,7 +146,8 @@ DocumentThreadableLoader::~DocumentThreadableLoader()
 
 void DocumentThreadableLoader::cancel()
 {
-    if (m_client) {
+    // Cacnel can re-enter and m_resource might be null here as a result.
+    if (m_client && m_resource) {
         ResourceError error(errorDomainWebKitInternal, 0, m_resource->url(), "Load cancelled");
         error.setIsCancellation(true);
         didFail(error);
@@ -163,9 +164,13 @@ void DocumentThreadableLoader::setDefersLoading(bool value)
 
 void DocumentThreadableLoader::clearResource()
 {
-    if (m_resource) {
-        m_resource->removeClient(this);
+    // Script can cancel and restart a request reentrantly within removeClient(),
+    // which could lead to calling CachedResource::removeClient() multiple times for
+    // this DocumentThreadableLoader. Save off a copy of m_resource and clear it to
+    // prevent the reentrancy.
+    if (CachedResourceHandle<CachedRawResource> resource = m_resource) {
         m_resource = 0;
+        resource->removeClient(this);
     }
 }