Changeset 231968 in webkit


Ignore:
Timestamp:
May 18, 2018 11:28:37 AM (6 years ago)
Author:
commit-queue@webkit.org
Message:

[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.

Patch by Basuke Suzuki <Basuke Suzuki> on 2018-05-18
Reviewed by Youenn Fablet.

Source/WebCore:

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.

LayoutTests:

  • platform/wincairo/TestExpectations: Enable loader/ tests for WinCairo.
Location:
trunk
Files:
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r231967 r231968  
     12018-05-18  Basuke Suzuki  <Basuke.Suzuki@sony.com>
     2
     3        [Curl] Bug fix on suspend/resume behavior.
     4        https://bugs.webkit.org/show_bug.cgi?id=183089
     5
     6        The flag was not set correctly. Also wrong method was called.
     7
     8        Reviewed by Youenn Fablet.
     9
     10        * platform/wincairo/TestExpectations: Enable loader/ tests for WinCairo.
     11
    1122018-05-18  Wenson Hsieh  <wenson_hsieh@apple.com>
    213

  • trunk/LayoutTests/platform/wincairo/TestExpectations

    r231890 r231968  
    15621562legacy-animation-engine/fast/shadow-dom [ Skip ]
    15631563legacy-animation-engine/fullscreen [ Skip ]
    1564 loader [ Skip ]
     1564loader/reload-subresource-when-type-changes.html [ Skip ]
     1565loader/stateobjects/pushstate-size-iframe.html [ Skip ]
     1566loader/stateobjects/pushstate-size.html [ Skip ]
    15651567mathml [ Skip ]
    15661568media [ Skip ]

  • trunk/Source/WebCore/ChangeLog

    r231963 r231968  
     12018-05-18  Basuke Suzuki  <Basuke.Suzuki@sony.com>
     2
     3        [Curl] Bug fix on suspend/resume behavior.
     4        https://bugs.webkit.org/show_bug.cgi?id=183089
     5
     6        The flag was not set correctly. Also wrong method was called.
     7
     8        Reviewed by Youenn Fablet.
     9
     10        Enable loader tests to cover this case.
     11
     12        * platform/network/curl/CurlRequest.cpp:
     13        (WebCore::CurlRequest::cancel): Remove unnecessary cleanup. Use runXXX method.
     14        (WebCore::CurlRequest::suspend): Added cancel check.
     15        (WebCore::CurlRequest::resume): Ditto.
     16        (WebCore::CurlRequest::callClient): Use runXXX method. Change to move semantics.
     17        (WebCore::runOnMainThread): Added.
     18        (WebCore::CurlRequest::runOnWorkerThreadIfRequired): Added.
     19        (WebCore::CurlRequest::setupTransfer): Bug fix. Call setRequestPaused directly.
     20        (WebCore::CurlRequest::didReceiveData): Add state flag update.
     21        (WebCore::CurlRequest::invokeDidReceiveResponseForFile): Use runXXX to simplify.
     22        (WebCore::CurlRequest::completeDidReceiveResponse): Ditto.
     23        (WebCore::CurlRequest::setRequestPaused): Protect state change by mutex.
     24        (WebCore::CurlRequest::setCallbackPaused): Ditto.
     25        (WebCore::CurlRequest::invokeCancel): Added.
     26        (WebCore::CurlRequest::pausedStatusChanged): Use runXXX to simplify.
     27        (WebCore::CurlRequest::updateHandlePauseState): Accessor for m_isHandlePaused.
     28        (WebCore::CurlRequest::isHandlePaused const): Ditto.
     29        * platform/network/curl/CurlRequest.h: Add mutex and paused state.
     30        (WebCore::CurlRequest::shouldBePaused const): Rename from isPaused.
     31        (WebCore::CurlRequest::isPaused const): Deleted.
     32
    1332018-05-18  Chris Dumez  <cdumez@apple.com>
    234

  • trunk/Source/WebCore/platform/network/curl/CurlRequest.cpp

    r231696 r231968  
    3939namespace WebCore {
    4040
     41static void runOnMainThread(Function<void()>&& task);
     42
    4143CurlRequest::CurlRequest(const ResourceRequest&request, CurlRequestClient* client, bool shouldSuspend, bool enableMultipart)
    4244    : m_request(request.isolatedCopy())
    … …  
    114116
    115117        if (needToInvokeDidCancelTransfer()) {
    116             scheduler.callOnWorkerThread([protectedThis = makeRef(*this)]() {
    117                 protectedThis->didCancelTransfer();
     118            runOnWorkerThreadIfRequired([this, protectedThis = makeRef(*this)]() {
     119                didCancelTransfer();
    118120            });
    119121        } else
    … …  
    124126    }
    125127
     128    invalidateClient();
     129}
     130
     131void CurlRequest::suspend()
     132{
     133    ASSERT(isMainThread());
     134
     135    if (isCompletedOrCancelled())
     136        return;
     137
     138    setRequestPaused(true);
     139}
     140
     141void CurlRequest::resume()
     142{
     143    ASSERT(isMainThread());
     144
     145    if (isCompletedOrCancelled())
     146        return;
     147
    126148    setRequestPaused(false);
    127     setCallbackPaused(false);
    128     invalidateClient();
    129 }
    130 
    131 void CurlRequest::suspend()
    132 {
    133     ASSERT(isMainThread());
    134 
    135     setRequestPaused(true);
    136 }
    137 
    138 void CurlRequest::resume()
    139 {
    140     ASSERT(isMainThread());
    141 
    142     setRequestPaused(false);
    143149}
    144150
    145151/* `this` is protected inside this method. */
    146 void CurlRequest::callClient(WTF::Function<void(CurlRequest&, CurlRequestClient&)> task)
    147 {
    148     if (isMainThread()) {
     152void CurlRequest::callClient(Function<void(CurlRequest&, CurlRequestClient&)>&& task)
     153{
     154    runOnMainThread([this, protectedThis = makeRef(*this), task = WTFMove(task)]() mutable {
    149155        if (CurlRequestClient* client = m_client) {
    150             RefPtr<CurlRequestClient> protectedClient(client);
    151             task(*this, *client);
     156            task(*this, makeRef(*client));
    152157        }
    153     } else {
    154         callOnMainThread([this, protectedThis = makeRef(*this), task = WTFMove(task)]() mutable {
    155             if (CurlRequestClient* client = protectedThis->m_client) {
    156                 RefPtr<CurlRequestClient> protectedClient(client);
    157                 task(*this, *client);
    158             }
    159         });
    160     }
     158    });
     159}
     160
     161static void runOnMainThread(Function<void()>&& task)
     162{
     163    if (isMainThread())
     164        task();
     165    else
     166        callOnMainThread(WTFMove(task));
     167}
     168
     169void CurlRequest::runOnWorkerThreadIfRequired(Function<void()>&& task)
     170{
     171    if (isMainThread() && !m_isSyncRequest)
     172        CurlContext::singleton().scheduler().callOnWorkerThread(WTFMove(task));
     173    else
     174        task();
    161175}
    162176
    … …  
    229243
    230244    if (m_shouldSuspend)
    231         suspend();
     245        setRequestPaused(true);
    232246
    233247#ifndef NDEBUG
    … …  
    359373            setCallbackPaused(true);
    360374            invokeDidReceiveResponse(m_response, Action::ReceiveData);
     375            // Because libcurl pauses the handle after returning this CURL_WRITEFUNC_PAUSE,
     376            // we need to update its state here.
     377            updateHandlePauseState(true);
    361378            return CURL_WRITEFUNC_PAUSE;
    362379        }
    … …  
    538555    if (!m_isSyncRequest) {
    539556        // DidReceiveResponse must not be called immediately
    540         CurlContext::singleton().scheduler().callOnWorkerThread([protectedThis = makeRef(*this)]() {
    541             protectedThis->invokeDidReceiveResponse(protectedThis->m_response, Action::StartTransfer);
     557        runOnWorkerThreadIfRequired([this, protectedThis = makeRef(*this)]() {
     558            invokeDidReceiveResponse(m_response, Action::StartTransfer);
    542559        });
    543560    } else {
    … …  
    581598    } else if (m_actionAfterInvoke == Action::FinishTransfer) {
    582599        if (!m_isSyncRequest) {
    583             CurlContext::singleton().scheduler().callOnWorkerThread([protectedThis = makeRef(*this), finishedResultCode = m_finishedResultCode]() {
    584                 protectedThis->didCompleteTransfer(finishedResultCode);
     600            runOnWorkerThreadIfRequired([this, protectedThis = makeRef(*this), finishedResultCode = m_finishedResultCode]() {
     601                didCompleteTransfer(finishedResultCode);
    585602            });
    586603        } else
    … …  
    591608void CurlRequest::setRequestPaused(bool paused)
    592609{
    593     auto wasPaused = isPaused();
    594 
    595     m_isPausedOfRequest = paused;
    596 
    597     if (isPaused() == wasPaused)
    598         return;
     610    {
     611        LockHolder lock(m_pauseStateMutex);
     612
     613        auto savedState = shouldBePaused();
     614        m_shouldSuspend = m_isPausedOfRequest = paused;
     615        if (shouldBePaused() == savedState)
     616            return;
     617    }
    599618
    600619    pausedStatusChanged();
    … …  
    603622void CurlRequest::setCallbackPaused(bool paused)
    604623{
    605     auto wasPaused = isPaused();
    606 
    607     m_isPausedOfCallback = paused;
    608 
    609     if (isPaused() == wasPaused)
    610         return;
    611 
    612     // In this case, PAUSE will be executed within didReceiveData(). Change pause state and return.
    613     if (paused)
    614         return;
     624    {
     625        LockHolder lock(m_pauseStateMutex);
     626
     627        auto savedState = shouldBePaused();
     628        m_isPausedOfCallback = paused;
     629
     630        // If pause is requested, it is called within didReceiveData() which means
     631        // actual change happens inside libcurl. No need to update manually here.
     632        if (shouldBePaused() == savedState || paused)
     633            return;
     634    }
    615635
    616636    pausedStatusChanged();
    617637}
    618638
     639void CurlRequest::invokeCancel()
     640{
     641    // There's no need to extract this method. This is a workaround for MSVC's bug
     642    // which happens when using lambda inside other lambda. The compiler loses context
     643    // of `this` which prevent makeRef.
     644    runOnMainThread([this, protectedThis = makeRef(*this)]() {
     645        cancel();
     646    });
     647}
     648
    619649void CurlRequest::pausedStatusChanged()
    620650{
    621     if (isCompletedOrCancelled())
    622         return;
    623 
    624     if (!m_isSyncRequest && isMainThread()) {
    625         CurlContext::singleton().scheduler().callOnWorkerThread([protectedThis = makeRef(*this), paused = isPaused()]() {
    626             if (protectedThis->isCompletedOrCancelled())
     651    runOnWorkerThreadIfRequired([this, protectedThis = makeRef(*this)]() {
     652        if (isCompletedOrCancelled())
     653            return;
     654
     655        bool needCancel { false };
     656        {
     657            LockHolder lock(m_pauseStateMutex);
     658            bool paused = shouldBePaused();
     659
     660            if (isHandlePaused() == paused)
    627661                return;
    628662
    629             auto error = protectedThis->m_curlHandle->pause(paused ? CURLPAUSE_ALL : CURLPAUSE_CONT);
    630             if ((error != CURLE_OK) && !paused) {
    631                 // Restarting the handle has failed so just cancel it.
    632                 callOnMainThread([protectedThis = makeRef(protectedThis.get())]() {
    633                     protectedThis->cancel();
    634                 });
    635             }
    636         });
    637     } else {
    638         auto error = m_curlHandle->pause(isPaused() ? CURLPAUSE_ALL : CURLPAUSE_CONT);
    639         if ((error != CURLE_OK) && !isPaused())
    640             cancel();
    641     }
     663            auto error = m_curlHandle->pause(paused ? CURLPAUSE_ALL : CURLPAUSE_CONT);
     664            if (error == CURLE_OK)
     665                updateHandlePauseState(paused);
     666
     667            needCancel = (error != CURLE_OK && paused);
     668        }
     669
     670        if (needCancel)
     671            invokeCancel();
     672    });
     673}
     674
     675void CurlRequest::updateHandlePauseState(bool paused)
     676{
     677    ASSERT(!isMainThread() || m_isSyncRequest);
     678    m_isHandlePaused = paused;
     679}
     680
     681bool CurlRequest::isHandlePaused() const
     682{
     683    ASSERT(!isMainThread() || m_isSyncRequest);
     684    return m_isHandlePaused;
    642685}
    643686

  • trunk/Source/WebCore/platform/network/curl/CurlRequest.h

    r230734 r231968  
    106106    void startWithJobManager();
    107107
    108     void callClient(WTF::Function<void(CurlRequest&, CurlRequestClient&)>);
     108    void callClient(Function<void(CurlRequest&, CurlRequestClient&)>&&);
     109    void runOnWorkerThreadIfRequired(Function<void()>&&);
    109110
    110111    // Transfer processing of Request body, Response header/body
    … …  
    120121    void didCancelTransfer() override;
    121122    void finalizeTransfer();
     123    void invokeCancel();
    122124
    123125    // For setup
    … …  
    135137    void setCallbackPaused(bool);
    136138    void pausedStatusChanged();
    137     bool isPaused() const { return m_isPausedOfRequest || m_isPausedOfCallback; };
     139    bool shouldBePaused() const { return m_isPausedOfRequest || m_isPausedOfCallback; };
     140    void updateHandlePauseState(bool);
     141    bool isHandlePaused() const;
    138142
    139143    // Download
    … …  
    174178    bool m_isPausedOfRequest { false };
    175179    bool m_isPausedOfCallback { false };
     180    Lock m_pauseStateMutex;
     181    // Following `m_isHandlePaused` is actual paused state of CurlHandle. It's required because pause
     182    // request coming from main thread has a time lag until it invokes and receive callback can
     183    // change the state by returning a special value. So that is must be managed by this flag.
     184    // Unfortunately libcurl doesn't have an interface to check the state.
     185    // There's also no need to protect this flag by the mutex because it is and MUST BE accessed only
     186    // within worker thread. The access must be using accessor to detect irregular usage.
     187    // [TODO] When libcurl is updated to fetch paused state, remove this state variable and
     188    // setter/getter above.
     189    bool m_isHandlePaused { false };
    176190
    177191    Lock m_downloadMutex;
Note: See TracChangeset for help on using the changeset viewer.