Changeset 245802 in webkit

Timestamp:

May 27, 2019 5:10:41 PM (5 years ago)

Author:

commit-queue@webkit.org

Message:

[CURL] Fix crashing SocketStreamHandle.
​https://bugs.webkit.org/show_bug.cgi?id=197873

Patch by Takashi Komori <​Takashi.Komori@sony.com> on 2019-05-27
Reviewed by Fujii Hironori.

Source/WebCore:

When NetworkSocketStream was destructed SocketStreamHandleImple::platformClose was called wrongly times.
This is because closed state is not set.

Test: http/tests/websocket/tests/hybi/workers/close.html

• platform/network/curl/SocketStreamHandleImpl.h:
• platform/network/curl/SocketStreamHandleImplCurl.cpp:

(WebCore::SocketStreamHandleImpl::platformSendInternal):
(WebCore::SocketStreamHandleImpl::platformClose):
(WebCore::SocketStreamHandleImpl::threadEntryPoint):
(WebCore::SocketStreamHandleImpl::handleError):
(WebCore::SocketStreamHandleImpl::callOnWorkerThread):
(WebCore::SocketStreamHandleImpl::executeTasks):

LayoutTests:

• platform/wincairo-wk1/TestExpectations:
• platform/wincairo/TestExpectations:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/wincairo-wk1/TestExpectations (modified) (1 diff)
• LayoutTests/platform/wincairo/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h (modified) (3 diffs)
• Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp (modified) (8 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2019-05-27  Takashi Komori  <Takashi.Komori@sony.com>
+
+        [CURL] Fix crashing SocketStreamHandle.
+        https://bugs.webkit.org/show_bug.cgi?id=197873
+
+        Reviewed by Fujii Hironori.
+
+        * platform/wincairo-wk1/TestExpectations:
+        * platform/wincairo/TestExpectations:
+
 2019-05-27  Oriol Brufau  <obrufau@igalia.com>
 

trunk/LayoutTests/platform/wincairo-wk1/TestExpectations

@@ -294 +294 @@
 # Failures on WebKit Legacy
 
+webkit.org/b/89153 http/tests/websocket/tests/hybi/workers/close.html [ Pass Failure ]
+
 # Cookie policy only supported in WK2.
 http/tests/cookies/only-accept-first-party-cookies.html [ Skip ]

trunk/LayoutTests/platform/wincairo/TestExpectations

@@ -977 +977 @@
 http/tests/websocket/tests/hybi/websocket-blocked-from-setting-cookie-as-third-party.html [ Pass Failure ]
 http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html [ Pass Failure ]
-http/tests/websocket/tests/hybi/workers/close.html [ Pass Failure ]
 http/tests/websocket/tests/hybi/workers/worker-reload.html [ Timeout Pass ]
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-05-27  Takashi Komori  <Takashi.Komori@sony.com>
+
+        [CURL] Fix crashing SocketStreamHandle.
+        https://bugs.webkit.org/show_bug.cgi?id=197873
+
+        Reviewed by Fujii Hironori.
+
+        When NetworkSocketStream was destructed SocketStreamHandleImple::platformClose was called wrongly times.
+        This is because closed state is not set.
+
+        Test: http/tests/websocket/tests/hybi/workers/close.html
+
+        * platform/network/curl/SocketStreamHandleImpl.h:
+        * platform/network/curl/SocketStreamHandleImplCurl.cpp:
+        (WebCore::SocketStreamHandleImpl::platformSendInternal):
+        (WebCore::SocketStreamHandleImpl::platformClose):
+        (WebCore::SocketStreamHandleImpl::threadEntryPoint):
+        (WebCore::SocketStreamHandleImpl::handleError):
+        (WebCore::SocketStreamHandleImpl::callOnWorkerThread):
+        (WebCore::SocketStreamHandleImpl::executeTasks):
+
 2019-05-27  Oriol Brufau  <obrufau@igalia.com>
 

trunk/Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h

@@ -35 +35 @@
 #include "SocketStreamHandle.h"
 #include <pal/SessionID.h>
+#include <wtf/Function.h>
 #include <wtf/Lock.h>
+#include <wtf/MessageQueue.h>
 #include <wtf/RefCounted.h>
-#include <wtf/Seconds.h>
 #include <wtf/StreamBuffer.h>
 #include <wtf/Threading.h>
@@ -68 +69 @@
     void stopThread();
 
-    static const size_t kWriteBufferSize = 4 * 1024;
+    void callOnWorkerThread(Function<void()>&&);
+    void executeTasks();
+
     static const size_t kReadBufferSize = 4 * 1024;
 
@@ -75 +78 @@
     std::atomic<bool> m_running { true };
 
-    std::atomic<size_t> m_writeBufferSize { 0 };
-    size_t m_writeBufferOffset;
+    MessageQueue<Function<void()>> m_taskQueue;
+
+    bool m_hasPendingWriteData { false };
+    size_t m_writeBufferSize { 0 };
+    size_t m_writeBufferOffset { 0 };
     UniqueArray<uint8_t> m_writeBuffer;
 

trunk/Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp

@@ -41 +41 @@
 #include "SocketStreamHandleClient.h"
 #include "StorageSessionProvider.h"
-#include <wtf/Lock.h>
 #include <wtf/MainThread.h>
 #include <wtf/URL.h>
@@ -75 +74 @@
     ASSERT(isMainThread());
 
-    // If there's data waiting, return zero to indicate whole data should be put in a m_buffer.
-    // This is thread-safe because state is read in atomic. Also even if the state is cleared by
-    // worker thread between load() and evaluation of size, it is okay because invocation of
-    // sendPendingData() is serialized in the main thread, so that another call will be happen
-    // immediately.
-    if (m_writeBufferSize.load())
+    if (m_hasPendingWriteData)
         return 0;
 
-    if (length > kWriteBufferSize)
-        length = kWriteBufferSize;
-
-    // We copy the buffer and then set the state atomically to say there are bytes available.
-    // The worker thread will skip reading the buffer if no bytes are available, so it won't
-    // access the buffer prematurely
-    m_writeBuffer = makeUniqueArray<uint8_t>(length);
-    memcpy(m_writeBuffer.get(), data, length);
-    m_writeBufferOffset = 0;
-    m_writeBufferSize.store(length);
+    m_hasPendingWriteData = true;
+
+    auto writeBuffer = makeUniqueArray<uint8_t>(length);
+    memcpy(writeBuffer.get(), data, length);
+
+    callOnWorkerThread([this, writeBuffer = WTFMove(writeBuffer), writeBufferSize = length]() mutable {
+        ASSERT(!isMainThread());
+        m_writeBuffer = WTFMove(writeBuffer);
+        m_writeBufferSize = writeBufferSize;
+        m_writeBufferOffset = 0;
+    });
 
     return length;
@@ -104 +99 @@
     if (m_state == Closed)
         return;
+    m_state = Closed;
 
     stopThread();
@@ -113 +109 @@
     ASSERT(!isMainThread());
 
-    CurlSocketHandle socket { m_url, [this](CURLcode errorCode) {
+    CurlSocketHandle socket { m_url.isolatedCopy(), [this](CURLcode errorCode) {
         handleError(errorCode);
     }};
@@ -129 +125 @@
 
     while (m_running) {
-        auto writeBufferSize = m_writeBufferSize.load();
-        auto result = socket.wait(20_ms, writeBufferSize > 0);
+        executeTasks();
+
+        auto result = socket.wait(20_ms, m_writeBuffer.get());
         if (!result)
             continue;
 
-        // These logic only run when there's data waiting. In that case, m_writeBufferSize won't
-        // updated by `platformSendInternal()` running in main thread.
+        // These logic only run when there's data waiting.
         if (result->writable && m_running) {
-            auto offset = m_writeBufferOffset;
-            auto bytesSent = socket.send(m_writeBuffer.get() + offset, writeBufferSize - offset);
-            offset += bytesSent;
-
-            if (writeBufferSize > offset)
-                m_writeBufferOffset = offset;
-            else {
+            auto bytesSent = socket.send(m_writeBuffer.get() + m_writeBufferOffset, m_writeBufferSize - m_writeBufferOffset);
+            m_writeBufferOffset += bytesSent;
+
+            if (m_writeBufferSize <= m_writeBufferOffset) {
                 m_writeBuffer = nullptr;
+                m_writeBufferSize = 0;
                 m_writeBufferOffset = 0;
-                m_writeBufferSize.store(0);
+
                 callOnMainThread([this, protectedThis = makeRef(*this)] {
+                    m_hasPendingWriteData = false;
                     sendPendingData();
                 });
@@ -175 +170 @@
         }
     }
+
+    m_writeBuffer = nullptr;
 }
 
@@ -180 +177 @@
 {
     m_running = false;
-    callOnMainThread([this, protectedThis = makeRef(*this), errorCode, localizedDescription = CurlHandle::errorDescription(errorCode)] {
+    callOnMainThread([this, protectedThis = makeRef(*this), errorCode, localizedDescription = CurlHandle::errorDescription(errorCode).isolatedCopy()] {
+        if (m_state == Closed)
+            return;
+
         if (errorCode == CURLE_RECV_ERROR)
             m_client.didFailToReceiveSocketStreamData(*this);
@@ -200 +200 @@
 }
 
+void SocketStreamHandleImpl::callOnWorkerThread(Function<void()>&& task)
+{
+    ASSERT(isMainThread());
+    m_taskQueue.append(std::make_unique<Function<void()>>(WTFMove(task)));
+}
+
+void SocketStreamHandleImpl::executeTasks()
+{
+    ASSERT(!isMainThread());
+
+    auto tasks = m_taskQueue.takeAllMessages();
+    for (auto& task : tasks)
+        (*task)();
+}
+
 } // namespace WebCore