[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:
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@174684 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 4c715f1..1a3f210 100644
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,19 @@
+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>
Add test to check stretchy value is case sensitive
diff --git a/LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-abort-expected.txt b/LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-abort-expected.txt
new file mode 100644
index 0000000..1dc7ec5
--- /dev/null
+++ b/LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-abort-expected.txt
@@ -0,0 +1,5 @@
+insertedText
+Reentrancy, cancellation and explicit abort. Check that we don't crash and report the expected abort event.
+DOMContentLoaded
+PASS
+
diff --git a/LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-abort.html b/LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-abort.html
new file mode 100644
index 0000000..ae8cd55
--- /dev/null
+++ b/LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-abort.html
@@ -0,0 +1,52 @@
+<!doctype html>
+<html>
+<body>
+<script>
+if (window.testRunner) {
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+}
+
+function log(str)
+{
+ document.body.appendChild(document.createTextNode(str));
+ document.body.appendChild(document.createElement("br"));
+}
+
+function addElement(e)
+{
+ var txt = (e && e.type) || "insertedText";
+ log(txt);
+}
+document.addEventListener("DOMContentLoaded", addElement, false);
+
+var abortDispatched = false;
+function reportResult()
+{
+ log(abortDispatched ? "PASS" : "FAIL");
+ testRunner.notifyDone();
+}
+
+window.onload = function () {
+ xhr.open("GET", "", true);
+ setTimeout(reportResult, 100);
+};
+
+var xhr = new XMLHttpRequest();
+xhr.onabort = function () {
+ abortDispatched = true;
+};
+
+function sendAndAbort()
+{
+ xhr.open("GET", "", true);
+ xhr.send();
+ xhr.abort();
+}
+window.addEventListener("DOMSubtreeModified", sendAndAbort);
+addElement();
+</script>
+Reentrancy, cancellation and explicit abort. Check that we don't crash
+and report the expected abort event.<br/>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/xmlhttprequest/reentrant-cancel.html b/LayoutTests/http/tests/xmlhttprequest/reentrant-cancel.html
index ab9e7aa..73f55c1 100644
--- a/LayoutTests/http/tests/xmlhttprequest/reentrant-cancel.html
+++ b/LayoutTests/http/tests/xmlhttprequest/reentrant-cancel.html
@@ -1,6 +1,8 @@
<script>
-if (window.testRunner)
+if (window.testRunner) {
testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+}
function addElement() {
document.documentElement.appendChild(document.createTextNode('X'));
@@ -12,7 +14,17 @@
function sendXHR()
{
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);
addElement();
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 4e838b0..703acf1 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,22 @@
+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>
Multiple refactors in RenderMathMLOperator
diff --git a/Source/WebCore/xml/XMLHttpRequest.cpp b/Source/WebCore/xml/XMLHttpRequest.cpp
index 83031b8..558fde1 100644
--- a/Source/WebCore/xml/XMLHttpRequest.cpp
+++ b/Source/WebCore/xml/XMLHttpRequest.cpp
@@ -463,7 +463,9 @@
void XMLHttpRequest::open(const String& method, const URL& url, bool async, ExceptionCode& ec)
{
- internalAbort();
+ if (!internalAbort())
+ return;
+
State previousState = m_state;
m_state = UNSENT;
m_error = false;
@@ -805,7 +807,8 @@
bool sendFlag = m_loader;
- internalAbort();
+ if (!internalAbort())
+ return;
clearResponseBuffers();
@@ -823,26 +826,35 @@
dispatchErrorEvents(eventNames().abortEvent);
}
-void XMLHttpRequest::internalAbort()
+bool XMLHttpRequest::internalAbort()
{
- bool hadLoader = m_loader;
-
m_error = true;
// FIXME: when we add the support for multi-part XHR, we will have to think be careful with this initialization.
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;
}
void XMLHttpRequest::clearResponse()
@@ -1218,7 +1230,8 @@
{
// internalAbort() calls dropProtection(), which may release the last reference.
Ref<XMLHttpRequest> protect(*this);
- internalAbort();
+ if (!internalAbort())
+ return;
clearResponse();
clearRequest();
diff --git a/Source/WebCore/xml/XMLHttpRequest.h b/Source/WebCore/xml/XMLHttpRequest.h
index 951bd40..c5e13de 100644
--- a/Source/WebCore/xml/XMLHttpRequest.h
+++ b/Source/WebCore/xml/XMLHttpRequest.h
@@ -194,7 +194,11 @@
void changeState(State newState);
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();
void clearRequest();
