XHR abort() event firing does not match spec
authoryouenn.fablet@crf.canon.fr <youenn.fablet@crf.canon.fr@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 12 Nov 2015 13:34:01 +0000 (13:34 +0000)
committeryouenn.fablet@crf.canon.fr <youenn.fablet@crf.canon.fr@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 12 Nov 2015 13:34:01 +0000 (13:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=98404

Reviewed by Darin Adler.

LayoutTests/imported/w3c:

* web-platform-tests/XMLHttpRequest/abort-after-receive-expected.txt:
* web-platform-tests/XMLHttpRequest/abort-after-timeout-expected.txt:
* web-platform-tests/XMLHttpRequest/abort-during-upload-expected.txt:
* web-platform-tests/XMLHttpRequest/abort-event-abort-expected.txt:
* web-platform-tests/XMLHttpRequest/abort-event-order-expected.txt:
* web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-aborted-expected.txt:
* web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-aborted-expected.txt:

Source/WebCore:

Introducing explicit sendFlag as in the specification.
Previously, sendFlag was computed using !!m_loader.
But this does not work well for loadstart events since sendFlag should be true but m_loader is still null at that time.

Updated abort event firing test according specification.
For instance, compared to previously, the abort event is not fired in DONE state or if sendFlag is not set.

Covered by rebased tests.

* xml/XMLHttpRequest.cpp:
(WebCore::XMLHttpRequest::callReadyStateChangeListener): Compute whether dispatching the load event before calling XHR readystatechange event callback.
(WebCore::XMLHttpRequest::setWithCredentials): Replacing m_loader by m_sendFlag to align with the spec.
(WebCore::XMLHttpRequest::open): Unsetting m_sendFlag..
(WebCore::XMLHttpRequest::initSend): Replacing m_loader by m_sendFlag to align with the spec.
(WebCore::XMLHttpRequest::createRequest): Setting m_sendFlag.
(WebCore::XMLHttpRequest::abort): Checking m_sendFlag and updating code to follow the specification.
(WebCore::XMLHttpRequest::genericError): Unsetting m_sendFlag.
(WebCore::XMLHttpRequest::setRequestHeader): Replacing m_loader by m_sendFlag to align with the spec.
(WebCore::XMLHttpRequest::didFinishLoading): Ditto.
(WebCore::XMLHttpRequest::didReachTimeout): Ditto.
* xml/XMLHttpRequest.h: Added m_sendFlag.

LayoutTests:

Rebasing test expectations.

* fast/events/event-fire-order-expected.txt:
* fast/events/event-fire-order.html: Updated so that it does not expect any event when aborting just after open().
* http/tests/xmlhttprequest/onloadend-event-after-sync-requests.html: Updated to expect load event and not abort event when XHR state is DONE.
* http/tests/xmlhttprequest/upload-onloadend-event-after-sync-requests.html: Ditto.
* platform/gtk/TestExpectations: Removing timeout expectation for imported/w3c/web-platform-tests/XMLHttpRequest/interfaces.html

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

17 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/events/event-fire-order-expected.txt
LayoutTests/fast/events/event-fire-order.html
LayoutTests/http/tests/xmlhttprequest/onloadend-event-after-sync-requests.html
LayoutTests/http/tests/xmlhttprequest/upload-onloadend-event-after-sync-requests.html
LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/abort-after-receive-expected.txt
LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/abort-after-timeout-expected.txt
LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/abort-during-upload-expected.txt
LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/abort-event-abort-expected.txt
LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/abort-event-order-expected.txt
LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-aborted-expected.txt
LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-aborted-expected.txt
LayoutTests/platform/gtk/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/xml/XMLHttpRequest.cpp
Source/WebCore/xml/XMLHttpRequest.h

index 36ddab8..c70c522 100644 (file)
@@ -1,3 +1,18 @@
+2015-11-12  Youenn Fablet  <youenn.fablet@crf.canon.fr>
+
+        XHR abort() event firing does not match spec
+        https://bugs.webkit.org/show_bug.cgi?id=98404
+
+        Reviewed by Darin Adler.
+
+        Rebasing test expectations.
+
+        * fast/events/event-fire-order-expected.txt:
+        * fast/events/event-fire-order.html: Updated so that it does not expect any event when aborting just after open().
+        * http/tests/xmlhttprequest/onloadend-event-after-sync-requests.html: Updated to expect load event and not abort event when XHR state is DONE.
+        * http/tests/xmlhttprequest/upload-onloadend-event-after-sync-requests.html: Ditto.
+        * platform/gtk/TestExpectations: Removing timeout expectation for imported/w3c/web-platform-tests/XMLHttpRequest/interfaces.html
+
 2015-11-10  Keith Miller  <keith_miller@apple.com>
 
         Regression(r191815): 5.3% regression on Dromaeo JS Library Benchmark
index e25895d..7d64bf9 100644 (file)
@@ -2,7 +2,7 @@ This page tests event listener fire order for a few objects that had it wrong in
 
 If the test passes, you'll see a series of PASS messages below.
 
-PASS: result should be f1,f2 and is.
-PASS: result should be f1,f2 and is.
-PASS: result should be f1,f2 and is.
+PASS: result should be 'f1,f2' and is.
+PASS: result should be 'f1,f2' and is.
+PASS: result should be '' and is.
 
index fdf481c..1e95754 100644 (file)
@@ -49,12 +49,12 @@ function reportResult(name, expected)
     var end = result.length > expected.length ? result.length : expected.length;
     for (var i = 0; i < end; ++i) {
         if (result[i] != expected[i]) {
-            log("FAIL: " + name + " result[" + i + "] should be " + expected[i] + " but instead is " + result[i] + ".");
+            log("FAIL: " + name + " result[" + i + "] should be '" + expected[i] + "' but instead is '" + result[i] + "'.");
             passed = false;
         }
     }
     if (passed)
-        log("PASS: result should be " + expected + " and is.");
+        log("PASS: result should be '" + expected + "' and is.");
 }
 
 var tests = [
@@ -97,7 +97,7 @@ var tests = [
         x.open("POST", "resources/does-not-exist");
         x.abort();
 
-        reportResult(arguments.callee.name, [ "f1", "f2" ]);
+        reportResult(arguments.callee.name, []);
     }
 ];
 
index a9a1443..f73f606 100644 (file)
@@ -81,11 +81,12 @@ function testAbort()
     results = "";
 
     xhr.onloadstart = logUnexpectedProgressEvent;
-    xhr.onabort = logProgressEvent;
+    xhr.onabort = logUnexpectedProgressEvent;
     xhr.onerror = logUnexpectedProgressEvent;
-    xhr.onload = logUnexpectedProgressEvent;
+    xhr.onload = logProgressEvent;
     xhr.onloadend = logProgressEvent;
     xhr.onreadystatechange = function(e) {
+        // abort in DONE state should do nothing.
         if (xhr.readyState == xhr.DONE)
             xhr.abort();
     }
@@ -98,8 +99,7 @@ function testAbort()
         if (e.code != e.NETWORK_ERR)
             results += " " + e;
     }
-    
-    completeTest(" abort loadend");
+    completeTest(" load loadend");
 }
 
 testNormal();
index caf552c..a12cda1 100644 (file)
@@ -59,11 +59,12 @@ function testAbort()
     results = "";
 
     xhr.onloadstart = logUnexpectedProgressEvent;
-    xhr.onabort = logProgressEvent;
+    xhr.onabort = logUnexpectedProgressEvent;
     xhr.onerror = logUnexpectedProgressEvent;
-    xhr.onload = logUnexpectedProgressEvent;
+    xhr.onload = logProgressEvent;
     xhr.onloadend = logProgressEvent;
     xhr.onreadystatechange = function(e) {
+        // abort in DONE state should do nothing.
         if (xhr.readyState == xhr.DONE)
             xhr.abort();
     }
@@ -77,7 +78,7 @@ function testAbort()
             results += " " + e;
     }
     
-    completeTest(" abort loadend");
+    completeTest(" load loadend");
 }
 
 testNormal();
index c0fd0a8..80e2c9b 100644 (file)
@@ -1,3 +1,18 @@
+2015-11-12  Youenn Fablet  <youenn.fablet@crf.canon.fr>
+
+        XHR abort() event firing does not match spec
+        https://bugs.webkit.org/show_bug.cgi?id=98404
+
+        Reviewed by Darin Adler.
+
+        * web-platform-tests/XMLHttpRequest/abort-after-receive-expected.txt:
+        * web-platform-tests/XMLHttpRequest/abort-after-timeout-expected.txt:
+        * web-platform-tests/XMLHttpRequest/abort-during-upload-expected.txt:
+        * web-platform-tests/XMLHttpRequest/abort-event-abort-expected.txt:
+        * web-platform-tests/XMLHttpRequest/abort-event-order-expected.txt:
+        * web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-aborted-expected.txt:
+        * web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-aborted-expected.txt:
+
 2015-11-10  Youenn Fablet  <youenn.fablet@crf.canon.fr>
 
         XMLHttpRequestUpload should support ontimeout event handler
index 152c04d..606c860 100644 (file)
@@ -1,3 +1,3 @@
 
-FAIL XMLHttpRequest: abort() after successful receive should not fire "abort" event assert_unreached: abort() should not cause the abort event to fire Reached unreachable code
+PASS XMLHttpRequest: abort() after successful receive should not fire "abort" event 
 
index 0a4b080..04106e6 100644 (file)
@@ -1,3 +1,3 @@
 
-FAIL XMLHttpRequest: abort() after a timeout should not fire "abort" event assert_unreached: abort() should not cause the abort event to fire Reached unreachable code
+PASS XMLHttpRequest: abort() after a timeout should not fire "abort" event 
 
index 1279415..9ae6c02 100644 (file)
@@ -1,3 +1,3 @@
 
-FAIL XMLHttpRequest: The abort() method: do not fire abort event in OPENED state when send() flag is unset. send() throws after abort(). assert_unreached: when abort() is called, state is OPENED with the send() flag being unset, must not fire abort event per spec Reached unreachable code
+PASS XMLHttpRequest: The abort() method: do not fire abort event in OPENED state when send() flag is unset. send() throws after abort(). 
 
index cfb5935..a01c33f 100644 (file)
@@ -1,3 +1,3 @@
 
-FAIL XMLHttpRequest: The abort() method: abort and loadend events assert_array_equals: lengths differ, expected 6 got 5
+PASS XMLHttpRequest: The abort() method: abort and loadend events 
 
index 2655211..656cefc 100644 (file)
@@ -6,5 +6,4 @@ This test validates that the XHR2 timeout property behaves as expected in async
 PASS Timeout test: No events should fire for an unsent, unaborted request 
 PASS Timeout test: time to abort is -1, timeout set at 2000 
 PASS Timeout test: time to abort is 5000, timeout set at 2000 
-PASS Timeout test: time to abort is 5000, timeout set at 2000 
 
index cf46c71..d6f548f 100644 (file)
@@ -6,5 +6,4 @@ This test validates that the XHR2 timeout property behaves as expected in in a w
 PASS Timeout test: No events should fire for an unsent, unaborted request 
 PASS Timeout test: time to abort is -1, timeout set at 2000 
 PASS Timeout test: time to abort is 5000, timeout set at 2000 
-PASS Timeout test: time to abort is 5000, timeout set at 2000 
 
index 65de4c4..de4e025 100644 (file)
@@ -2518,7 +2518,6 @@ imported/w3c/web-platform-tests/XMLHttpRequest/status-async.htm [ Failure ]
 imported/w3c/web-platform-tests/XMLHttpRequest/status-basic.htm [ Failure ]
 imported/w3c/web-platform-tests/XMLHttpRequest/status-error.htm [ Failure ]
 imported/w3c/web-platform-tests/XMLHttpRequest/event-readystatechange-loaded.htm [ Timeout ]
-imported/w3c/web-platform-tests/XMLHttpRequest/interfaces.html [ Timeout ]
 
 webkit.org/b/148938 accessibility/aria-table-hierarchy.html [ Failure ]
 webkit.org/b/148938 accessibility/aria-table-with-presentational-elements.html [ Failure ]
index 3f922ba..5096571 100644 (file)
@@ -1,3 +1,32 @@
+2015-11-12  Youenn Fablet  <youenn.fablet@crf.canon.fr>
+
+        XHR abort() event firing does not match spec
+        https://bugs.webkit.org/show_bug.cgi?id=98404
+
+        Reviewed by Darin Adler.
+
+        Introducing explicit sendFlag as in the specification.
+        Previously, sendFlag was computed using !!m_loader.
+        But this does not work well for loadstart events since sendFlag should be true but m_loader is still null at that time.
+
+        Updated abort event firing test according specification.
+        For instance, compared to previously, the abort event is not fired in DONE state or if sendFlag is not set.
+
+        Covered by rebased tests.
+
+        * xml/XMLHttpRequest.cpp:
+        (WebCore::XMLHttpRequest::callReadyStateChangeListener): Compute whether dispatching the load event before calling XHR readystatechange event callback.
+        (WebCore::XMLHttpRequest::setWithCredentials): Replacing m_loader by m_sendFlag to align with the spec.
+        (WebCore::XMLHttpRequest::open): Unsetting m_sendFlag..
+        (WebCore::XMLHttpRequest::initSend): Replacing m_loader by m_sendFlag to align with the spec.
+        (WebCore::XMLHttpRequest::createRequest): Setting m_sendFlag.
+        (WebCore::XMLHttpRequest::abort): Checking m_sendFlag and updating code to follow the specification.
+        (WebCore::XMLHttpRequest::genericError): Unsetting m_sendFlag.
+        (WebCore::XMLHttpRequest::setRequestHeader): Replacing m_loader by m_sendFlag to align with the spec.
+        (WebCore::XMLHttpRequest::didFinishLoading): Ditto.
+        (WebCore::XMLHttpRequest::didReachTimeout): Ditto.
+        * xml/XMLHttpRequest.h: Added m_sendFlag.
+
 2015-11-12  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [GStreamer] Use RunLoop::timer in MediaPlayerPrivateGStreamerBase for GL drawing
index 6beb6ad..83b3f2a 100644 (file)
@@ -373,10 +373,13 @@ void XMLHttpRequest::callReadyStateChangeListener()
     if (!scriptExecutionContext())
         return;
 
+    // Check whether sending load and loadend events before sending readystatechange event, as it may change m_error/m_state values.
+    bool shouldSendLoadEvent = (m_state == DONE && !m_error);
+
     if (m_async || (m_state <= OPENED || m_state == DONE))
         m_progressEventThrottle.dispatchReadyStateChangeEvent(Event::create(eventNames().readystatechangeEvent, false, false), m_state == DONE ? FlushProgressEvent : DoNotFlushProgressEvent);
 
-    if (m_state == DONE && !m_error) {
+    if (shouldSendLoadEvent) {
         m_progressEventThrottle.dispatchProgressEvent(eventNames().loadEvent);
         m_progressEventThrottle.dispatchProgressEvent(eventNames().loadendEvent);
     }
@@ -384,7 +387,7 @@ void XMLHttpRequest::callReadyStateChangeListener()
 
 void XMLHttpRequest::setWithCredentials(bool value, ExceptionCode& ec)
 {
-    if (m_state > OPENED || m_loader) {
+    if (m_state > OPENED || m_sendFlag) {
         ec = INVALID_STATE_ERR;
         return;
     }
@@ -476,6 +479,7 @@ void XMLHttpRequest::open(const String& method, const URL& url, bool async, Exce
     State previousState = m_state;
     m_state = UNSENT;
     m_error = false;
+    m_sendFlag = false;
     m_uploadComplete = false;
 
     // clear stuff from possible previous load
@@ -565,10 +569,11 @@ bool XMLHttpRequest::initSend(ExceptionCode& ec)
     if (!scriptExecutionContext())
         return false;
 
-    if (m_state != OPENED || m_loader) {
+    if (m_state != OPENED || m_sendFlag) {
         ec = INVALID_STATE_ERR;
         return false;
     }
+    ASSERT(!m_loader);
 
     m_error = false;
     return true;
@@ -716,6 +721,8 @@ void XMLHttpRequest::createRequest(ExceptionCode& ec)
         return;
     }
 
+    m_sendFlag = true;
+
     // The presence of upload event listeners forces us to use preflighting because POSTing to an URL that does not
     // permit cross origin requests should look exactly like POSTing to an URL that does not respond at all.
     // Also, only async requests support upload progress events.
@@ -782,6 +789,7 @@ void XMLHttpRequest::createRequest(ExceptionCode& ec)
         // and they are referenced by the JavaScript wrapper.
         setPendingActivity(this);
         if (!m_loader) {
+            m_sendFlag = false;
             m_timeoutTimer.stop();
             m_networkErrorTimer.startOneShot(0);
         }
@@ -801,8 +809,6 @@ void XMLHttpRequest::abort()
     // internalAbort() calls dropProtection(), which may release the last reference.
     Ref<XMLHttpRequest> protect(*this);
 
-    bool sendFlag = m_loader;
-
     if (!internalAbort())
         return;
 
@@ -810,16 +816,13 @@ void XMLHttpRequest::abort()
 
     // Clear headers as required by the spec
     m_requestHeaders.clear();
-
-    if ((m_state <= OPENED && !sendFlag) || m_state == DONE)
-        m_state = UNSENT;
-    else {
+    if ((m_state == OPENED && m_sendFlag) || m_state == HEADERS_RECEIVED || m_state == LOADING) {
         ASSERT(!m_loader);
+        m_sendFlag = false;
         changeState(DONE);
-        m_state = UNSENT;
+        dispatchErrorEvents(eventNames().abortEvent);
     }
-
-    dispatchErrorEvents(eventNames().abortEvent);
+    m_state = UNSENT;
 }
 
 bool XMLHttpRequest::internalAbort()
@@ -881,6 +884,7 @@ void XMLHttpRequest::genericError()
 {
     clearResponse();
     clearRequest();
+    m_sendFlag = false;
     m_error = true;
 
     changeState(DONE);
@@ -934,7 +938,7 @@ void XMLHttpRequest::overrideMimeType(const String& override, ExceptionCode& ec)
 
 void XMLHttpRequest::setRequestHeader(const String& name, const String& value, ExceptionCode& ec)
 {
-    if (m_state != OPENED || m_loader) {
+    if (m_state != OPENED || m_sendFlag) {
 #if ENABLE(DASHBOARD_SUPPORT)
         if (usesDashboardBackwardCompatibilityMode())
             return;
@@ -1118,6 +1122,7 @@ void XMLHttpRequest::didFinishLoading(unsigned long identifier, double)
     bool hadLoader = m_loader;
     m_loader = nullptr;
 
+    m_sendFlag = false;
     changeState(DONE);
     m_responseEncoding = String();
     m_decoder = nullptr;
@@ -1241,6 +1246,7 @@ void XMLHttpRequest::didReachTimeout()
     clearResponse();
     clearRequest();
 
+    m_sendFlag = false;
     m_error = true;
     m_exceptionCode = XMLHttpRequestException::TIMEOUT_ERR;
 
index 5a310b8..8855f44 100644 (file)
@@ -216,6 +216,7 @@ private:
 
     RefPtr<ThreadableLoader> m_loader;
     State m_state;
+    bool m_sendFlag { false };
 
     ResourceResponse m_response;
     String m_responseEncoding;