Attachment 370668 Details for Bug 197873 – Patch

[patch] Patch

bug-197873-20190527142146.patch (text/plain), 10.25 KB, created by Takashi Komori on 2019-05-26 22:24:15 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Takashi Komori

Created: 2019-05-26 22:24:15 PDT

Size: 10.25 KB

patch

obsolete

>Subversion Revision: 245606
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 4c1401e329f7788ce80318a5496d23680df8ecfb..ca81b28d825cb904a20d3921da6993e6e87cd88e 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,24 @@
>+2019-05-26  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 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-21  Myles C. Maxfield  <mmaxfield@apple.com>
> 
>         font-optical-sizing applies the wrong variation value
>diff --git a/Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h b/Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h
>index b32c56a9941a59bd4d4ce432009bd6ec48788e20..ae935ad87311ba87fd47ad2b6fb51920fca55ac3 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,15 +68,20 @@ private:
>     void handleError(CURLcode);
>     void stopThread();
> 
>-    static const size_t kWriteBufferSize = 4 * 1024;
>+    void callOnWorkerThread(Function<void()>&&);
>+    void executeTasks();
>+
>     static const size_t kReadBufferSize = 4 * 1024;
> 
>     RefPtr<const StorageSessionProvider> m_storageSessionProvider;
>     RefPtr<Thread> m_workerThread;
>     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;
> 
>     StreamBuffer<uint8_t, 1024 * 1024> m_buffer;
>diff --git a/Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp b/Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp
>index ddec679eb9885045526490b0cc5c07322a52dea6..026e6c8f1f48a245452c91f082f698ec01138377 100644
>--- a/Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp
>+++ b/Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp
>@@ -40,7 +40,6 @@
> #include "SocketStreamError.h"
> #include "SocketStreamHandleClient.h"
> #include "StorageSessionProvider.h"
>-#include <wtf/Lock.h>
> #include <wtf/MainThread.h>
> #include <wtf/URL.h>
> #include <wtf/text/CString.h>
>@@ -74,24 +73,20 @@ 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())
>+    if (m_hasPendingWriteData)
>         return 0;
> 
>-    if (length > kWriteBufferSize)
>-        length = kWriteBufferSize;
>+    m_hasPendingWriteData = true;
> 
>-    // 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 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;
> }
>@@ -103,6 +98,7 @@ void SocketStreamHandleImpl::platformClose()
> 
>     if (m_state == Closed)
>         return;
>+    m_state = Closed;
> 
>     stopThread();
>     m_client.didCloseSocketStream(*this);
>@@ -112,7 +108,7 @@ void SocketStreamHandleImpl::threadEntryPoint()
> {
>     ASSERT(!isMainThread());
> 
>-    CurlSocketHandle socket { m_url, [this](CURLcode errorCode) {
>+    CurlSocketHandle socket { m_url.isolatedCopy(), [this](CURLcode errorCode) {
>         handleError(errorCode);
>     }};
> 
>@@ -128,25 +124,24 @@ void SocketStreamHandleImpl::threadEntryPoint()
>     });
> 
>     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;
>+            auto bytesSent = socket.send(m_writeBuffer.get() + m_writeBufferOffset, m_writeBufferSize - m_writeBufferOffset);
>+            m_writeBufferOffset += bytesSent;
> 
>-            if (writeBufferSize > offset)
>-                m_writeBufferOffset = offset;
>-            else {
>+            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();
>                 });
>             }
>@@ -174,12 +169,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 +199,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 839b0fd34cf435824a22ced30eabc25488aecc8a..181c90098d9d91494f586b834a7546ab740bb3c7 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,13 @@
>+2019-05-26  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-21  Myles C. Maxfield  <mmaxfield@apple.com>
> 
>         font-optical-sizing applies the wrong variation value
>diff --git a/LayoutTests/platform/wincairo-wk1/TestExpectations b/LayoutTests/platform/wincairo-wk1/TestExpectations
>index b1a56b62ca4c955a5fc6df22a266287542988afa..9a53fb8751dfca3b30e103f1e584e5464f3e7eb0 100644
>--- a/LayoutTests/platform/wincairo-wk1/TestExpectations
>+++ b/LayoutTests/platform/wincairo-wk1/TestExpectations
>@@ -293,6 +293,8 @@ webkit.org/b/183955 accessibility/row-with-aria-role-in-native-table.html [ Fail
> 
> # 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 0bfdf12c627ca23f18005088e56978778e874b87..df908d305a60a7772a1429cdd28b2a90ad56e0f5 100644
>--- a/LayoutTests/platform/wincairo/TestExpectations
>+++ b/LayoutTests/platform/wincairo/TestExpectations
>@@ -976,7 +976,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