Attachment 368298 Details for Bug 191650 – Patch

[patch] Patch

bug-191650-20190426115803.patch (text/plain), 15.40 KB, created by Takashi Komori on 2019-04-25 20:00:53 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Takashi Komori

Created: 2019-04-25 20:00:53 PDT

Size: 15.40 KB

patch

obsolete

>Subversion Revision: 244103
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 42806aa74fe4caf3b55b73faf16f505e2479f39e..b3f78b8456061bb2a8ec0ab76a6200b6c50cc28d 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,37 @@
>+2019-04-25  Takashi Komori  <Takashi.Komori@sony.com>
>+
>+        [Curl] Fix Curl Request Scheduler not to release wrong Curl handle when request is cancelled.
>+        https://bugs.webkit.org/show_bug.cgi?id=191650
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Test: http/tests/misc/repeat-open-cancel.html
>+
>+        * platform/network/curl/CurlRequest.cpp:
>+        (WebCore::CurlRequest::cancel):
>+        (WebCore::CurlRequest::isCancelled):
>+        (WebCore::CurlRequest::isCompletedOrCancelled):
>+        (WebCore::CurlRequest::didCompleteTransfer):
>+        (WebCore::CurlRequest::completeDidReceiveResponse):
>+        (WebCore::CurlRequest::pausedStatusChanged):
>+        * platform/network/curl/CurlRequest.h:
>+        (WebCore::CurlRequest::isCompleted const): Deleted.
>+        (WebCore::CurlRequest::isCancelled const): Deleted.
>+        (WebCore::CurlRequest::isCompletedOrCancelled const): Deleted.
>+        * platform/network/curl/CurlRequestScheduler.cpp:
>+        (WebCore::CurlRequestScheduler::cancel):
>+        (WebCore::CurlRequestScheduler::callOnWorkerThread):
>+        (WebCore::CurlRequestScheduler::startThreadIfNeeded):
>+        (WebCore::CurlRequestScheduler::stopThreadIfNoMoreJobRunning):
>+        (WebCore::CurlRequestScheduler::stopThread):
>+        (WebCore::CurlRequestScheduler::executeTasks):
>+        (WebCore::CurlRequestScheduler::workerThread):
>+        (WebCore::CurlRequestScheduler::startTransfer):
>+        (WebCore::CurlRequestScheduler::completeTransfer):
>+        (WebCore::CurlRequestScheduler::cancelTransfer):
>+        (WebCore::CurlRequestScheduler::finalizeTransfer):
>+        * platform/network/curl/CurlRequestScheduler.h:
>+
> 2019-04-09  Keith Rollin  <krollin@apple.com>
> 
>         Unreviewed build maintenance -- update .xcfilelists.
>diff --git a/Source/WebCore/platform/network/curl/CurlRequest.cpp b/Source/WebCore/platform/network/curl/CurlRequest.cpp
>index 3aaf891180baf30b08662ca70121d1b5e9172063..17dcfd7bbcff5ea5780cd15cbdf7ca64bd40c007 100644
>--- a/Source/WebCore/platform/network/curl/CurlRequest.cpp
>+++ b/Source/WebCore/platform/network/curl/CurlRequest.cpp
>@@ -126,10 +126,13 @@ void CurlRequest::cancel()
> {
>     ASSERT(isMainThread());
> 
>-    if (isCompletedOrCancelled())
>-        return;
>+    {
>+        auto locker = holdLock(m_statusMutex);
>+        if (m_cancelled)
>+            return;
> 
>-    m_cancelled = true;
>+        m_cancelled = true;
>+    }
> 
>     auto& scheduler = CurlContext::singleton().scheduler();
> 
>@@ -143,6 +146,18 @@ void CurlRequest::cancel()
>     invalidateClient();
> }
> 
>+bool CurlRequest::isCancelled()
>+{
>+    auto locker = holdLock(m_statusMutex);
>+    return m_cancelled;
>+}
>+
>+bool CurlRequest::isCompletedOrCancelled()
>+{
>+    auto locker = holdLock(m_statusMutex);
>+    return m_completed || m_cancelled;
>+}
>+
> void CurlRequest::suspend()
> {
>     ASSERT(isMainThread());
>@@ -415,8 +430,8 @@ void CurlRequest::didReceiveDataFromMultipart(Ref<SharedBuffer>&& buffer)
> 
> void CurlRequest::didCompleteTransfer(CURLcode result)
> {
>-    if (m_cancelled) {
>-        m_curlHandle = nullptr;
>+    if (isCancelled()) {
>+        didCancelTransfer();
>         return;
>     }
> 
>@@ -455,6 +470,11 @@ void CurlRequest::didCompleteTransfer(CURLcode result)
>             client.curlDidFailWithError(request, error);
>         });
>     }
>+
>+    {
>+        auto locker = holdLock(m_statusMutex);
>+        m_completed = true;
>+    }
> }
> 
> void CurlRequest::didCancelTransfer()
>@@ -567,10 +587,7 @@ void CurlRequest::completeDidReceiveResponse()
>     ASSERT(m_didNotifyResponse);
>     ASSERT(!m_didReturnFromNotify || m_multipartHandle);
> 
>-    if (isCancelled())
>-        return;
>-
>-    if (m_actionAfterInvoke != Action::StartTransfer && isCompleted())
>+    if (isCompletedOrCancelled())
>         return;
> 
>     m_didReturnFromNotify = true;
>@@ -635,7 +652,7 @@ void CurlRequest::pausedStatusChanged()
>         return;
> 
>     runOnWorkerThreadIfRequired([this, protectedThis = makeRef(*this)]() {
>-        if (isCompletedOrCancelled())
>+        if (isCompletedOrCancelled() || !m_curlHandle)
>             return;
> 
>         bool needCancel { false };
>diff --git a/Source/WebCore/platform/network/curl/CurlRequest.h b/Source/WebCore/platform/network/curl/CurlRequest.h
>index 8ac48e94d841f67439df3c9934e8a4f7443b3d41..d13766e3bf4e3b025b8e8784f2109fb1355b8c28 100644
>--- a/Source/WebCore/platform/network/curl/CurlRequest.h
>+++ b/Source/WebCore/platform/network/curl/CurlRequest.h
>@@ -83,9 +83,8 @@ public:
>     WEBCORE_EXPORT void resume();
> 
>     const ResourceRequest& resourceRequest() const { return m_request; }
>-    bool isCompleted() const { return !m_curlHandle; }
>-    bool isCancelled() const { return m_cancelled; }
>-    bool isCompletedOrCancelled() const { return isCompleted() || isCancelled(); }
>+    bool isCancelled();
>+    bool isCompletedOrCancelled();
>     Seconds timeoutInterval() const;
> 
>     const String& user() const { return m_user; }
>@@ -166,7 +165,9 @@ private:
> 
> 
>     CurlRequestClient* m_client { };
>+    Lock m_statusMutex;
>     bool m_cancelled { false };
>+    bool m_completed { false };
>     MessageQueue<Function<void()>>* m_messageQueue { };
> 
>     ResourceRequest m_request;
>diff --git a/Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp b/Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp
>index 11f21002705d24ce15e3e8b1f46a8b4eee4b8864..91837af9f3e4773b5c71c1c260db96f586b211af 100644
>--- a/Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp
>+++ b/Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp
>@@ -58,16 +58,16 @@ void CurlRequestScheduler::cancel(CurlRequestSchedulerClient* client)
> {
>     ASSERT(isMainThread());
> 
>-    if (!client || !client->handle())
>+    if (!client)
>         return;
> 
>-    cancelTransfer(client->handle());
>+    cancelTransfer(client);
> }
> 
> void CurlRequestScheduler::callOnWorkerThread(WTF::Function<void()>&& task)
> {
>     {
>-        LockHolder locker(m_mutex);
>+        auto locker = holdLock(m_mutex);
>         m_taskQueue.append(WTFMove(task));
>     }
> 
>@@ -78,28 +78,34 @@ void CurlRequestScheduler::startThreadIfNeeded()
> {
>     ASSERT(isMainThread());
> 
>-    LockHolder locker(m_mutex);
>-    if (!m_runThread) {
>-        if (m_thread)
>-            m_thread->waitForCompletion();
>+    {
>+        auto locker = holdLock(m_mutex);
>+        if (m_runThread)
>+            return;
>+    }
> 
>+    if (m_thread)
>+        m_thread->waitForCompletion();
>+
>+    {
>+        auto locker = holdLock(m_mutex);
>         m_runThread = true;
>-        m_thread = Thread::create("curlThread", [this] {
>-            workerThread();
>-            m_runThread = false;
>-        });
>     }
>+
>+    m_thread = Thread::create("curlThread", [this] {
>+        workerThread();
>+
>+        auto locker = holdLock(m_mutex);
>+        m_runThread = false;
>+    });
> }
> 
> void CurlRequestScheduler::stopThreadIfNoMoreJobRunning()
> {
>     ASSERT(!isMainThread());
> 
>-    if (m_activeJobs.size())
>-        return;
>-
>-    LockHolder locker(m_mutex);
>-    if (m_taskQueue.size())
>+    auto locker = holdLock(m_mutex);
>+    if (m_activeJobs.size() || m_taskQueue.size())
>         return;
> 
>     m_runThread = false;
>@@ -107,7 +113,10 @@ void CurlRequestScheduler::stopThreadIfNoMoreJobRunning()
> 
> void CurlRequestScheduler::stopThread()
> {
>-    m_runThread = false;
>+    {
>+        auto locker = holdLock(m_mutex);
>+        m_runThread = false;
>+    }
> 
>     if (m_thread) {
>         m_thread->waitForCompletion();
>@@ -122,7 +131,7 @@ void CurlRequestScheduler::executeTasks()
>     Vector<WTF::Function<void()>> taskQueue;
> 
>     {
>-        LockHolder locker(m_mutex);
>+        auto locker = holdLock(m_mutex);
>         taskQueue = WTFMove(m_taskQueue);
>     }
> 
>@@ -139,7 +148,13 @@ void CurlRequestScheduler::workerThread()
>     m_curlMultiHandle->setMaxTotalConnections(m_maxTotalConnections);
>     m_curlMultiHandle->setMaxHostConnections(m_maxHostConnections);
> 
>-    while (m_runThread) {
>+    while (true) {
>+        {
>+            auto locker = holdLock(m_mutex);
>+            if (!m_runThread)
>+                break;
>+        }
>+
>         executeTasks();
> 
>         // Retry 'select' if it was interrupted by a process signal.
>@@ -177,7 +192,8 @@ void CurlRequestScheduler::workerThread()
>                 break;
> 
>             ASSERT(msg->msg == CURLMSG_DONE);
>-            completeTransfer(msg->easy_handle, msg->data.result);
>+            if (auto client = m_clientMaps.inlineGet(msg->easy_handle))
>+                completeTransfer(client, msg->data.result);
>         }
> 
>         stopThreadIfNoMoreJobRunning();
>@@ -192,49 +208,59 @@ void CurlRequestScheduler::startTransfer(CurlRequestSchedulerClient* client)
> 
>     auto task = [this, client]() {
>         CURL* handle = client->setupTransfer();
>-        if (!handle)
>+        if (!handle) {
>+            completeTransfer(client, CURLE_FAILED_INIT);
>             return;
>+        }
> 
>-        m_activeJobs.add(handle, client);
>         m_curlMultiHandle->addHandle(handle);
>+
>+        ASSERT(!m_clientMaps.contains(handle));
>+        m_clientMaps.set(handle, client);
>     };
> 
>-    LockHolder locker(m_mutex);
>+    auto locker = holdLock(m_mutex);
>+    m_activeJobs.add(client);
>     m_taskQueue.append(WTFMove(task));
> }
> 
>-void CurlRequestScheduler::completeTransfer(CURL* handle, CURLcode result)
>+void CurlRequestScheduler::completeTransfer(CurlRequestSchedulerClient* client, CURLcode result)
> {
>-    finalizeTransfer(handle, [result](CurlRequestSchedulerClient* client) {
>+    finalizeTransfer(client, [client, result]() {
>         client->didCompleteTransfer(result);
>     });
> }
> 
>-void CurlRequestScheduler::cancelTransfer(CURL* handle)
>+void CurlRequestScheduler::cancelTransfer(CurlRequestSchedulerClient* client)
> {
>-    finalizeTransfer(handle, [](CurlRequestSchedulerClient* client) {
>+    finalizeTransfer(client, [client]() {
>         client->didCancelTransfer();
>     });
> }
> 
>-void CurlRequestScheduler::finalizeTransfer(CURL* handle, Function<void(CurlRequestSchedulerClient*)> completionHandler)
>+void CurlRequestScheduler::finalizeTransfer(CurlRequestSchedulerClient* client, Function<void()> completionHandler)
> {
>-    auto task = [this, handle, completion = WTFMove(completionHandler)]() {
>-        if (!m_activeJobs.contains(handle))
>-            return;
>+    auto locker = holdLock(m_mutex);
>+
>+    if (!m_activeJobs.contains(client))
>+        return;
>+
>+    m_activeJobs.remove(client);
> 
>-        CurlRequestSchedulerClient* client = m_activeJobs.inlineGet(handle);
>+    auto task = [this, client, completionHandler = WTFMove(completionHandler)]() {
>+        if (client->handle()) {
>+            ASSERT(m_clientMaps.contains(client->handle()));
>+            m_clientMaps.remove(client->handle());
>+            m_curlMultiHandle->removeHandle(client->handle());
>+        }
> 
>-        m_curlMultiHandle->removeHandle(handle);
>-        m_activeJobs.remove(handle);
>-        completion(client);
>+        completionHandler();
> 
>         callOnMainThread([client]() {
>             client->release();
>         });
>     };
> 
>-    LockHolder locker(m_mutex);
>     m_taskQueue.append(WTFMove(task));
> }
> 
>diff --git a/Source/WebCore/platform/network/curl/CurlRequestScheduler.h b/Source/WebCore/platform/network/curl/CurlRequestScheduler.h
>index 264b90e106b930bd3b40c2a69050f05bdf99fa0e..ca3bba813440bff6e80312640976cc388a74864f 100644
>--- a/Source/WebCore/platform/network/curl/CurlRequestScheduler.h
>+++ b/Source/WebCore/platform/network/curl/CurlRequestScheduler.h
>@@ -59,16 +59,17 @@ private:
>     void workerThread();
> 
>     void startTransfer(CurlRequestSchedulerClient*);
>-    void completeTransfer(CURL*, CURLcode);
>-    void cancelTransfer(CURL*);
>-    void finalizeTransfer(CURL*, Function<void(CurlRequestSchedulerClient*)>);
>+    void completeTransfer(CurlRequestSchedulerClient*, CURLcode);
>+    void cancelTransfer(CurlRequestSchedulerClient*);
>+    void finalizeTransfer(CurlRequestSchedulerClient*, Function<void()>);
> 
>-    mutable Lock m_mutex;
>+    Lock m_mutex;
>     RefPtr<Thread> m_thread;
>     bool m_runThread { false };
> 
>     Vector<Function<void()>> m_taskQueue;
>-    HashMap<CURL*, CurlRequestSchedulerClient*> m_activeJobs;
>+    HashSet<CurlRequestSchedulerClient*> m_activeJobs;
>+    HashMap<CURL*, CurlRequestSchedulerClient*> m_clientMaps;
> 
>     std::unique_ptr<CurlMultiHandle> m_curlMultiHandle;
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 2d5c8ae29f9dc4c7e0ef363ddd1bd2975f8ecc58..90513d2b21bde588ea8ccdf63009bbae2c84c39a 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,13 @@
>+2019-04-25  Takashi Komori  <Takashi.Komori@sony.com>
>+
>+        [Curl] Fix Curl Request Scheduler not to release wrong Curl handle when request is cancelled.
>+        https://bugs.webkit.org/show_bug.cgi?id=191650
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * http/tests/misc/repeat-open-cancel-expected.txt: Added.
>+        * http/tests/misc/repeat-open-cancel.html: Added.
>+
> 2019-04-09  John Wilander  <wilander@apple.com>
> 
>         Add Resource Load Statistics test case for Ping
>diff --git a/LayoutTests/http/tests/misc/repeat-open-cancel-expected.txt b/LayoutTests/http/tests/misc/repeat-open-cancel-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..83c081ef3a681328fc6f39d6120146e5c6d3174e
>--- /dev/null
>+++ b/LayoutTests/http/tests/misc/repeat-open-cancel-expected.txt
>@@ -0,0 +1,5 @@
>+This is a test for https://bugs.webkit.org/show_bug.cgi?id=191650.
>+
>+In case that several requests are closed and created pretty fast, a result of old request sometimes goes to a wrong request.
>+
>+PASS
>diff --git a/LayoutTests/http/tests/misc/repeat-open-cancel.html b/LayoutTests/http/tests/misc/repeat-open-cancel.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..0ac161a1b0340e6df8f6cbd9b129c0ef96e32fc6
>--- /dev/null
>+++ b/LayoutTests/http/tests/misc/repeat-open-cancel.html
>@@ -0,0 +1,52 @@
>+<html>
>+<head>
>+<script>
>+var repeatCount = 0;
>+
>+function sendRequest() {
>+    var firstReq = new XMLHttpRequest();
>+    firstReq.open("GET", "/resources/download-json-with-delay.php?iteration=100&delay=1");
>+
>+    firstReq.onreadystatechange = function() {
>+        if (firstReq.readyState == firstReq.HEADERS_RECEIVED) {
>+            var secondReq = new XMLHttpRequest();
>+            secondReq.open("GET", "/resources/download-json-with-delay.php?iteration=10&delay=0");
>+
>+            secondReq.onload = function() {
>+                if (repeatCount++ < 40)
>+                    sendRequest();
>+                else {
>+                    document.getElementById('result').innerText = "PASS";
>+                    testRunner.notifyDone();
>+                }
>+            }
>+
>+            secondReq.onerror = function() {
>+                document.getElementById('result').innerText = "FAIL";
>+                testRunner.notifyDone();
>+            }
>+
>+            firstReq.abort();
>+            secondReq.send();
>+        }
>+    }
>+
>+    firstReq.send();
>+}
>+
>+function runTest() {
>+    if (window.testRunner) {
>+        testRunner.waitUntilDone();
>+        testRunner.dumpAsText();
>+    }
>+
>+    sendRequest();
>+}
>+</script>
>+</head>
>+<body onload="runTest()">
>+<p>This is a test for https://bugs.webkit.org/show_bug.cgi?id=191650.</p>
>+<p>In case that several requests are closed and created pretty fast, a result of old request sometimes goes to a wrong request.</p>
>+<p id="result">RUNNING</p>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 191650: 367302 | 367791 | 367937 | 368019 | 368298