Title: [284472] trunk
Revision
284472
Author
you...@apple.com
Date
2021-10-19 11:41:13 -0700 (Tue, 19 Oct 2021)

Log Message

Guarantee order of WebSocket events in case of being resumed
https://bugs.webkit.org/show_bug.cgi?id=231664

Reviewed by Chris Dumez.

LayoutTests/imported/w3c:

* web-platform-tests/websockets/interfaces/WebSocket/close/close-nested-expected.txt:
* web-platform-tests/websockets/interfaces/WebSocket/readyState/003-expected.txt:

Source/WebCore:

Introduce a WebSocket task source as per spec.
This aligns with https://html.spec.whatwg.org/multipage/web-sockets.html where the user agent is expected to a queue task
when closing handshake is started and so on.

By queuing an event loop task to fire WebSocket events, we ensure ordering of events even in case of resuming, while simplifying the implementation.
This makes it use the event loop which is extra nice if we resolve promises in event listeners.

A follow-up patch should refactor code so that WebSocket::didReceiveMessageError can directly fire the close event without having to wait for a didClose call.

Covered by existing tests.

* Modules/websockets/WebSocket.cpp:
(WebCore::WebSocket::WebSocket):
(WebCore::WebSocket::suspend):
(WebCore::WebSocket::resume):
(WebCore::WebSocket::stop):
(WebCore::WebSocket::dispatchOrQueueEvent):
(WebCore::WebSocket::resumeTimerFired): Deleted.
* Modules/websockets/WebSocket.h:
* Modules/websockets/WebSocketChannel.cpp:
(WebCore::WebSocketChannel::send):
* dom/TaskSource.h:

Source/WebKit:

We no longer need to handle resume/suspend in WebSocketChannel layer since WebSocket will deal with itself by enqueuing a task in the event loop.

* WebProcess/Network/WebSocketChannel.cpp:
(WebKit::WebSocketChannel::disconnect):
(WebKit::WebSocketChannel::didConnect):
(WebKit::WebSocketChannel::didReceiveText):
(WebKit::WebSocketChannel::didReceiveBinaryData):
(WebKit::WebSocketChannel::didClose):
(WebKit::WebSocketChannel::didReceiveMessageError):
(WebKit::WebSocketChannel::suspend):
(WebKit::WebSocketChannel::resume):
(WebKit::WebSocketChannel::didSendHandshakeRequest):
(WebKit::WebSocketChannel::didReceiveHandshakeResponse):
(WebKit::WebSocketChannel::enqueueTask): Deleted.
* WebProcess/Network/WebSocketChannel.h:

LayoutTests:

* http/tests/websocket/tests/hybi/inspector/send-and-receive.html:
The WebSocket server was racing to close the connection with the User Agent.
The patch queueing a task to fire events is further amplifying this race.
To make the test non flaky, we use an echo server that waits for User Agent to close the connection.

Modified Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (284471 => 284472)


--- trunk/LayoutTests/ChangeLog	2021-10-19 18:40:20 UTC (rev 284471)
+++ trunk/LayoutTests/ChangeLog	2021-10-19 18:41:13 UTC (rev 284472)
@@ -1,3 +1,15 @@
+2021-10-19  Youenn Fablet  <you...@apple.com>
+
+        Guarantee order of WebSocket events in case of being resumed
+        https://bugs.webkit.org/show_bug.cgi?id=231664
+
+        Reviewed by Chris Dumez.
+
+        * http/tests/websocket/tests/hybi/inspector/send-and-receive.html:
+        The WebSocket server was racing to close the connection with the User Agent.
+        The patch queueing a task to fire events is further amplifying this race.
+        To make the test non flaky, we use an echo server that waits for User Agent to close the connection.
+
 2021-10-19  Ayumi Kojima  <ayumi_koj...@apple.com>
 
         [ iOS ] http/tests/cache/disk-cache/redirect-chain-limits.html is a flaky timeout.

Modified: trunk/LayoutTests/http/tests/websocket/tests/hybi/inspector/send-and-receive.html (284471 => 284472)


--- trunk/LayoutTests/http/tests/websocket/tests/hybi/inspector/send-and-receive.html	2021-10-19 18:40:20 UTC (rev 284471)
+++ trunk/LayoutTests/http/tests/websocket/tests/hybi/inspector/send-and-receive.html	2021-10-19 18:41:13 UTC (rev 284472)
@@ -9,7 +9,7 @@
 
 function createWebSocketConnection()
 {
-    webSocket = new WebSocket("ws://127.0.0.1:8880/websocket/tests/hybi/inspector/echo");
+    webSocket = new WebSocket("ws://127.0.0.1:8880/websocket/tests/hybi/echo");
 
     webSocket._onopen_ = function()
     {

Modified: trunk/LayoutTests/imported/w3c/ChangeLog (284471 => 284472)


--- trunk/LayoutTests/imported/w3c/ChangeLog	2021-10-19 18:40:20 UTC (rev 284471)
+++ trunk/LayoutTests/imported/w3c/ChangeLog	2021-10-19 18:41:13 UTC (rev 284472)
@@ -1,3 +1,13 @@
+2021-10-19  Youenn Fablet  <you...@apple.com>
+
+        Guarantee order of WebSocket events in case of being resumed
+        https://bugs.webkit.org/show_bug.cgi?id=231664
+
+        Reviewed by Chris Dumez.
+
+        * web-platform-tests/websockets/interfaces/WebSocket/close/close-nested-expected.txt:
+        * web-platform-tests/websockets/interfaces/WebSocket/readyState/003-expected.txt:
+
 2021-10-19  Antoine Quint  <grao...@webkit.org>
 
         REGRESSION(r284313): ::marker accelerated animations are broken

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/websockets/interfaces/WebSocket/close/close-nested-expected.txt (284471 => 284472)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/websockets/interfaces/WebSocket/close/close-nested-expected.txt	2021-10-19 18:40:20 UTC (rev 284471)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/websockets/interfaces/WebSocket/close/close-nested-expected.txt	2021-10-19 18:41:13 UTC (rev 284472)
@@ -1,3 +1,3 @@
 
-FAIL WebSockets: close() in close event handler assert_equals: expected 2 but got 3
+PASS WebSockets: close() in close event handler
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/websockets/interfaces/WebSocket/readyState/003-expected.txt (284471 => 284472)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/websockets/interfaces/WebSocket/readyState/003-expected.txt	2021-10-19 18:40:20 UTC (rev 284471)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/websockets/interfaces/WebSocket/readyState/003-expected.txt	2021-10-19 18:41:13 UTC (rev 284472)
@@ -1,3 +1,3 @@
 
-FAIL WebSockets: delete readyState assert_equals: delete ws.readyState expected 2 but got 3
+PASS WebSockets: delete readyState
 

Modified: trunk/Source/WebCore/ChangeLog (284471 => 284472)


--- trunk/Source/WebCore/ChangeLog	2021-10-19 18:40:20 UTC (rev 284471)
+++ trunk/Source/WebCore/ChangeLog	2021-10-19 18:41:13 UTC (rev 284472)
@@ -1,3 +1,33 @@
+2021-10-19  Youenn Fablet  <you...@apple.com>
+
+        Guarantee order of WebSocket events in case of being resumed
+        https://bugs.webkit.org/show_bug.cgi?id=231664
+
+        Reviewed by Chris Dumez.
+
+        Introduce a WebSocket task source as per spec.
+        This aligns with https://html.spec.whatwg.org/multipage/web-sockets.html where the user agent is expected to a queue task
+        when closing handshake is started and so on.
+
+        By queuing an event loop task to fire WebSocket events, we ensure ordering of events even in case of resuming, while simplifying the implementation.
+        This makes it use the event loop which is extra nice if we resolve promises in event listeners.
+
+        A follow-up patch should refactor code so that WebSocket::didReceiveMessageError can directly fire the close event without having to wait for a didClose call.
+
+        Covered by existing tests.
+
+        * Modules/websockets/WebSocket.cpp:
+        (WebCore::WebSocket::WebSocket):
+        (WebCore::WebSocket::suspend):
+        (WebCore::WebSocket::resume):
+        (WebCore::WebSocket::stop):
+        (WebCore::WebSocket::dispatchOrQueueEvent):
+        (WebCore::WebSocket::resumeTimerFired): Deleted.
+        * Modules/websockets/WebSocket.h:
+        * Modules/websockets/WebSocketChannel.cpp:
+        (WebCore::WebSocketChannel::send):
+        * dom/TaskSource.h:
+
 2021-10-19  Antoine Quint  <grao...@webkit.org>
 
         REGRESSION(r284313): ::marker accelerated animations are broken

Modified: trunk/Source/WebCore/Modules/websockets/WebSocket.cpp (284471 => 284472)


--- trunk/Source/WebCore/Modules/websockets/WebSocket.cpp	2021-10-19 18:40:20 UTC (rev 284471)
+++ trunk/Source/WebCore/Modules/websockets/WebSocket.cpp	2021-10-19 18:41:13 UTC (rev 284472)
@@ -145,7 +145,6 @@
     : ActiveDOMObject(&context)
     , m_subprotocol(emptyString())
     , m_extensions(emptyString())
-    , m_resumeTimer(*this, &WebSocket::resumeTimerFired)
 {
     Locker locker { allActiveWebSocketsLock() };
     allActiveWebSockets().add(this);
@@ -210,15 +209,13 @@
 
 void WebSocket::failAsynchronously()
 {
-    m_pendingActivity = makePendingActivity(*this);
+    queueTaskKeepingObjectAlive(*this, TaskSource::WebSocket, [this] {
+        // We must block this connection. Instead of throwing an exception, we indicate this
+        // using the error event. But since this code executes as part of the WebSocket's
+        // constructor, we have to wait until the constructor has completed before firing the
+        // event; otherwise, users can't connect to the event.
 
-    // We must block this connection. Instead of throwing an exception, we indicate this
-    // using the error event. But since this code executes as part of the WebSocket's
-    // constructor, we have to wait until the constructor has completed before firing the
-    // event; otherwise, users can't connect to the event.
-
-    scriptExecutionContext()->postTask([this, protectedThis = Ref { *this }](auto&) {
-        this->dispatchOrQueueErrorEvent();
+        this->dispatchErrorEventIfNeeded();
         this->stop();
     });
 }
@@ -511,18 +508,16 @@
 
 void WebSocket::suspend(ReasonForSuspension reason)
 {
-    if (m_resumeTimer.isActive())
-        m_resumeTimer.stop();
+    if (!m_channel)
+        return;
 
-    m_shouldDelayEventFiring = true;
+    if (reason == ReasonForSuspension::BackForwardCache) {
+        // This will cause didClose() to be called.
+        m_channel->fail("WebSocket is closed due to suspension.");
+        return;
+    }
 
-    if (m_channel) {
-        if (reason == ReasonForSuspension::BackForwardCache) {
-            // This will cause didClose() to be called.
-            m_channel->fail("WebSocket is closed due to suspension.");
-        } else
-            m_channel->suspend();
-    }
+    m_channel->suspend();
 }
 
 void WebSocket::resume()
@@ -529,27 +524,8 @@
 {
     if (m_channel)
         m_channel->resume();
-
-    if (!m_pendingEvents.isEmpty() && !m_resumeTimer.isActive()) {
-        // Fire the pending events in a timer as we are not allowed to execute arbitrary JS from resume().
-        m_resumeTimer.startOneShot(0_s);
-    }
-
-    m_shouldDelayEventFiring = false;
 }
 
-void WebSocket::resumeTimerFired()
-{
-    Ref<WebSocket> protectedThis(*this);
-
-    ASSERT(!m_pendingEvents.isEmpty());
-
-    // Check m_shouldDelayEventFiring when iterating in case firing an event causes
-    // suspend() to be called.
-    while (!m_pendingEvents.isEmpty() && !m_shouldDelayEventFiring)
-        dispatchEvent(m_pendingEvents.takeFirst());
-}
-
 void WebSocket::stop()
 {
     if (m_channel)
@@ -556,7 +532,6 @@
         m_channel->disconnect();
     m_channel = nullptr;
     m_state = CLOSED;
-    m_pendingEvents.clear();
     ActiveDOMObject::stop();
     m_pendingActivity = nullptr;
 }
@@ -569,46 +544,61 @@
 void WebSocket::didConnect()
 {
     LOG(Network, "WebSocket %p didConnect()", this);
-    if (m_state != CONNECTING) {
-        didClose(0, ClosingHandshakeIncomplete, WebSocketChannel::CloseEventCodeAbnormalClosure, emptyString());
-        return;
-    }
-    ASSERT(scriptExecutionContext());
-    m_state = OPEN;
-    m_subprotocol = m_channel->subprotocol();
-    m_extensions = m_channel->extensions();
-    dispatchOrQueueEvent(Event::create(eventNames().openEvent, Event::CanBubble::No, Event::IsCancelable::No));
+    queueTaskKeepingObjectAlive(*this, TaskSource::WebSocket, [this] {
+        if (m_state == CLOSED)
+            return;
+        if (m_state != CONNECTING) {
+            didClose(0, ClosingHandshakeIncomplete, WebSocketChannel::CloseEventCodeAbnormalClosure, emptyString());
+            return;
+        }
+        ASSERT(scriptExecutionContext());
+        m_state = OPEN;
+        m_subprotocol = m_channel->subprotocol();
+        m_extensions = m_channel->extensions();
+        dispatchEvent(Event::create(eventNames().openEvent, Event::CanBubble::No, Event::IsCancelable::No));
+    });
 }
 
 void WebSocket::didReceiveMessage(const String& msg)
 {
     LOG(Network, "WebSocket %p didReceiveMessage() Text message '%s'", this, msg.utf8().data());
-    if (m_state != OPEN)
-        return;
-    ASSERT(scriptExecutionContext());
-    dispatchOrQueueEvent(MessageEvent::create(msg, SecurityOrigin::create(m_url)->toString()));
+    queueTaskKeepingObjectAlive(*this, TaskSource::WebSocket, [this, msg] {
+        if (m_state != OPEN)
+            return;
+        ASSERT(scriptExecutionContext());
+        dispatchEvent(MessageEvent::create(msg, SecurityOrigin::create(m_url)->toString()));
+    });
 }
 
 void WebSocket::didReceiveBinaryData(Vector<uint8_t>&& binaryData)
 {
     LOG(Network, "WebSocket %p didReceiveBinaryData() %u byte binary message", this, static_cast<unsigned>(binaryData.size()));
-    switch (m_binaryType) {
-    case BinaryType::Blob:
-        // FIXME: We just received the data from NetworkProcess, and are sending it back. This is inefficient.
-        dispatchOrQueueEvent(MessageEvent::create(Blob::create(scriptExecutionContext(), WTFMove(binaryData), emptyString()), SecurityOrigin::create(m_url)->toString()));
-        break;
-    case BinaryType::ArrayBuffer:
-        dispatchOrQueueEvent(MessageEvent::create(ArrayBuffer::create(binaryData.data(), binaryData.size()), SecurityOrigin::create(m_url)->toString()));
-        break;
-    }
+    queueTaskKeepingObjectAlive(*this, TaskSource::WebSocket, [this, binaryData = WTFMove(binaryData)]() mutable {
+        if (m_state != OPEN)
+            return;
+        switch (m_binaryType) {
+        case BinaryType::Blob:
+            // FIXME: We just received the data from NetworkProcess, and are sending it back. This is inefficient.
+            dispatchEvent(MessageEvent::create(Blob::create(scriptExecutionContext(), WTFMove(binaryData), emptyString()), SecurityOrigin::create(m_url)->toString()));
+            break;
+        case BinaryType::ArrayBuffer:
+            dispatchEvent(MessageEvent::create(ArrayBuffer::create(binaryData.data(), binaryData.size()), SecurityOrigin::create(m_url)->toString()));
+            break;
+        }
+    });
 }
 
 void WebSocket::didReceiveMessageError()
 {
     LOG(Network, "WebSocket %p didReceiveErrorMessage()", this);
-    m_state = CLOSED;
-    ASSERT(scriptExecutionContext());
-    dispatchOrQueueErrorEvent();
+    queueTaskKeepingObjectAlive(*this, TaskSource::WebSocket, [this] {
+        if (m_state == CLOSED)
+            return;
+        m_state = CLOSED;
+        ASSERT(scriptExecutionContext());
+        // FIXME: As per https://html.spec.whatwg.org/multipage/web-sockets.html#feedback-from-the-protocol:concept-websocket-closed, we should synchronously fire a close event.
+        dispatchErrorEventIfNeeded();
+    });
 }
 
 void WebSocket::didUpdateBufferedAmount(unsigned bufferedAmount)
@@ -622,26 +612,33 @@
 void WebSocket::didStartClosingHandshake()
 {
     LOG(Network, "WebSocket %p didStartClosingHandshake()", this);
-    m_state = CLOSING;
+    queueTaskKeepingObjectAlive(*this, TaskSource::WebSocket, [this] {
+        if (m_state == CLOSED)
+            return;
+        m_state = CLOSING;
+    });
 }
 
 void WebSocket::didClose(unsigned unhandledBufferedAmount, ClosingHandshakeCompletionStatus closingHandshakeCompletion, unsigned short code, const String& reason)
 {
     LOG(Network, "WebSocket %p didClose()", this);
-    if (!m_channel)
-        return;
-    bool wasClean = m_state == CLOSING && !unhandledBufferedAmount && closingHandshakeCompletion == ClosingHandshakeComplete && code != WebSocketChannel::CloseEventCodeAbnormalClosure;
-    m_state = CLOSED;
-    m_bufferedAmount = unhandledBufferedAmount;
-    ASSERT(scriptExecutionContext());
+    queueTaskKeepingObjectAlive(*this, TaskSource::WebSocket, [this, unhandledBufferedAmount, closingHandshakeCompletion, code, reason] {
+        if (!m_channel)
+            return;
 
-    dispatchOrQueueEvent(CloseEvent::create(wasClean, code, reason));
+        bool wasClean = m_state == CLOSING && !unhandledBufferedAmount && closingHandshakeCompletion == ClosingHandshakeComplete && code != WebSocketChannel::CloseEventCodeAbnormalClosure;
+        m_state = CLOSED;
+        m_bufferedAmount = unhandledBufferedAmount;
+        ASSERT(scriptExecutionContext());
 
-    if (m_channel) {
-        m_channel->disconnect();
-        m_channel = nullptr;
-    }
-    m_pendingActivity = nullptr;
+        dispatchEvent(CloseEvent::create(wasClean, code, reason));
+
+        if (m_channel) {
+            m_channel->disconnect();
+            m_channel = nullptr;
+        }
+        m_pendingActivity = nullptr;
+    });
 }
 
 void WebSocket::didUpgradeURL()
@@ -664,21 +661,13 @@
     return overhead;
 }
 
-void WebSocket::dispatchOrQueueErrorEvent()
+void WebSocket::dispatchErrorEventIfNeeded()
 {
     if (m_dispatchedErrorEvent)
         return;
 
     m_dispatchedErrorEvent = true;
-    dispatchOrQueueEvent(Event::create(eventNames().errorEvent, Event::CanBubble::No, Event::IsCancelable::No));
+    dispatchEvent(Event::create(eventNames().errorEvent, Event::CanBubble::No, Event::IsCancelable::No));
 }
 
-void WebSocket::dispatchOrQueueEvent(Ref<Event>&& event)
-{
-    if (m_shouldDelayEventFiring)
-        m_pendingEvents.append(WTFMove(event));
-    else
-        dispatchEvent(event);
-}
-
 } // namespace WebCore

Modified: trunk/Source/WebCore/Modules/websockets/WebSocket.h (284471 => 284472)


--- trunk/Source/WebCore/Modules/websockets/WebSocket.h	2021-10-19 18:40:20 UTC (rev 284471)
+++ trunk/Source/WebCore/Modules/websockets/WebSocket.h	2021-10-19 18:41:13 UTC (rev 284472)
@@ -33,10 +33,8 @@
 #include "ActiveDOMObject.h"
 #include "EventTarget.h"
 #include "ExceptionOr.h"
-#include "Timer.h"
 #include <wtf/URL.h>
 #include "WebSocketChannelClient.h"
-#include <wtf/Deque.h>
 #include <wtf/HashSet.h>
 #include <wtf/Lock.h>
 
@@ -101,9 +99,7 @@
 private:
     explicit WebSocket(ScriptExecutionContext&);
 
-    void resumeTimerFired();
-    void dispatchOrQueueErrorEvent();
-    void dispatchOrQueueEvent(Ref<Event>&&);
+    void dispatchErrorEventIfNeeded();
 
     void contextDestroyed() final;
     void suspend(ReasonForSuspension) final;
@@ -142,9 +138,6 @@
     String m_subprotocol;
     String m_extensions;
 
-    Timer m_resumeTimer;
-    bool m_shouldDelayEventFiring { false };
-    Deque<Ref<Event>> m_pendingEvents;
     bool m_dispatchedErrorEvent { false };
     RefPtr<PendingActivity<WebSocket>> m_pendingActivity;
 };

Modified: trunk/Source/WebCore/Modules/websockets/WebSocketChannel.cpp (284471 => 284472)


--- trunk/Source/WebCore/Modules/websockets/WebSocketChannel.cpp	2021-10-19 18:40:20 UTC (rev 284471)
+++ trunk/Source/WebCore/Modules/websockets/WebSocketChannel.cpp	2021-10-19 18:41:13 UTC (rev 284472)
@@ -139,6 +139,9 @@
 
 ThreadableWebSocketChannel::SendResult WebSocketChannel::send(const String& message)
 {
+    if (m_outgoingFrameQueueStatus != OutgoingFrameQueueOpen)
+        return ThreadableWebSocketChannel::SendSuccess;
+
     LOG(Network, "WebSocketChannel %p send() Sending String '%s'", this, message.utf8().data());
     CString utf8 = message.utf8(StrictConversionReplacingUnpairedSurrogatesWithFFFD);
     enqueueTextFrame(utf8);
@@ -154,6 +157,9 @@
 
 ThreadableWebSocketChannel::SendResult WebSocketChannel::send(const ArrayBuffer& binaryData, unsigned byteOffset, unsigned byteLength)
 {
+    if (m_outgoingFrameQueueStatus != OutgoingFrameQueueOpen)
+        return ThreadableWebSocketChannel::SendSuccess;
+
     LOG(Network, "WebSocketChannel %p send() Sending ArrayBuffer %p byteOffset=%u byteLength=%u", this, &binaryData, byteOffset, byteLength);
     enqueueRawFrame(WebSocketFrame::OpCodeBinary, static_cast<const uint8_t*>(binaryData.data()) + byteOffset, byteLength);
     processOutgoingFrameQueue();
@@ -162,6 +168,9 @@
 
 ThreadableWebSocketChannel::SendResult WebSocketChannel::send(Blob& binaryData)
 {
+    if (m_outgoingFrameQueueStatus != OutgoingFrameQueueOpen)
+        return ThreadableWebSocketChannel::SendSuccess;
+
     LOG(Network, "WebSocketChannel %p send() Sending Blob '%s'", this, binaryData.url().string().utf8().data());
     enqueueBlobFrame(WebSocketFrame::OpCodeBinary, binaryData);
     processOutgoingFrameQueue();
@@ -170,6 +179,9 @@
 
 bool WebSocketChannel::send(const uint8_t* data, int length)
 {
+    if (m_outgoingFrameQueueStatus != OutgoingFrameQueueOpen)
+        return ThreadableWebSocketChannel::SendSuccess;
+
     LOG(Network, "WebSocketChannel %p send() Sending uint8_t* data="" length=%d", this, data, length);
     enqueueRawFrame(WebSocketFrame::OpCodeBinary, data, length);
     processOutgoingFrameQueue();

Modified: trunk/Source/WebCore/dom/TaskSource.h (284471 => 284472)


--- trunk/Source/WebCore/dom/TaskSource.h	2021-10-19 18:40:20 UTC (rev 284471)
+++ trunk/Source/WebCore/dom/TaskSource.h	2021-10-19 18:41:13 UTC (rev 284472)
@@ -45,6 +45,7 @@
     UserInteraction,
     WebGL,
     WebXR,
+    WebSocket,
 
     // Internal to WebCore
     InternalAsyncTask, // Safe to re-order or delay.

Modified: trunk/Source/WebKit/ChangeLog (284471 => 284472)


--- trunk/Source/WebKit/ChangeLog	2021-10-19 18:40:20 UTC (rev 284471)
+++ trunk/Source/WebKit/ChangeLog	2021-10-19 18:41:13 UTC (rev 284472)
@@ -1,3 +1,26 @@
+2021-10-19  Youenn Fablet  <you...@apple.com>
+
+        Guarantee order of WebSocket events in case of being resumed
+        https://bugs.webkit.org/show_bug.cgi?id=231664
+
+        Reviewed by Chris Dumez.
+
+        We no longer need to handle resume/suspend in WebSocketChannel layer since WebSocket will deal with itself by enqueuing a task in the event loop.
+
+        * WebProcess/Network/WebSocketChannel.cpp:
+        (WebKit::WebSocketChannel::disconnect):
+        (WebKit::WebSocketChannel::didConnect):
+        (WebKit::WebSocketChannel::didReceiveText):
+        (WebKit::WebSocketChannel::didReceiveBinaryData):
+        (WebKit::WebSocketChannel::didClose):
+        (WebKit::WebSocketChannel::didReceiveMessageError):
+        (WebKit::WebSocketChannel::suspend):
+        (WebKit::WebSocketChannel::resume):
+        (WebKit::WebSocketChannel::didSendHandshakeRequest):
+        (WebKit::WebSocketChannel::didReceiveHandshakeResponse):
+        (WebKit::WebSocketChannel::enqueueTask): Deleted.
+        * WebProcess/Network/WebSocketChannel.h:
+
 2021-10-19  David Kilzer  <ddkil...@apple.com>
 
         Follow-up #2: WebKit::LocalConnection::createCredentialPrivateKey leaks an NSMutableDictionary

Modified: trunk/Source/WebKit/WebProcess/Network/WebSocketChannel.cpp (284471 => 284472)


--- trunk/Source/WebKit/WebProcess/Network/WebSocketChannel.cpp	2021-10-19 18:40:20 UTC (rev 284471)
+++ trunk/Source/WebKit/WebProcess/Network/WebSocketChannel.cpp	2021-10-19 18:41:13 UTC (rev 284472)
@@ -232,7 +232,6 @@
 {
     m_client = nullptr;
     m_document = nullptr;
-    m_pendingTasks.clear();
     m_messageQueue.clear();
 
     m_inspector.didCloseWebSocket(m_document.get());
@@ -248,13 +247,6 @@
     if (!m_client)
         return;
 
-    if (m_isSuspended) {
-        enqueueTask([this, subprotocol = WTFMove(subprotocol), extensions = WTFMove(extensions)] () mutable {
-            didConnect(WTFMove(subprotocol), WTFMove(extensions));
-        });
-        return;
-    }
-
     m_subprotocol = WTFMove(subprotocol);
     m_extensions = WTFMove(extensions);
     m_client->didConnect();
@@ -286,13 +278,6 @@
     if (!m_client)
         return;
 
-    if (m_isSuspended) {
-        enqueueTask([this, message = WTFMove(message)] () mutable {
-            didReceiveText(WTFMove(message));
-        });
-        return;
-    }
-
     auto utf8Message = message.utf8();
     m_inspector.didReceiveWebSocketFrame(m_document.get(), createWebSocketFrameForWebInspector(utf8Message.dataAsUInt8Ptr(), utf8Message.length(), WebSocketFrame::OpCode::OpCodeText));
 
@@ -307,14 +292,6 @@
     if (!m_client)
         return;
 
-    if (m_isSuspended) {
-        enqueueTask([this, data = "" () mutable {
-            if (!m_isClosing && m_client)
-                m_client->didReceiveBinaryData(WTFMove(data));
-        });
-        return;
-    }
-
     m_inspector.didReceiveWebSocketFrame(m_document.get(), createWebSocketFrameForWebInspector(data.data(), data.size(), WebSocketFrame::OpCode::OpCodeBinary));
 
     m_client->didReceiveBinaryData(data.vector());
@@ -325,13 +302,6 @@
     if (!m_client)
         return;
 
-    if (m_isSuspended) {
-        enqueueTask([this, code, reason = WTFMove(reason)] () mutable {
-            didClose(code, WTFMove(reason));
-        });
-        return;
-    }
-
     WebSocketFrame closingFrame(WebSocketFrame::OpCodeClose, true, false, false);
     m_inspector.didReceiveWebSocketFrame(m_document.get(), closingFrame);
     m_inspector.didCloseWebSocket(m_document.get());
@@ -364,13 +334,6 @@
     if (!m_client)
         return;
 
-    if (m_isSuspended) {
-        enqueueTask([this, errorMessage = WTFMove(errorMessage)] () mutable {
-            didReceiveMessageError(WTFMove(errorMessage));
-        });
-        return;
-    }
-
     logErrorMessage(errorMessage);
     m_client->didReceiveMessageError();
 }
@@ -382,31 +345,14 @@
 
 void WebSocketChannel::suspend()
 {
-    m_isSuspended = true;
 }
 
 void WebSocketChannel::resume()
 {
-    Ref protectedThis { *this };
-    m_isSuspended = false;
-    while (!m_isSuspended && !m_pendingTasks.isEmpty())
-        m_pendingTasks.takeFirst()();
 }
 
-void WebSocketChannel::enqueueTask(Function<void()>&& task)
-{
-    m_pendingTasks.append(WTFMove(task));
-}
-
 void WebSocketChannel::didSendHandshakeRequest(ResourceRequest&& request)
 {
-    if (m_isSuspended) {
-        enqueueTask([this, request = WTFMove(request)]() mutable {
-            didSendHandshakeRequest(WTFMove(request));
-        });
-        return;
-    }
-
     m_inspector.willSendWebSocketHandshakeRequest(m_document.get(), request);
     m_handshakeRequest = WTFMove(request);
 }
@@ -413,13 +359,6 @@
 
 void WebSocketChannel::didReceiveHandshakeResponse(ResourceResponse&& response)
 {
-    if (m_isSuspended) {
-        enqueueTask([this, response = WTFMove(response)]() mutable {
-            didReceiveHandshakeResponse(WTFMove(response));
-        });
-        return;
-    }
-
     m_inspector.didReceiveWebSocketHandshakeResponse(m_document.get(), response);
     m_handshakeResponse = WTFMove(response);
 }

Modified: trunk/Source/WebKit/WebProcess/Network/WebSocketChannel.h (284471 => 284472)


--- trunk/Source/WebKit/WebProcess/Network/WebSocketChannel.h	2021-10-19 18:40:20 UTC (rev 284471)
+++ trunk/Source/WebKit/WebProcess/Network/WebSocketChannel.h	2021-10-19 18:41:13 UTC (rev 284472)
@@ -96,7 +96,6 @@
     bool increaseBufferedAmount(size_t);
     void decreaseBufferedAmount(size_t);
     template<typename T> void sendMessage(T&&, size_t byteLength);
-    void enqueueTask(Function<void()>&&);
 
     WebCore::WebSocketChannelIdentifier progressIdentifier() const final { return m_inspector.progressIdentifier(); }
     bool hasCreatedHandshake() const final { return !m_url.isNull(); }
@@ -111,8 +110,6 @@
     String m_extensions;
     size_t m_bufferedAmount { 0 };
     bool m_isClosing { false };
-    bool m_isSuspended { false };
-    Deque<Function<void()>> m_pendingTasks;
     WebCore::NetworkSendQueue m_messageQueue;
     WebCore::WebSocketChannelInspector m_inspector;
     WebCore::ResourceRequest m_handshakeRequest;
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to