Attachment 340683 Details for Bug 183089 – Fixed

[patch] Fixed

183089.diff (text/plain), 14.06 KB, created by Basuke Suzuki on 2018-05-17 23:45:42 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Basuke Suzuki

Created: 2018-05-17 23:45:42 PDT

Size: 14.06 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 52440efd26c..529bdab3a02 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,14 @@
>+2018-05-16  Basuke Suzuki  <Basuke.Suzuki@sony.com>
>+
>+        [Curl] Bug fix on suspend/resume behavior.
>+        https://bugs.webkit.org/show_bug.cgi?id=183089
>+
>+        The flag was not set correctly. Also wrong method was called.
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * platform/wincairo/TestExpectations: Enable loader/ tests for WinCairo.
>+
> 2018-05-15  Charles Vazac  <cvazac@gmail.com>
> 
>         Add the PerformanceServerTiming Interface which makes Server-Timing header timing values available to JavaScript running in the browser.
>diff --git a/LayoutTests/platform/wincairo/TestExpectations b/LayoutTests/platform/wincairo/TestExpectations
>index 93b69bb7a85..079ef3aa80e 100644
>--- a/LayoutTests/platform/wincairo/TestExpectations
>+++ b/LayoutTests/platform/wincairo/TestExpectations
>@@ -1549,7 +1549,9 @@ legacy-animation-engine/transitions [ Skip ]
> legacy-animation-engine/fast/layers [ Skip ]
> legacy-animation-engine/fast/shadow-dom [ Skip ]
> legacy-animation-engine/fullscreen [ Skip ]
>-loader [ Skip ]
>+loader/reload-subresource-when-type-changes.html [ Skip ]
>+loader/stateobjects/pushstate-size-iframe.html [ Skip ]
>+loader/stateobjects/pushstate-size.html [ Skip ]
> mathml [ Skip ]
> media [ Skip ]
> pageoverlay [ Skip ]
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index ecfcf7245e6..950b5e3bc8e 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,35 @@
>+2018-05-16  Basuke Suzuki  <Basuke.Suzuki@sony.com>
>+
>+        [Curl] Bug fix on suspend/resume behavior.
>+        https://bugs.webkit.org/show_bug.cgi?id=183089
>+
>+        The flag was not set correctly. Also wrong method was called.
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Enable loader tests to cover this case.
>+
>+        * platform/network/curl/CurlRequest.cpp:
>+        (WebCore::CurlRequest::cancel): Remove unnecessary cleanup. Use runXXX method.
>+        (WebCore::CurlRequest::suspend): Added cancel check.
>+        (WebCore::CurlRequest::resume): Ditto.
>+        (WebCore::CurlRequest::callClient): Use runXXX method. Change to move semantics.
>+        (WebCore::runOnMainThread): Added.
>+        (WebCore::CurlRequest::runOnWorkerThreadIfRequired): Added.
>+        (WebCore::CurlRequest::setupTransfer): Bug fix. Call setRequestPaused directly.
>+        (WebCore::CurlRequest::didReceiveData): Add state flag update.
>+        (WebCore::CurlRequest::invokeDidReceiveResponseForFile): Use runXXX to simplify.
>+        (WebCore::CurlRequest::completeDidReceiveResponse): Ditto.
>+        (WebCore::CurlRequest::setRequestPaused): Protect state change by mutex.
>+        (WebCore::CurlRequest::setCallbackPaused): Ditto.
>+        (WebCore::CurlRequest::invokeCancel): Added.
>+        (WebCore::CurlRequest::pausedStatusChanged): Use runXXX to simplify.
>+        (WebCore::CurlRequest::updateHandlePauseState): Accessor for m_isHandlePaused.
>+        (WebCore::CurlRequest::isHandlePaused const): Ditto.
>+        * platform/network/curl/CurlRequest.h: Add mutex and paused state.
>+        (WebCore::CurlRequest::shouldBePaused const): Rename from isPaused.
>+        (WebCore::CurlRequest::isPaused const): Deleted.
>+
> 2018-05-15  Charles Vazac  <cvazac@gmail.com>
> 
>         Add the PerformanceServerTiming Interface which makes Server-Timing header timing values available to JavaScript running in the browser.
>diff --git a/Source/WebCore/platform/network/curl/CurlRequest.cpp b/Source/WebCore/platform/network/curl/CurlRequest.cpp
>index ad1b9a713e4..2aa48f16179 100644
>--- a/Source/WebCore/platform/network/curl/CurlRequest.cpp
>+++ b/Source/WebCore/platform/network/curl/CurlRequest.cpp
>@@ -38,6 +38,8 @@
> 
> namespace WebCore {
> 
>+static void runOnMainThread(Function<void()>&& task);
>+
> CurlRequest::CurlRequest(const ResourceRequest&request, CurlRequestClient* client, bool shouldSuspend, bool enableMultipart)
>     : m_request(request.isolatedCopy())
>     , m_client(client)
>@@ -113,8 +115,8 @@ void CurlRequest::cancel()
>         auto& scheduler = CurlContext::singleton().scheduler();
> 
>         if (needToInvokeDidCancelTransfer()) {
>-            scheduler.callOnWorkerThread([protectedThis = makeRef(*this)]() {
>-                protectedThis->didCancelTransfer();
>+            runOnWorkerThreadIfRequired([this, protectedThis = makeRef(*this)]() {
>+                didCancelTransfer();
>             });
>         } else
>             scheduler.cancel(this);
>@@ -123,8 +125,6 @@ void CurlRequest::cancel()
>             didCancelTransfer();
>     }
> 
>-    setRequestPaused(false);
>-    setCallbackPaused(false);
>     invalidateClient();
> }
> 
>@@ -132,6 +132,9 @@ void CurlRequest::suspend()
> {
>     ASSERT(isMainThread());
> 
>+    if (isCompletedOrCancelled())
>+        return;
>+
>     setRequestPaused(true);
> }
> 
>@@ -139,25 +142,36 @@ void CurlRequest::resume()
> {
>     ASSERT(isMainThread());
> 
>+    if (isCompletedOrCancelled())
>+        return;
>+
>     setRequestPaused(false);
> }
> 
> /* `this` is protected inside this method. */
>-void CurlRequest::callClient(WTF::Function<void(CurlRequest&, CurlRequestClient&)> task)
>+void CurlRequest::callClient(Function<void(CurlRequest&, CurlRequestClient&)>&& task)
> {
>-    if (isMainThread()) {
>+    runOnMainThread([this, protectedThis = makeRef(*this), task = WTFMove(task)]() mutable {
>         if (CurlRequestClient* client = m_client) {
>-            RefPtr<CurlRequestClient> protectedClient(client);
>-            task(*this, *client);
>+            task(*this, makeRef(*client));
>         }
>-    } else {
>-        callOnMainThread([this, protectedThis = makeRef(*this), task = WTFMove(task)]() mutable {
>-            if (CurlRequestClient* client = protectedThis->m_client) {
>-                RefPtr<CurlRequestClient> protectedClient(client);
>-                task(*this, *client);
>-            }
>-        });
>-    }
>+    });
>+}
>+
>+static void runOnMainThread(Function<void()>&& task)
>+{
>+    if (isMainThread())
>+        task();
>+    else
>+        callOnMainThread(WTFMove(task));
>+}
>+
>+void CurlRequest::runOnWorkerThreadIfRequired(Function<void()>&& task)
>+{
>+    if (isMainThread() && !m_isSyncRequest)
>+        CurlContext::singleton().scheduler().callOnWorkerThread(WTFMove(task));
>+    else
>+        task();
> }
> 
> CURL* CurlRequest::setupTransfer()
>@@ -228,7 +242,7 @@ CURL* CurlRequest::setupTransfer()
>     m_curlHandle->setCACertPath(sslHandle.getCACertPath().utf8().data());
> 
>     if (m_shouldSuspend)
>-        suspend();
>+        setRequestPaused(true);
> 
> #ifndef NDEBUG
>     m_curlHandle->enableVerboseIfUsed();
>@@ -358,6 +372,9 @@ size_t CurlRequest::didReceiveData(Ref<SharedBuffer>&& buffer)
>             // For asynchronous, pause until completeDidReceiveResponse() is called.
>             setCallbackPaused(true);
>             invokeDidReceiveResponse(m_response, Action::ReceiveData);
>+            // Because libcurl pauses the handle after returning this CURL_WRITEFUNC_PAUSE,
>+            // we need to update its state here.
>+            updateHandlePauseState(true);
>             return CURL_WRITEFUNC_PAUSE;
>         }
> 
>@@ -537,8 +554,8 @@ void CurlRequest::invokeDidReceiveResponseForFile(URL& url)
> 
>     if (!m_isSyncRequest) {
>         // DidReceiveResponse must not be called immediately
>-        CurlContext::singleton().scheduler().callOnWorkerThread([protectedThis = makeRef(*this)]() {
>-            protectedThis->invokeDidReceiveResponse(protectedThis->m_response, Action::StartTransfer);
>+        runOnWorkerThreadIfRequired([this, protectedThis = makeRef(*this)]() {
>+            invokeDidReceiveResponse(m_response, Action::StartTransfer);
>         });
>     } else {
>         // For synchronous, completeDidReceiveResponse() is called in platformContinueSynchronousDidReceiveResponse().
>@@ -580,8 +597,8 @@ void CurlRequest::completeDidReceiveResponse()
>         startWithJobManager();
>     } else if (m_actionAfterInvoke == Action::FinishTransfer) {
>         if (!m_isSyncRequest) {
>-            CurlContext::singleton().scheduler().callOnWorkerThread([protectedThis = makeRef(*this), finishedResultCode = m_finishedResultCode]() {
>-                protectedThis->didCompleteTransfer(finishedResultCode);
>+            runOnWorkerThreadIfRequired([this, protectedThis = makeRef(*this), finishedResultCode = m_finishedResultCode]() {
>+                didCompleteTransfer(finishedResultCode);
>             });
>         } else
>             didCompleteTransfer(m_finishedResultCode);
>@@ -590,55 +607,81 @@ void CurlRequest::completeDidReceiveResponse()
> 
> void CurlRequest::setRequestPaused(bool paused)
> {
>-    auto wasPaused = isPaused();
>-
>-    m_isPausedOfRequest = paused;
>+    {
>+        LockHolder lock(m_pauseStateMutex);
> 
>-    if (isPaused() == wasPaused)
>-        return;
>+        auto savedState = shouldBePaused();
>+        m_shouldSuspend = m_isPausedOfRequest = paused;
>+        if (shouldBePaused() == savedState)
>+            return;
>+    }
> 
>     pausedStatusChanged();
> }
> 
> void CurlRequest::setCallbackPaused(bool paused)
> {
>-    auto wasPaused = isPaused();
>-
>-    m_isPausedOfCallback = paused;
>+    {
>+        LockHolder lock(m_pauseStateMutex);
> 
>-    if (isPaused() == wasPaused)
>-        return;
>+        auto savedState = shouldBePaused();
>+        m_isPausedOfCallback = paused;
> 
>-    // In this case, PAUSE will be executed within didReceiveData(). Change pause state and return.
>-    if (paused)
>-        return;
>+        // If pause is requested, it is called within didReceiveData() which means
>+        // actual change happens inside libcurl. No need to update manually here.
>+        if (shouldBePaused() == savedState || paused)
>+            return;
>+    }
> 
>     pausedStatusChanged();
> }
> 
>+void CurlRequest::invokeCancel()
>+{
>+    // There's no need to extract this method. This is a workaround for MSVC's bug
>+    // which happens when using lambda inside other lambda. The compiler loses context
>+    // of `this` which prevent makeRef.
>+    runOnMainThread([this, protectedThis = makeRef(*this)]() {
>+        cancel();
>+    });
>+}
>+
> void CurlRequest::pausedStatusChanged()
> {
>-    if (isCompletedOrCancelled())
>-        return;
>+    runOnWorkerThreadIfRequired([this, protectedThis = makeRef(*this)]() {
>+        if (isCompletedOrCancelled())
>+            return;
>+
>+        bool needCancel { false };
>+        {
>+            LockHolder lock(m_pauseStateMutex);
>+            bool paused = shouldBePaused();
> 
>-    if (!m_isSyncRequest && isMainThread()) {
>-        CurlContext::singleton().scheduler().callOnWorkerThread([protectedThis = makeRef(*this), paused = isPaused()]() {
>-            if (protectedThis->isCompletedOrCancelled())
>+            if (isHandlePaused() == paused)
>                 return;
> 
>-            auto error = protectedThis->m_curlHandle->pause(paused ? CURLPAUSE_ALL : CURLPAUSE_CONT);
>-            if ((error != CURLE_OK) && !paused) {
>-                // Restarting the handle has failed so just cancel it.
>-                callOnMainThread([protectedThis = makeRef(protectedThis.get())]() {
>-                    protectedThis->cancel();
>-                });
>-            }
>-        });
>-    } else {
>-        auto error = m_curlHandle->pause(isPaused() ? CURLPAUSE_ALL : CURLPAUSE_CONT);
>-        if ((error != CURLE_OK) && !isPaused())
>-            cancel();
>-    }
>+            auto error = m_curlHandle->pause(paused ? CURLPAUSE_ALL : CURLPAUSE_CONT);
>+            if (error == CURLE_OK)
>+                updateHandlePauseState(paused);
>+
>+            needCancel = (error != CURLE_OK && paused);
>+        }
>+
>+        if (needCancel)
>+            invokeCancel();
>+    });
>+}
>+
>+void CurlRequest::updateHandlePauseState(bool paused)
>+{
>+    ASSERT(!isMainThread() || m_isSyncRequest);
>+    m_isHandlePaused = paused;
>+}
>+
>+bool CurlRequest::isHandlePaused() const
>+{
>+    ASSERT(!isMainThread() || m_isSyncRequest);
>+    return m_isHandlePaused;
> }
> 
> void CurlRequest::enableDownloadToFile()
>diff --git a/Source/WebCore/platform/network/curl/CurlRequest.h b/Source/WebCore/platform/network/curl/CurlRequest.h
>index 92c451e1ee4..ff5e09df5c4 100644
>--- a/Source/WebCore/platform/network/curl/CurlRequest.h
>+++ b/Source/WebCore/platform/network/curl/CurlRequest.h
>@@ -105,7 +105,8 @@ private:
> 
>     void startWithJobManager();
> 
>-    void callClient(WTF::Function<void(CurlRequest&, CurlRequestClient&)>);
>+    void callClient(Function<void(CurlRequest&, CurlRequestClient&)>&&);
>+    void runOnWorkerThreadIfRequired(Function<void()>&&);
> 
>     // Transfer processing of Request body, Response header/body
>     // Called by worker thread in case of async, main thread in case of sync.
>@@ -119,6 +120,7 @@ private:
>     void didCompleteTransfer(CURLcode) override;
>     void didCancelTransfer() override;
>     void finalizeTransfer();
>+    void invokeCancel();
> 
>     // For setup 
>     void appendAcceptLanguageHeader(HTTPHeaderMap&);
>@@ -134,7 +136,7 @@ private:
>     void setRequestPaused(bool);
>     void setCallbackPaused(bool);
>     void pausedStatusChanged();
>-    bool isPaused() const { return m_isPausedOfRequest || m_isPausedOfCallback; };
>+    bool shouldBePaused() const { return m_isPausedOfRequest || m_isPausedOfCallback; };
> 
>     // Download
>     void writeDataToDownloadFileIfEnabled(const SharedBuffer&);
>@@ -173,6 +175,17 @@ private:
> 
>     bool m_isPausedOfRequest { false };
>     bool m_isPausedOfCallback { false };
>+    Lock m_pauseStateMutex;
>+    // Following `m_isHandlePaused` is actual paused state of CurlHandle. It's required because pause
>+    // request coming from main thread has a time lag until it invokes and receive callback can
>+    // change the state by returning a special value. So that is must be managed by this flag.
>+    // Unfortunately libcurl doesn't have an interface to check the state.
>+    // There's also no need to protect this flag by the mutex because it is and MUST BE accessed only
>+    // within worker thread. The access must be using accessor to detect irregular usage.
>+    // [TODO] When libcurl is updated to fetch paused state, remove this state variable.
>+    bool m_isHandlePaused { false };
>+    void updateHandlePauseState(bool);
>+    bool isHandlePaused() const;
> 
>     Lock m_downloadMutex;
>     bool m_isEnabledDownloadToFile { false };

Flags:

youennf: review+
ews-watchlist: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 183089: 334541 | 339887 | 340529 | 340530 | 340621 | 340625 | 340674 | 340683 | 340691 | 340713