XMLHttpRequestUpload's loadstart event not correct initialized
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 27 Mar 2019 18:10:51 +0000 (18:10 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 27 Mar 2019 18:10:51 +0000 (18:10 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196174
<rdar://problem/49191412>

Reviewed by Alex Christensen.

LayoutTests/imported/w3c:

* web-platform-tests/xhr/event-error-order.sub.html:
Update test after https://github.com/web-platform-tests/wpt/pull/13365

* web-platform-tests/xhr/abort-during-upload-expected.txt:
* web-platform-tests/xhr/event-error-order.sub-expected.txt:
* web-platform-tests/xhr/event-loadstart-upload-expected.txt:
* web-platform-tests/xhr/event-timeout-order-expected.txt:
* web-platform-tests/xhr/send-response-event-order-expected.txt:
Rebaseline several WPT tests that are now passing.

Source/WebCore:

Align progress event firing with the XHR specification.

No new tests, rebaselined existing tests.

* xml/XMLHttpRequest.cpp:
(WebCore::XMLHttpRequest::createRequest):
As per [1], the loadstart event fired on the XMLHttpRequestUpload object should use
loaded=0 and total=`req’s body’s total bytes`.
[1] https://xhr.spec.whatwg.org/#the-send()-method (step 11.2.)

(WebCore::XMLHttpRequest::didSendData):
As per [2], the progress / load / loadend should use loaded=transmitted and total=length.
[2] https://xhr.spec.whatwg.org/#ref-for-process-request-end-of-body (steps 5, 6 and 7)

(WebCore::XMLHttpRequest::didReceiveData):
As per [3], we should fire the readystatechange event *before* the progress event.
This is covered by web-platform-tests/xhr/send-response-event-order.htm which was failing
differently after the other changes in this patch.
[3] https://xhr.spec.whatwg.org/#ref-for-process-response (steps 9.4 and 9.5)

(WebCore::XMLHttpRequest::dispatchErrorEvents):
As per [4], in case of an error, we should fire the provided 'event' and 'loadend' with
loaded=0 and total=0.
[4] https://xhr.spec.whatwg.org/#request-error-steps (steps 7 and 8)

* xml/XMLHttpRequestUpload.cpp:
(WebCore::XMLHttpRequestUpload::dispatchProgressEvent):
* xml/XMLHttpRequestUpload.h:
Simplify XMLHttpRequestUpload. It no longer needs to store loaded / total as data
members now that they are always passed by the call site. lengthComputable is set
to !!total as [5] says to set it to true if length/total is not 0.
[5] https://xhr.spec.whatwg.org/#concept-event-fire-progress

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

14 files changed:
LayoutTests/http/tests/xmlhttprequest/upload-onabort-progressevent-attributes.html
LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/xhr/abort-during-upload-expected.txt
LayoutTests/imported/w3c/web-platform-tests/xhr/event-error-order.sub-expected.txt
LayoutTests/imported/w3c/web-platform-tests/xhr/event-error-order.sub.html
LayoutTests/imported/w3c/web-platform-tests/xhr/event-loadstart-upload-expected.txt
LayoutTests/imported/w3c/web-platform-tests/xhr/event-timeout-order-expected.txt
LayoutTests/imported/w3c/web-platform-tests/xhr/send-response-event-order-expected.txt
LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/xhr/event-error-order.sub-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/xml/XMLHttpRequest.cpp
Source/WebCore/xml/XMLHttpRequest.h
Source/WebCore/xml/XMLHttpRequestUpload.cpp
Source/WebCore/xml/XMLHttpRequestUpload.h

index 1f81170..fe9cdab 100644 (file)
@@ -26,6 +26,15 @@ function onProgressEvent(e)
             + "(" + e.loaded + " / " + e.total + "), expected (" + loaded + " / " + total + ")");
 }
 
+function onErrorProgressEvent(e)
+{
+    if (e.lengthComputable)
+        fail("Event " + e.type + " lengthComputable is true");
+    if (e.total != 0 || e.loaded != 0)
+        fail("Event " + e.type + " total/loaded values not matching: "
+            + "(" + e.loaded + " / " + e.total + "), expected (0 / 0)");
+}
+
 function onUnexpectedProgressEvent(e)
 {
     fail("unexpected ProgressEvent: " + e.type);
@@ -50,13 +59,13 @@ function test()
     var req = new XMLHttpRequest();
     req.upload.onerror = onUnexpectedProgressEvent;
     req.upload.onload = onUnexpectedProgressEvent;
-    req.upload.onabort = onProgressEvent;
+    req.upload.onabort = onErrorProgressEvent;
     req.upload.onprogress = function(e) {
         onProgressEvent(e);
         req.abort();
     }
     req.upload.onloadend = function(e) {
-        onProgressEvent(e);
+        onErrorProgressEvent(e);
         completeTest();
     }
 
index 591ca53..0ee899f 100644 (file)
@@ -1,3 +1,21 @@
+2019-03-27  Chris Dumez  <cdumez@apple.com>
+
+        XMLHttpRequestUpload's loadstart event not correct initialized
+        https://bugs.webkit.org/show_bug.cgi?id=196174
+        <rdar://problem/49191412>
+
+        Reviewed by Alex Christensen.
+
+        * web-platform-tests/xhr/event-error-order.sub.html:
+        Update test after https://github.com/web-platform-tests/wpt/pull/13365
+
+        * web-platform-tests/xhr/abort-during-upload-expected.txt:
+        * web-platform-tests/xhr/event-error-order.sub-expected.txt:
+        * web-platform-tests/xhr/event-loadstart-upload-expected.txt:
+        * web-platform-tests/xhr/event-timeout-order-expected.txt:
+        * web-platform-tests/xhr/send-response-event-order-expected.txt:
+        Rebaseline several WPT tests that are now passing.
+
 2019-03-25  Javier Fernandez  <jfernandez@igalia.com>
 
         A single leading space is not considered as a word break even when word-break: break-all is set
index c5ee05f..5b13c50 100644 (file)
@@ -1,3 +1,3 @@
 
-FAIL XMLHttpRequest: abort() while sending data assert_equals: expected "upload.loadstart(0,9999,true)" but got "upload.loadstart(0,0,false)"
+PASS XMLHttpRequest: abort() while sending data 
 
index 47de75f..026f11e 100644 (file)
@@ -1,4 +1,4 @@
 Blocked access to external URL http://nonexistent.localhost:8800/
 
-FAIL XMLHttpRequest: event - error (order of events) assert_equals: expected "upload.loadstart(0,12,true)" but got "upload.loadstart(0,0,false)"
+PASS XMLHttpRequest: event - error (order of events) 
 
index 252a90b..f03707e 100644 (file)
@@ -22,7 +22,7 @@
             xhr.addEventListener("loadend", function() {
                 test.step(function() {
                     // no progress events due to CORS failure
-                    assert_xhr_event_order_matches([1, "loadstart(0,0,false)", "upload.loadstart(0,12,true)", 2, 4, "upload.error(0,0,false)", "upload.loadend(0,0,false)", "error(0,0,false)", "loadend(0,0,false)"]);
+                    assert_xhr_event_order_matches([1, "loadstart(0,0,false)", "upload.loadstart(0,12,true)", 4, "upload.error(0,0,false)", "upload.loadend(0,0,false)", "error(0,0,false)", "loadend(0,0,false)"]);
                     test.done();
                 });
             });
index 8dac459..418c07f 100644 (file)
@@ -1,3 +1,3 @@
 
-FAIL XMLHttpRequest: The send() method: Fire a progress event named loadstart on upload object (synchronous flag is unset) assert_equals: upload.onloadstart: event.total expected 7 but got 0
+PASS XMLHttpRequest: The send() method: Fire a progress event named loadstart on upload object (synchronous flag is unset) 
 
index bc30c4e..0dc1527 100644 (file)
@@ -1,3 +1,3 @@
 
-FAIL XMLHttpRequest: event - timeout (order of events) assert_equals: expected "upload.loadstart(0,12,true)" but got "upload.loadstart(0,0,false)"
+PASS XMLHttpRequest: event - timeout (order of events) 
 
index aa24d30..2b1da9f 100644 (file)
@@ -1,3 +1,3 @@
 
-FAIL XMLHttpRequest: The send() method: event order when synchronous flag is unset assert_equals: expected "upload.loadstart(0,12,true)" but got "upload.loadstart(0,0,false)"
+PASS XMLHttpRequest: The send() method: event order when synchronous flag is unset 
 
index 85e046d..5d3f815 100644 (file)
@@ -1,3 +1,3 @@
 
-FAIL XMLHttpRequest: event - error (order of events) assert_equals: expected "upload.loadstart(0,12,true)" but got "upload.loadstart(0,0,false)"
+PASS XMLHttpRequest: event - error (order of events) 
 
index 1ffbf3f..bce22c8 100644 (file)
@@ -1,3 +1,44 @@
+2019-03-27  Chris Dumez  <cdumez@apple.com>
+
+        XMLHttpRequestUpload's loadstart event not correct initialized
+        https://bugs.webkit.org/show_bug.cgi?id=196174
+        <rdar://problem/49191412>
+
+        Reviewed by Alex Christensen.
+
+        Align progress event firing with the XHR specification.
+
+        No new tests, rebaselined existing tests.
+
+        * xml/XMLHttpRequest.cpp:
+        (WebCore::XMLHttpRequest::createRequest):
+        As per [1], the loadstart event fired on the XMLHttpRequestUpload object should use
+        loaded=0 and total=`req’s body’s total bytes`.
+        [1] https://xhr.spec.whatwg.org/#the-send()-method (step 11.2.)
+
+        (WebCore::XMLHttpRequest::didSendData):
+        As per [2], the progress / load / loadend should use loaded=transmitted and total=length.
+        [2] https://xhr.spec.whatwg.org/#ref-for-process-request-end-of-body (steps 5, 6 and 7)
+
+        (WebCore::XMLHttpRequest::didReceiveData):
+        As per [3], we should fire the readystatechange event *before* the progress event.
+        This is covered by web-platform-tests/xhr/send-response-event-order.htm which was failing
+        differently after the other changes in this patch.
+        [3] https://xhr.spec.whatwg.org/#ref-for-process-response (steps 9.4 and 9.5)
+
+        (WebCore::XMLHttpRequest::dispatchErrorEvents):
+        As per [4], in case of an error, we should fire the provided 'event' and 'loadend' with
+        loaded=0 and total=0.
+        [4] https://xhr.spec.whatwg.org/#request-error-steps (steps 7 and 8)
+
+        * xml/XMLHttpRequestUpload.cpp:
+        (WebCore::XMLHttpRequestUpload::dispatchProgressEvent):
+        * xml/XMLHttpRequestUpload.h:
+        Simplify XMLHttpRequestUpload. It no longer needs to store loaded / total as data
+        members now that they are always passed by the call site. lengthComputable is set
+        to !!total as [5] says to set it to true if length/total is not 0. 
+        [5] https://xhr.spec.whatwg.org/#concept-event-fire-progress
+
 2019-03-27  Simon Fraser  <simon.fraser@apple.com>
 
         REGRESSION (r242687): Fullscreen YouTube videos show blank white space at top
index 0af9244..d868ddf 100644 (file)
@@ -626,7 +626,7 @@ ExceptionOr<void> XMLHttpRequest::createRequest()
     if (m_async) {
         m_progressEventThrottle.dispatchProgressEvent(eventNames().loadstartEvent);
         if (!m_uploadComplete && m_uploadListenerFlag)
-            m_upload->dispatchProgressEvent(eventNames().loadstartEvent);
+            m_upload->dispatchProgressEvent(eventNames().loadstartEvent, 0, request.httpBody()->lengthInBytes());
 
         if (readyState() != OPENED || !m_sendFlag || m_loader)
             return { };
@@ -714,6 +714,7 @@ bool XMLHttpRequest::internalAbort()
 void XMLHttpRequest::clearResponse()
 {
     m_response = ResourceResponse();
+    m_didReceiveResponseHandler = nullptr;
     clearResponseBuffers();
 }
 
@@ -945,14 +946,22 @@ void XMLHttpRequest::didSendData(unsigned long long bytesSent, unsigned long lon
     if (!m_upload)
         return;
 
+    if (m_response.isNull()) {
+        // We should not notify of progress until we've received a response from the server.
+        m_didReceiveResponseHandler = [this, bytesSent, totalBytesToBeSent] {
+            didSendData(bytesSent, totalBytesToBeSent);
+        };
+        return;
+    }
+
     if (m_uploadListenerFlag)
-        m_upload->dispatchThrottledProgressEvent(true, bytesSent, totalBytesToBeSent);
+        m_upload->dispatchProgressEvent(eventNames().progressEvent, bytesSent, totalBytesToBeSent);
 
     if (bytesSent == totalBytesToBeSent && !m_uploadComplete) {
         m_uploadComplete = true;
         if (m_uploadListenerFlag) {
-            m_upload->dispatchProgressEvent(eventNames().loadEvent);
-            m_upload->dispatchProgressEvent(eventNames().loadendEvent);
+            m_upload->dispatchProgressEvent(eventNames().loadEvent, bytesSent, totalBytesToBeSent);
+            m_upload->dispatchProgressEvent(eventNames().loadendEvent, bytesSent, totalBytesToBeSent);
         }
     }
 }
@@ -960,6 +969,8 @@ void XMLHttpRequest::didSendData(unsigned long long bytesSent, unsigned long lon
 void XMLHttpRequest::didReceiveResponse(unsigned long, const ResourceResponse& response)
 {
     m_response = response;
+    if (auto didReceiveResponseHandler = WTFMove(m_didReceiveResponseHandler))
+        didReceiveResponseHandler();
 }
 
 static inline bool shouldDecodeResponse(XMLHttpRequest::ResponseType type)
@@ -1047,18 +1058,19 @@ void XMLHttpRequest::didReceiveData(const char* data, int len)
     if (!m_error) {
         m_receivedLength += len;
 
+        if (readyState() != LOADING)
+            changeState(LOADING);
+        else {
+            // Firefox calls readyStateChanged every time it receives data, 4449442
+            callReadyStateChangeListener();
+        }
+
         if (m_async) {
             long long expectedLength = m_response.expectedContentLength();
             bool lengthComputable = expectedLength > 0 && m_receivedLength <= expectedLength;
             unsigned long long total = lengthComputable ? expectedLength : 0;
             m_progressEventThrottle.dispatchThrottledProgressEvent(lengthComputable, m_receivedLength, total);
         }
-
-        if (readyState() != LOADING)
-            changeState(LOADING);
-        else
-            // Firefox calls readyStateChanged every time it receives data, 4449442
-            callReadyStateChangeListener();
     }
 }
 
@@ -1067,8 +1079,8 @@ void XMLHttpRequest::dispatchErrorEvents(const AtomicString& type)
     if (!m_uploadComplete) {
         m_uploadComplete = true;
         if (m_upload && m_uploadListenerFlag) {
-            m_upload->dispatchProgressEvent(type);
-            m_upload->dispatchProgressEvent(eventNames().loadendEvent);
+            m_upload->dispatchProgressEvent(type, 0, 0);
+            m_upload->dispatchProgressEvent(eventNames().loadendEvent, 0, 0);
         }
     }
     m_progressEventThrottle.dispatchProgressEvent(type);
index fa59735..29ff88d 100644 (file)
@@ -218,6 +218,7 @@ private:
     String m_responseEncoding;
 
     ResourceResponse m_response;
+    Function<void()> m_didReceiveResponseHandler;
 
     RefPtr<TextResourceDecoder> m_decoder;
 
index 8c06583..f8b9518 100644 (file)
@@ -38,26 +38,12 @@ XMLHttpRequestUpload::XMLHttpRequestUpload(XMLHttpRequest& request)
 {
 }
 
-void XMLHttpRequestUpload::dispatchThrottledProgressEvent(bool lengthComputable, unsigned long long loaded, unsigned long long total)
-{
-    m_lengthComputable = lengthComputable;
-    m_loaded = loaded;
-    m_total = total;
-
-    dispatchEvent(XMLHttpRequestProgressEvent::create(eventNames().progressEvent, lengthComputable, loaded, total));
-}
-
-void XMLHttpRequestUpload::dispatchProgressEvent(const AtomicString& type)
+void XMLHttpRequestUpload::dispatchProgressEvent(const AtomicString& type, unsigned long long loaded, unsigned long long total)
 {
     ASSERT(type == eventNames().loadstartEvent || type == eventNames().progressEvent || type == eventNames().loadEvent || type == eventNames().loadendEvent || type == eventNames().abortEvent || type == eventNames().errorEvent || type == eventNames().timeoutEvent);
 
-    if (type == eventNames().loadstartEvent) {
-        m_lengthComputable = false;
-        m_loaded = 0;
-        m_total = 0;
-    }
-
-    dispatchEvent(XMLHttpRequestProgressEvent::create(type, m_lengthComputable, m_loaded, m_total));
+    // https://xhr.spec.whatwg.org/#firing-events-using-the-progressevent-interface
+    dispatchEvent(XMLHttpRequestProgressEvent::create(type, !!total, loaded, total));
 }
 
 } // namespace WebCore
index e9ebb58..0516b07 100644 (file)
@@ -38,8 +38,7 @@ public:
     void ref() { m_request.ref(); }
     void deref() { m_request.deref(); }
 
-    void dispatchThrottledProgressEvent(bool lengthComputable, unsigned long long loaded, unsigned long long total);
-    void dispatchProgressEvent(const AtomicString& type);
+    void dispatchProgressEvent(const AtomicString& type, unsigned long long loaded, unsigned long long total);
 
 private:
     void refEventTarget() final { ref(); }
@@ -49,9 +48,6 @@ private:
     ScriptExecutionContext* scriptExecutionContext() const final { return m_request.scriptExecutionContext(); }
 
     XMLHttpRequest& m_request;
-    bool m_lengthComputable { false };
-    unsigned long long m_loaded { 0 };
-    unsigned long long m_total { 0 };
 };
     
 } // namespace WebCore