Changeset 174684 in webkit

Timestamp:

Oct 14, 2014 10:10:24 AM (10 years ago)

Author:

commit-queue@webkit.org

Message:

[XHR] Abort method execution when m_loader->cancel() in internalAbort() caused reentry
​https://bugs.webkit.org/show_bug.cgi?id=126975

Patch by Youenn Fablet <​youennf@gmail.com> on 2014-10-14
Reviewed by Alexey Proskuryakov.

Source/WebCore:

Merging ​https://chromium.googlesource.com/chromium/blink/+/0d75daf2053631518606ae15daaece701a25b2c4
Ensuring new test from ​https://codereview.chromium.org/76133002/ is passing.

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

• xml/XMLHttpRequest.cpp:

(WebCore::XMLHttpRequest::open): exit early if internalAbort asks so
(WebCore::XMLHttpRequest::abort): exit early if internalAbort asks so
(WebCore::XMLHttpRequest::internalAbort): ask calling function to exit early if a new loader is created during the cancellation of the loader (potential reentrant case through window.onload callback)
(WebCore::XMLHttpRequest::didTimeout): exit early if internalAbort asks so

• xml/XMLHttpRequest.h:

LayoutTests:

Adding reentrant-cancel-abort.html (from ​https://codereview.chromium.org/76133002/)
that crashes without the patch
Updated reentrant-cancel.html test to expect the exception
that is now hit in initSend function (since a XHR open() call is aborted)

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

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-abort-expected.txt (added)
• LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-abort.html (added)
• LayoutTests/http/tests/xmlhttprequest/reentrant-cancel.html (modified) (2 diffs)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/xml/XMLHttpRequest.cpp (modified) (5 diffs)
• Source/WebCore/xml/XMLHttpRequest.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2014-10-14  Youenn Fablet  <youennf@gmail.com>
+
+        [XHR] Abort method execution when m_loader->cancel() in internalAbort() caused reentry
+        https://bugs.webkit.org/show_bug.cgi?id=126975
+
+        Reviewed by Alexey Proskuryakov.
+
+        Adding reentrant-cancel-abort.html (from https://codereview.chromium.org/76133002/) 
+        that crashes without the patch
+        Updated reentrant-cancel.html test to expect the exception 
+        that is now hit in initSend function (since a XHR open() call is aborted)
+
+        * http/tests/xmlhttprequest/reentrant-cancel-abort-expected.txt: Added.
+        * http/tests/xmlhttprequest/reentrant-cancel-abort.html: Added.
+        * http/tests/xmlhttprequest/reentrant-cancel.html:
+
 2014-10-14  Alejandro G. Castro  <alex@igalia.com>
 

trunk/LayoutTests/http/tests/xmlhttprequest/reentrant-cancel.html

@@ -1 +1 @@
 <script>
-if (window.testRunner)
+if (window.testRunner) {
     testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+}
 
 function addElement() {
@@ -13 +15 @@
 {
     xhr.open("GET", "", true);
-    xhr.send();
+    try {
+        xhr.send();
+    }
+    catch(e) {
+        // In case of reentrant call, the call to "open" will be aborted by reentrant call.
+        // The call to "send" following the "open" aborted call will throw an exception (XHR not in open mode)
+        if (e.name != "InvalidStateError") {
+            console.log("FAILED: Was expecting an InvalidStateError exception");
+        }
+        testRunner.notifyDone();
+    }
 }
 window.addEventListener("DOMSubtreeModified", sendXHR);

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2014-10-14  Youenn Fablet  <youennf@gmail.com>
+
+        [XHR] Abort method execution when m_loader->cancel() in internalAbort() caused reentry
+        https://bugs.webkit.org/show_bug.cgi?id=126975
+
+        Reviewed by Alexey Proskuryakov.
+
+        Merging https://chromium.googlesource.com/chromium/blink/+/0d75daf2053631518606ae15daaece701a25b2c4
+        Ensuring new test from https://codereview.chromium.org/76133002/ is passing.
+
+        Test: http/tests/xmlhttprequest/reentrant-cancel-abort.html
+
+        * xml/XMLHttpRequest.cpp:
+        (WebCore::XMLHttpRequest::open): exit early if internalAbort asks so
+        (WebCore::XMLHttpRequest::abort): exit early if internalAbort asks so
+        (WebCore::XMLHttpRequest::internalAbort): ask calling function to exit early if a new loader is created during the cancellation of the loader (potential reentrant case through window.onload callback)   
+        (WebCore::XMLHttpRequest::didTimeout): exit early if internalAbort asks so
+        * xml/XMLHttpRequest.h:
+
 2014-10-14  Alejandro G. Castro  <alex@igalia.com>
 

trunk/Source/WebCore/xml/XMLHttpRequest.cpp

@@ -464 +464 @@
 void XMLHttpRequest::open(const String& method, const URL& url, bool async, ExceptionCode& ec)
 {
-    internalAbort();
+    if (!internalAbort())
+        return;
+
     State previousState = m_state;
     m_state = UNSENT;
@@ -806 +808 @@
     bool sendFlag = m_loader;
 
-    internalAbort();
+    if (!internalAbort())
+        return;
 
     clearResponseBuffers();
@@ -824 +827 @@
 }
 
-void XMLHttpRequest::internalAbort()
-{
-    bool hadLoader = m_loader;
-
+bool XMLHttpRequest::internalAbort()
+{
     m_error = true;
 
@@ -833 +834 @@
     m_receivedLength = 0;
 
-    if (hadLoader) {
-        m_loader->cancel();
-        m_loader = 0;
-    }
-
     m_decoder = 0;
 
     InspectorInstrumentation::didFailXHRLoading(scriptExecutionContext(), this);
 
-    if (hadLoader)
-        dropProtection();
+    if (!m_loader)
+        return true;
+
+    // Cancelling m_loader may trigger a window.onload callback which can call open() on the same xhr.
+    // This would create internalAbort reentrant call.
+    // m_loader is set to null before being cancelled to exit early in any reentrant internalAbort() call.
+    RefPtr<ThreadableLoader> loader = m_loader.release();
+    loader->cancel();
+
+    // If window.onload callback calls open() and send() on the same xhr, m_loader is now set to a new value.
+    // The function calling internalAbort() should abort to let the open() and send() calls continue properly.
+    // We ask the function calling internalAbort() to exit by returning false.
+    // Save this information to a local variable since we are going to drop protection.
+    bool newLoadStarted = m_loader;
+
+    dropProtection();
+
+    return !newLoadStarted;
 }
 
@@ -1219 +1231 @@
     // internalAbort() calls dropProtection(), which may release the last reference.
     Ref<XMLHttpRequest> protect(*this);
-    internalAbort();
+    if (!internalAbort())
+        return;
 
     clearResponse();

trunk/Source/WebCore/xml/XMLHttpRequest.h

@@ -195 +195 @@
     void callReadyStateChangeListener();
     void dropProtection();
-    void internalAbort();
+
+    // Returns false when cancelling the loader within internalAbort() triggers an event whose callback creates a new loader. 
+    // In that case, the function calling internalAbort should exit.
+    bool internalAbort();
+
     void clearResponse();
     void clearResponseBuffers();