Attachment 340621 Details for Bug 183089 – Add descriptive comment

[patch] Add descriptive comment

183089.diff (text/plain), 12.50 KB, created by Basuke Suzuki on 2018-05-17 12:11:19 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Basuke Suzuki

Created: 2018-05-17 12:11:19 PDT

Size: 12.50 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..ecc243584ac 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,31 @@
>+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::CurlRequest::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.
>+        * platform/network/curl/CurlRequest.h: Add mutex and paused state.
>+
> 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..ecd3da1f022 100644
>--- a/Source/WebCore/platform/network/curl/CurlRequest.cpp
>+++ b/Source/WebCore/platform/network/curl/CurlRequest.cpp
>@@ -113,8 +113,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 +123,6 @@ void CurlRequest::cancel()
>             didCancelTransfer();
>     }
> 
>-    setRequestPaused(false);
>-    setCallbackPaused(false);
>     invalidateClient();
> }
> 
>@@ -132,6 +130,9 @@ void CurlRequest::suspend()
> {
>     ASSERT(isMainThread());
> 
>+    if (isCompletedOrCancelled())
>+        return;
>+
>     setRequestPaused(true);
> }
> 
>@@ -139,25 +140,37 @@ 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(WTF::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);
>         }
>-    } else {
>-        callOnMainThread([this, protectedThis = makeRef(*this), task = WTFMove(task)]() mutable {
>-            if (CurlRequestClient* client = protectedThis->m_client) {
>-                RefPtr<CurlRequestClient> protectedClient(client);
>-                task(*this, *client);
>-            }
>-        });
>-    }
>+    });
>+}
>+
>+void CurlRequest::runOnMainThread(WTF::Function<void()>&& task)
>+{
>+    if (isMainThread())
>+        task();
>+    else
>+        callOnMainThread(WTFMove(task));
>+}
>+
>+void CurlRequest::runOnWorkerThreadIfRequired(WTF::Function<void()>&& task)
>+{
>+    if (isMainThread() && !m_isSyncRequest)
>+        CurlContext::singleton().scheduler().callOnWorkerThread(WTFMove(task));
>+    else
>+        task();
> }
> 
> CURL* CurlRequest::setupTransfer()
>@@ -228,7 +241,7 @@ CURL* CurlRequest::setupTransfer()
>     m_curlHandle->setCACertPath(sslHandle.getCACertPath().utf8().data());
> 
>     if (m_shouldSuspend)
>-        suspend();
>+        setRequestPaused(true);
> 
> #ifndef NDEBUG
>     m_curlHandle->enableVerboseIfUsed();
>@@ -358,6 +371,7 @@ size_t CurlRequest::didReceiveData(Ref<SharedBuffer>&& buffer)
>             // For asynchronous, pause until completeDidReceiveResponse() is called.
>             setCallbackPaused(true);
>             invokeDidReceiveResponse(m_response, Action::ReceiveData);
>+            m_handlePaused = true;
>             return CURL_WRITEFUNC_PAUSE;
>         }
> 
>@@ -537,8 +551,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 +594,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 +604,71 @@ void CurlRequest::completeDidReceiveResponse()
> 
> void CurlRequest::setRequestPaused(bool paused)
> {
>-    auto wasPaused = isPaused();
>-
>-    m_isPausedOfRequest = paused;
>+    {
>+        LockHolder lock(m_pauseStateMutex);
> 
>-    if (isPaused() == wasPaused)
>-        return;
>+        auto wasPaused = isPaused();
>+        m_shouldSuspend = m_isPausedOfRequest = paused;
>+        if (isPaused() == wasPaused)
>+            return;
>+    }
> 
>     pausedStatusChanged();
> }
> 
> void CurlRequest::setCallbackPaused(bool paused)
> {
>-    auto wasPaused = isPaused();
>-
>-    m_isPausedOfCallback = paused;
>+    {
>+        LockHolder lock(m_pauseStateMutex);
> 
>-    if (isPaused() == wasPaused)
>-        return;
>+        auto wasPaused = isPaused();
>+        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 (isPaused() == wasPaused || 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 loose 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 = isPaused();
> 
>-    if (!m_isSyncRequest && isMainThread()) {
>-        CurlContext::singleton().scheduler().callOnWorkerThread([protectedThis = makeRef(*this), paused = isPaused()]() {
>-            if (protectedThis->isCompletedOrCancelled())
>+            if (m_handlePaused == 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)
>+                m_handlePaused = paused;
>+
>+            needCancel = (error != CURLE_OK && paused);
>+        }
>+
>+        if (needCancel)
>+            invokeCancel();
>+    });
> }
> 
> void CurlRequest::enableDownloadToFile()
>diff --git a/Source/WebCore/platform/network/curl/CurlRequest.h b/Source/WebCore/platform/network/curl/CurlRequest.h
>index 92c451e1ee4..6e9b2112612 100644
>--- a/Source/WebCore/platform/network/curl/CurlRequest.h
>+++ b/Source/WebCore/platform/network/curl/CurlRequest.h
>@@ -105,7 +105,9 @@ private:
> 
>     void startWithJobManager();
> 
>-    void callClient(WTF::Function<void(CurlRequest&, CurlRequestClient&)>);
>+    void callClient(WTF::Function<void(CurlRequest&, CurlRequestClient&)>&&);
>+    void runOnMainThread(WTF::Function<void()>&&);
>+    void runOnWorkerThreadIfRequired(WTF::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 +121,7 @@ private:
>     void didCompleteTransfer(CURLcode) override;
>     void didCancelTransfer() override;
>     void finalizeTransfer();
>+    void invokeCancel();
> 
>     // For setup 
>     void appendAcceptLanguageHeader(HTTPHeaderMap&);
>@@ -173,6 +176,14 @@ private:
> 
>     bool m_isPausedOfRequest { false };
>     bool m_isPausedOfCallback { false };
>+    Lock m_pauseStateMutex;
>+    // Following `m_handlePaused` 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.
>+    // This flag also no need to manage by the mutex because it is and MUST BE accessed only
>+    // within worker thread.
>+    bool m_handlePaused { false };
> 
>     Lock m_downloadMutex;
>     bool m_isEnabledDownloadToFile { false };

Actions: View | Formatted Diff | Diff

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