Title: [192361] trunk
Revision
192361
Author
youenn.fab...@crf.canon.fr
Date
2015-11-12 05:34:01 -0800 (Thu, 12 Nov 2015)

Log Message

XHR abort() event firing does not match spec
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

Modified Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (192360 => 192361)


--- trunk/LayoutTests/ChangeLog	2015-11-12 13:19:30 UTC (rev 192360)
+++ trunk/LayoutTests/ChangeLog	2015-11-12 13:34:01 UTC (rev 192361)
@@ -1,3 +1,18 @@
+2015-11-12  Youenn Fablet  <youenn.fab...@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_mil...@apple.com>
 
         Regression(r191815): 5.3% regression on Dromaeo JS Library Benchmark

Modified: trunk/LayoutTests/fast/events/event-fire-order-expected.txt (192360 => 192361)


--- trunk/LayoutTests/fast/events/event-fire-order-expected.txt	2015-11-12 13:19:30 UTC (rev 192360)
+++ trunk/LayoutTests/fast/events/event-fire-order-expected.txt	2015-11-12 13:34:01 UTC (rev 192361)
@@ -2,7 +2,7 @@
 
 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.
 

Modified: trunk/LayoutTests/fast/events/event-fire-order.html (192360 => 192361)


--- trunk/LayoutTests/fast/events/event-fire-order.html	2015-11-12 13:19:30 UTC (rev 192360)
+++ trunk/LayoutTests/fast/events/event-fire-order.html	2015-11-12 13:34:01 UTC (rev 192361)
@@ -49,12 +49,12 @@
     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 @@
         x.open("POST", "resources/does-not-exist");
         x.abort();
 
-        reportResult(arguments.callee.name, [ "f1", "f2" ]);
+        reportResult(arguments.callee.name, []);
     }
 ];
 

Modified: trunk/LayoutTests/http/tests/xmlhttprequest/onloadend-event-after-sync-requests.html (192360 => 192361)


--- trunk/LayoutTests/http/tests/xmlhttprequest/onloadend-event-after-sync-requests.html	2015-11-12 13:19:30 UTC (rev 192360)
+++ trunk/LayoutTests/http/tests/xmlhttprequest/onloadend-event-after-sync-requests.html	2015-11-12 13:34:01 UTC (rev 192361)
@@ -81,11 +81,12 @@
     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 @@
         if (e.code != e.NETWORK_ERR)
             results += " " + e;
     }
-    
-    completeTest(" abort loadend");
+    completeTest(" load loadend");
 }
 
 testNormal();

Modified: trunk/LayoutTests/http/tests/xmlhttprequest/upload-onloadend-event-after-sync-requests.html (192360 => 192361)


--- trunk/LayoutTests/http/tests/xmlhttprequest/upload-onloadend-event-after-sync-requests.html	2015-11-12 13:19:30 UTC (rev 192360)
+++ trunk/LayoutTests/http/tests/xmlhttprequest/upload-onloadend-event-after-sync-requests.html	2015-11-12 13:34:01 UTC (rev 192361)
@@ -59,11 +59,12 @@
     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 @@
             results += " " + e;
     }
     
-    completeTest(" abort loadend");
+    completeTest(" load loadend");
 }
 
 testNormal();

Modified: trunk/LayoutTests/imported/w3c/ChangeLog (192360 => 192361)


--- trunk/LayoutTests/imported/w3c/ChangeLog	2015-11-12 13:19:30 UTC (rev 192360)
+++ trunk/LayoutTests/imported/w3c/ChangeLog	2015-11-12 13:34:01 UTC (rev 192361)
@@ -1,3 +1,18 @@
+2015-11-12  Youenn Fablet  <youenn.fab...@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.fab...@crf.canon.fr>
 
         XMLHttpRequestUpload should support ontimeout event handler

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/abort-after-receive-expected.txt (192360 => 192361)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/abort-after-receive-expected.txt	2015-11-12 13:19:30 UTC (rev 192360)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/abort-after-receive-expected.txt	2015-11-12 13:34:01 UTC (rev 192361)
@@ -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 
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/abort-after-timeout-expected.txt (192360 => 192361)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/abort-after-timeout-expected.txt	2015-11-12 13:19:30 UTC (rev 192360)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/abort-after-timeout-expected.txt	2015-11-12 13:34:01 UTC (rev 192361)
@@ -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 
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/abort-during-upload-expected.txt (192360 => 192361)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/abort-during-upload-expected.txt	2015-11-12 13:19:30 UTC (rev 192360)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/abort-during-upload-expected.txt	2015-11-12 13:34:01 UTC (rev 192361)
@@ -1,3 +1,3 @@
 
-FAIL XMLHttpRequest: abort() while sending data assert_equals: expected 4 but got 0
+PASS XMLHttpRequest: abort() while sending data 
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/abort-event-abort-expected.txt (192360 => 192361)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/abort-event-abort-expected.txt	2015-11-12 13:19:30 UTC (rev 192360)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/abort-event-abort-expected.txt	2015-11-12 13:34:01 UTC (rev 192361)
@@ -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(). 
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/abort-event-order-expected.txt (192360 => 192361)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/abort-event-order-expected.txt	2015-11-12 13:19:30 UTC (rev 192360)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/abort-event-order-expected.txt	2015-11-12 13:34:01 UTC (rev 192361)
@@ -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 
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-aborted-expected.txt (192360 => 192361)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-aborted-expected.txt	2015-11-12 13:19:30 UTC (rev 192360)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-aborted-expected.txt	2015-11-12 13:34:01 UTC (rev 192361)
@@ -6,5 +6,4 @@
 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 
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-aborted-expected.txt (192360 => 192361)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-aborted-expected.txt	2015-11-12 13:19:30 UTC (rev 192360)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-aborted-expected.txt	2015-11-12 13:34:01 UTC (rev 192361)
@@ -6,5 +6,4 @@
 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 
 

Modified: trunk/LayoutTests/platform/gtk/TestExpectations (192360 => 192361)


--- trunk/LayoutTests/platform/gtk/TestExpectations	2015-11-12 13:19:30 UTC (rev 192360)
+++ trunk/LayoutTests/platform/gtk/TestExpectations	2015-11-12 13:34:01 UTC (rev 192361)
@@ -2518,7 +2518,6 @@
 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 ]

Modified: trunk/Source/WebCore/ChangeLog (192360 => 192361)


--- trunk/Source/WebCore/ChangeLog	2015-11-12 13:19:30 UTC (rev 192360)
+++ trunk/Source/WebCore/ChangeLog	2015-11-12 13:34:01 UTC (rev 192361)
@@ -1,3 +1,32 @@
+2015-11-12  Youenn Fablet  <youenn.fab...@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  <cgar...@igalia.com>
 
         [GStreamer] Use RunLoop::timer in MediaPlayerPrivateGStreamerBase for GL drawing

Modified: trunk/Source/WebCore/xml/XMLHttpRequest.cpp (192360 => 192361)


--- trunk/Source/WebCore/xml/XMLHttpRequest.cpp	2015-11-12 13:19:30 UTC (rev 192360)
+++ trunk/Source/WebCore/xml/XMLHttpRequest.cpp	2015-11-12 13:34:01 UTC (rev 192361)
@@ -373,10 +373,13 @@
     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::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 @@
     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 @@
     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 @@
         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 @@
         // 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 @@
     // internalAbort() calls dropProtection(), which may release the last reference.
     Ref<XMLHttpRequest> protect(*this);
 
-    bool sendFlag = m_loader;
-
     if (!internalAbort())
         return;
 
@@ -810,16 +816,13 @@
 
     // 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 @@
 {
     clearResponse();
     clearRequest();
+    m_sendFlag = false;
     m_error = true;
 
     changeState(DONE);
@@ -934,7 +938,7 @@
 
 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 @@
     bool hadLoader = m_loader;
     m_loader = nullptr;
 
+    m_sendFlag = false;
     changeState(DONE);
     m_responseEncoding = String();
     m_decoder = nullptr;
@@ -1241,6 +1246,7 @@
     clearResponse();
     clearRequest();
 
+    m_sendFlag = false;
     m_error = true;
     m_exceptionCode = XMLHttpRequestException::TIMEOUT_ERR;
 

Modified: trunk/Source/WebCore/xml/XMLHttpRequest.h (192360 => 192361)


--- trunk/Source/WebCore/xml/XMLHttpRequest.h	2015-11-12 13:19:30 UTC (rev 192360)
+++ trunk/Source/WebCore/xml/XMLHttpRequest.h	2015-11-12 13:34:01 UTC (rev 192361)
@@ -216,6 +216,7 @@
 
     RefPtr<ThreadableLoader> m_loader;
     State m_state;
+    bool m_sendFlag { false };
 
     ResourceResponse m_response;
     String m_responseEncoding;
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to