Attachment 370479 Details for Bug 197873 – Patch

[patch] Patch

bug-197873-20190523113958.patch (text/plain), 10.69 KB, created by Takashi Komori on 2019-05-22 19:42:30 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Takashi Komori

Created: 2019-05-22 19:42:30 PDT

Size: 10.69 KB

patch

obsolete

>Subversion Revision: 245243
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 8848d916c96fc9f5bcef74dd9b26f80cd7b61e43..e64be1ec61cfa618a60a1a4c8378a17770bdca77 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,24 @@
>+2019-05-22  Takashi Komori  <Takashi.Komori@sony.com>
>+
>+        [CURL] Fix crashing SocketStreamHandle.
>+        https://bugs.webkit.org/show_bug.cgi?id=197873
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        When NetworkSocketStream is destructed SocketStreamHandleImple::platformClose is 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-13  Antti Koivisto  <antti@apple.com>
> 
>         REGRESSION (r245208): compositing/shared-backing/sharing-bounds-non-clipping-shared-layer.html asserts
>diff --git a/Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h b/Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h
>index b32c56a9941a59bd4d4ce432009bd6ec48788e20..3bc4ec666234f3907cbfae0e2c2276e2ba1b2cd0 100644
>--- a/Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h
>+++ b/Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h
>@@ -34,9 +34,10 @@
> #include "CurlContext.h"
> #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>
> #include <wtf/UniqueArray.h>
>@@ -67,6 +68,9 @@ private:
>     void handleError(CURLcode);
>     void stopThread();
> 
>+    void callOnWorkerThread(Function<void()>&&);
>+    void executeTasks();
>+
>     static const size_t kWriteBufferSize = 4 * 1024;
>     static const size_t kReadBufferSize = 4 * 1024;
> 
>@@ -74,9 +78,14 @@ private:
>     RefPtr<Thread> m_workerThread;
>     std::atomic<bool> m_running { true };
> 
>-    std::atomic<size_t> m_writeBufferSize { 0 };
>-    size_t m_writeBufferOffset;
>-    UniqueArray<uint8_t> m_writeBuffer;
>+    MessageQueue<Function<void()>> m_taskQueue;
>+
>+    using WriteBuffer = StreamBuffer<uint8_t, 1024 * 1024>;
>+    static const size_t kMaxScheduledWriteDataSize = 1024 * 1024;
>+
>+    Lock m_writeMutex;
>+    size_t m_scheduledWriteDataSize { 0 };
>+    std::unique_ptr<WriteBuffer> m_writeBuffer { nullptr };
> 
>     StreamBuffer<uint8_t, 1024 * 1024> m_buffer;
>     static const unsigned maxBufferSize = 100 * 1024 * 1024;
>diff --git a/Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp b/Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp
>index ddec679eb9885045526490b0cc5c07322a52dea6..224693931ec3583d019681d405707c7c185c3d61 100644
>--- a/Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp
>+++ b/Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp
>@@ -40,9 +40,10 @@
> #include "SocketStreamError.h"
> #include "SocketStreamHandleClient.h"
> #include "StorageSessionProvider.h"
>-#include <wtf/Lock.h>
>+#include <wtf/CrossThreadCopier.h>
> #include <wtf/MainThread.h>
> #include <wtf/URL.h>
>+#include <wtf/Vector.h>
> #include <wtf/text/CString.h>
> 
> namespace WebCore {
>@@ -74,24 +75,24 @@ Optional<size_t> SocketStreamHandleImpl::platformSendInternal(const uint8_t* dat
>     LOG(Network, "SocketStreamHandle %p platformSend", this);
>     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())
>-        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);
>+    {
>+        auto locker = holdLock(m_writeMutex);
>+
>+        length = std::min(kMaxScheduledWriteDataSize - m_scheduledWriteDataSize, length);
>+        if (!length)
>+            return 0;
>+
>+        m_scheduledWriteDataSize += length;
>+    }
>+
>+    Vector<uint8_t> sendData(length);
>+    memcpy(sendData.data(), data, length);
>+
>+    callOnWorkerThread([this, sendData = crossThreadCopy(sendData)]() {
>+        ASSERT(!isMainThread());
>+        ASSERT(m_writeBuffer.get());
>+        m_writeBuffer->append(sendData.data(), sendData.size());
>+    });
> 
>     return length;
> }
>@@ -103,6 +104,7 @@ void SocketStreamHandleImpl::platformClose()
> 
>     if (m_state == Closed)
>         return;
>+    m_state = Closed;
> 
>     stopThread();
>     m_client.didCloseSocketStream(*this);
>@@ -112,7 +114,7 @@ void SocketStreamHandleImpl::threadEntryPoint()
> {
>     ASSERT(!isMainThread());
> 
>-    CurlSocketHandle socket { m_url, [this](CURLcode errorCode) {
>+    CurlSocketHandle socket { m_url.isolatedCopy(), [this](CURLcode errorCode) {
>         handleError(errorCode);
>     }};
> 
>@@ -127,25 +129,26 @@ void SocketStreamHandleImpl::threadEntryPoint()
>         }
>     });
> 
>+    m_writeBuffer = std::make_unique<WriteBuffer>();
>+
>     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->size());
>         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 {
>-                m_writeBuffer = nullptr;
>-                m_writeBufferOffset = 0;
>-                m_writeBufferSize.store(0);
>+            auto bytesSent = socket.send(m_writeBuffer->firstBlockData(), std::min(m_writeBuffer->firstBlockSize(), kWriteBufferSize));
>+            if (bytesSent) {
>+                m_writeBuffer->consume(bytesSent);
>+
>+                {
>+                    auto locker = holdLock(m_writeMutex);
>+                    m_scheduledWriteDataSize -= bytesSent;
>+                }
>+
>                 callOnMainThread([this, protectedThis = makeRef(*this)] {
>                     sendPendingData();
>                 });
>@@ -174,12 +177,17 @@ void SocketStreamHandleImpl::threadEntryPoint()
>             });
>         }
>     }
>+
>+    m_writeBuffer = nullptr;
> }
> 
> void SocketStreamHandleImpl::handleError(CURLcode errorCode)
> {
>     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);
>         else
>@@ -199,6 +207,21 @@ void SocketStreamHandleImpl::stopThread()
>     }
> }
> 
>+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
> 
> #endif
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 1f22cab067665509f753132f185a2989de447368..c72d54b6b061897501036498353a63dbe47d0cb4 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,13 @@
>+2019-05-22  Takashi Komori  <Takashi.Komori@sony.com>
>+
>+        [CURL] Fix crashing SocketStreamHandle.
>+        https://bugs.webkit.org/show_bug.cgi?id=197873
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * platform/wincairo-wk1/TestExpectations:
>+        * platform/wincairo/TestExpectations:
>+
> 2019-05-13  Antti Koivisto  <antti@apple.com>
> 
>         REGRESSION (r245208): compositing/shared-backing/sharing-bounds-non-clipping-shared-layer.html asserts
>diff --git a/LayoutTests/platform/wincairo-wk1/TestExpectations b/LayoutTests/platform/wincairo-wk1/TestExpectations
>index 97a11f4b9276be3f74bbb1390aff8e956ad9198e..afb27269f59fd35c97d1ebfd24115be7423caea5 100644
>--- a/LayoutTests/platform/wincairo-wk1/TestExpectations
>+++ b/LayoutTests/platform/wincairo-wk1/TestExpectations
>@@ -12,6 +12,8 @@
> 
> # 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 ]
> http/tests/cookies/third-party-cookie-relaxing.html [ Skip ]
>diff --git a/LayoutTests/platform/wincairo/TestExpectations b/LayoutTests/platform/wincairo/TestExpectations
>index 4ee7e05efb53e00d69fd3d1dd6b9e1c408136836..30aa43795b141269e3665fb259dbaf8e451a35b4 100644
>--- a/LayoutTests/platform/wincairo/TestExpectations
>+++ b/LayoutTests/platform/wincairo/TestExpectations
>@@ -970,7 +970,6 @@ http/tests/websocket/tests/hybi/upgrade-simple-ws.html [ Pass Failure ]
> http/tests/websocket/tests/hybi/websocket-allowed-setting-cookie-as-third-party.html [ Pass Failure ]
> 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 ]
> 
> http/tests/workers/service [ Skip ]

Actions: View | Formatted Diff | Diff

Attachments on bug 197873: 369828 | 369839 | 370302 | 370388 | 370392 | 370479 | 370633 | 370634 | 370668