Changeset 244684 in webkit


Ignore:
Timestamp:
Apr 26, 2019 2:52:49 AM (5 years ago)
Author:
commit-queue@webkit.org
Message:

[Curl] Fix Curl Request Scheduler not to release wrong Curl handle when request is cancelled.
https://bugs.webkit.org/show_bug.cgi?id=191650

Patch by Takashi Komori <Takashi.Komori@sony.com> on 2019-04-26
Reviewed by Fujii Hironori.

Source/WebCore:

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:

LayoutTests:

  • http/tests/misc/repeat-open-cancel-expected.txt: Added.
  • http/tests/misc/repeat-open-cancel.html: Added.
Location:
trunk
Files:
2 added
6 edited

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/ChangeLog

    r244682 r244684  
     12019-04-26  Takashi Komori  <Takashi.Komori@sony.com>
     2
     3        [Curl] Fix Curl Request Scheduler not to release wrong Curl handle when request is cancelled.
     4        https://bugs.webkit.org/show_bug.cgi?id=191650
     5
     6        Reviewed by Fujii Hironori.
     7
     8        * http/tests/misc/repeat-open-cancel-expected.txt: Added.
     9        * http/tests/misc/repeat-open-cancel.html: Added.
     10
    1112019-04-25  Myles C. Maxfield  <mmaxfield@apple.com>
    212
  • trunk/Source/WebCore/ChangeLog

    r244683 r244684  
     12019-04-26  Takashi Komori  <Takashi.Komori@sony.com>
     2
     3        [Curl] Fix Curl Request Scheduler not to release wrong Curl handle when request is cancelled.
     4        https://bugs.webkit.org/show_bug.cgi?id=191650
     5
     6        Reviewed by Fujii Hironori.
     7
     8        Test: http/tests/misc/repeat-open-cancel.html
     9
     10        * platform/network/curl/CurlRequest.cpp:
     11        (WebCore::CurlRequest::cancel):
     12        (WebCore::CurlRequest::isCancelled):
     13        (WebCore::CurlRequest::isCompletedOrCancelled):
     14        (WebCore::CurlRequest::didCompleteTransfer):
     15        (WebCore::CurlRequest::completeDidReceiveResponse):
     16        (WebCore::CurlRequest::pausedStatusChanged):
     17        * platform/network/curl/CurlRequest.h:
     18        (WebCore::CurlRequest::isCompleted const): Deleted.
     19        (WebCore::CurlRequest::isCancelled const): Deleted.
     20        (WebCore::CurlRequest::isCompletedOrCancelled const): Deleted.
     21        * platform/network/curl/CurlRequestScheduler.cpp:
     22        (WebCore::CurlRequestScheduler::cancel):
     23        (WebCore::CurlRequestScheduler::callOnWorkerThread):
     24        (WebCore::CurlRequestScheduler::startThreadIfNeeded):
     25        (WebCore::CurlRequestScheduler::stopThreadIfNoMoreJobRunning):
     26        (WebCore::CurlRequestScheduler::stopThread):
     27        (WebCore::CurlRequestScheduler::executeTasks):
     28        (WebCore::CurlRequestScheduler::workerThread):
     29        (WebCore::CurlRequestScheduler::startTransfer):
     30        (WebCore::CurlRequestScheduler::completeTransfer):
     31        (WebCore::CurlRequestScheduler::cancelTransfer):
     32        (WebCore::CurlRequestScheduler::finalizeTransfer):
     33        * platform/network/curl/CurlRequestScheduler.h:
     34
    1352019-04-25  Myles C. Maxfield  <mmaxfield@apple.com>
    236
  • trunk/Source/WebCore/platform/network/curl/CurlRequest.cpp

    r243654 r244684  
    127127    ASSERT(isMainThread());
    128128
    129     if (isCompletedOrCancelled())
    130         return;
    131 
    132     m_cancelled = true;
     129    {
     130        auto locker = holdLock(m_statusMutex);
     131        if (m_cancelled)
     132            return;
     133
     134        m_cancelled = true;
     135    }
    133136
    134137    auto& scheduler = CurlContext::singleton().scheduler();
     
    142145
    143146    invalidateClient();
     147}
     148
     149bool CurlRequest::isCancelled()
     150{
     151    auto locker = holdLock(m_statusMutex);
     152    return m_cancelled;
     153}
     154
     155bool CurlRequest::isCompletedOrCancelled()
     156{
     157    auto locker = holdLock(m_statusMutex);
     158    return m_completed || m_cancelled;
    144159}
    145160
     
    416431void CurlRequest::didCompleteTransfer(CURLcode result)
    417432{
    418     if (m_cancelled) {
    419         m_curlHandle = nullptr;
     433    if (isCancelled()) {
     434        didCancelTransfer();
    420435        return;
    421436    }
     
    456471        });
    457472    }
     473
     474    {
     475        auto locker = holdLock(m_statusMutex);
     476        m_completed = true;
     477    }
    458478}
    459479
     
    568588    ASSERT(!m_didReturnFromNotify || m_multipartHandle);
    569589
    570     if (isCancelled())
    571         return;
    572 
    573     if (m_actionAfterInvoke != Action::StartTransfer && isCompleted())
     590    if (isCompletedOrCancelled())
    574591        return;
    575592
     
    636653
    637654    runOnWorkerThreadIfRequired([this, protectedThis = makeRef(*this)]() {
    638         if (isCompletedOrCancelled())
     655        if (isCompletedOrCancelled() || !m_curlHandle)
    639656            return;
    640657
  • trunk/Source/WebCore/platform/network/curl/CurlRequest.h

    r243654 r244684  
    8484
    8585    const ResourceRequest& resourceRequest() const { return m_request; }
    86     bool isCompleted() const { return !m_curlHandle; }
    87     bool isCancelled() const { return m_cancelled; }
    88     bool isCompletedOrCancelled() const { return isCompleted() || isCancelled(); }
     86    bool isCancelled();
     87    bool isCompletedOrCancelled();
    8988    Seconds timeoutInterval() const;
    9089
     
    167166
    168167    CurlRequestClient* m_client { };
     168    Lock m_statusMutex;
    169169    bool m_cancelled { false };
     170    bool m_completed { false };
    170171    MessageQueue<Function<void()>>* m_messageQueue { };
    171172
  • trunk/Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp

    r229471 r244684  
    5959    ASSERT(isMainThread());
    6060
    61     if (!client || !client->handle())
     61    if (!client)
    6262        return;
    6363
    64     cancelTransfer(client->handle());
     64    cancelTransfer(client);
    6565}
    6666
     
    6868{
    6969    {
    70         LockHolder locker(m_mutex);
     70        auto locker = holdLock(m_mutex);
    7171        m_taskQueue.append(WTFMove(task));
    7272    }
     
    7979    ASSERT(isMainThread());
    8080
    81     LockHolder locker(m_mutex);
    82     if (!m_runThread) {
    83         if (m_thread)
    84             m_thread->waitForCompletion();
    85 
     81    {
     82        auto locker = holdLock(m_mutex);
     83        if (m_runThread)
     84            return;
     85    }
     86
     87    if (m_thread)
     88        m_thread->waitForCompletion();
     89
     90    {
     91        auto locker = holdLock(m_mutex);
    8692        m_runThread = true;
    87         m_thread = Thread::create("curlThread", [this] {
    88             workerThread();
    89             m_runThread = false;
    90         });
    91     }
     93    }
     94
     95    m_thread = Thread::create("curlThread", [this] {
     96        workerThread();
     97
     98        auto locker = holdLock(m_mutex);
     99        m_runThread = false;
     100    });
    92101}
    93102
     
    96105    ASSERT(!isMainThread());
    97106
    98     if (m_activeJobs.size())
     107    auto locker = holdLock(m_mutex);
     108    if (m_activeJobs.size() || m_taskQueue.size())
    99109        return;
    100110
    101     LockHolder locker(m_mutex);
    102     if (m_taskQueue.size())
    103         return;
    104 
    105111    m_runThread = false;
    106112}
     
    108114void CurlRequestScheduler::stopThread()
    109115{
    110     m_runThread = false;
     116    {
     117        auto locker = holdLock(m_mutex);
     118        m_runThread = false;
     119    }
    111120
    112121    if (m_thread) {
     
    123132
    124133    {
    125         LockHolder locker(m_mutex);
     134        auto locker = holdLock(m_mutex);
    126135        taskQueue = WTFMove(m_taskQueue);
    127136    }
     
    140149    m_curlMultiHandle->setMaxHostConnections(m_maxHostConnections);
    141150
    142     while (m_runThread) {
     151    while (true) {
     152        {
     153            auto locker = holdLock(m_mutex);
     154            if (!m_runThread)
     155                break;
     156        }
     157
    143158        executeTasks();
    144159
     
    178193
    179194            ASSERT(msg->msg == CURLMSG_DONE);
    180             completeTransfer(msg->easy_handle, msg->data.result);
     195            if (auto client = m_clientMaps.inlineGet(msg->easy_handle))
     196                completeTransfer(client, msg->data.result);
    181197        }
    182198
     
    193209    auto task = [this, client]() {
    194210        CURL* handle = client->setupTransfer();
    195         if (!handle)
     211        if (!handle) {
     212            completeTransfer(client, CURLE_FAILED_INIT);
    196213            return;
    197 
    198         m_activeJobs.add(handle, client);
     214        }
     215
    199216        m_curlMultiHandle->addHandle(handle);
     217
     218        ASSERT(!m_clientMaps.contains(handle));
     219        m_clientMaps.set(handle, client);
    200220    };
    201221
    202     LockHolder locker(m_mutex);
     222    auto locker = holdLock(m_mutex);
     223    m_activeJobs.add(client);
    203224    m_taskQueue.append(WTFMove(task));
    204225}
    205226
    206 void CurlRequestScheduler::completeTransfer(CURL* handle, CURLcode result)
    207 {
    208     finalizeTransfer(handle, [result](CurlRequestSchedulerClient* client) {
     227void CurlRequestScheduler::completeTransfer(CurlRequestSchedulerClient* client, CURLcode result)
     228{
     229    finalizeTransfer(client, [client, result]() {
    209230        client->didCompleteTransfer(result);
    210231    });
    211232}
    212233
    213 void CurlRequestScheduler::cancelTransfer(CURL* handle)
    214 {
    215     finalizeTransfer(handle, [](CurlRequestSchedulerClient* client) {
     234void CurlRequestScheduler::cancelTransfer(CurlRequestSchedulerClient* client)
     235{
     236    finalizeTransfer(client, [client]() {
    216237        client->didCancelTransfer();
    217238    });
    218239}
    219240
    220 void CurlRequestScheduler::finalizeTransfer(CURL* handle, Function<void(CurlRequestSchedulerClient*)> completionHandler)
    221 {
    222     auto task = [this, handle, completion = WTFMove(completionHandler)]() {
    223         if (!m_activeJobs.contains(handle))
    224             return;
    225 
    226         CurlRequestSchedulerClient* client = m_activeJobs.inlineGet(handle);
    227 
    228         m_curlMultiHandle->removeHandle(handle);
    229         m_activeJobs.remove(handle);
    230         completion(client);
     241void CurlRequestScheduler::finalizeTransfer(CurlRequestSchedulerClient* client, Function<void()> completionHandler)
     242{
     243    auto locker = holdLock(m_mutex);
     244
     245    if (!m_activeJobs.contains(client))
     246        return;
     247
     248    m_activeJobs.remove(client);
     249
     250    auto task = [this, client, completionHandler = WTFMove(completionHandler)]() {
     251        if (client->handle()) {
     252            ASSERT(m_clientMaps.contains(client->handle()));
     253            m_clientMaps.remove(client->handle());
     254            m_curlMultiHandle->removeHandle(client->handle());
     255        }
     256
     257        completionHandler();
    231258
    232259        callOnMainThread([client]() {
     
    235262    };
    236263
    237     LockHolder locker(m_mutex);
    238264    m_taskQueue.append(WTFMove(task));
    239265}
  • trunk/Source/WebCore/platform/network/curl/CurlRequestScheduler.h

    r229471 r244684  
    6060
    6161    void startTransfer(CurlRequestSchedulerClient*);
    62     void completeTransfer(CURL*, CURLcode);
    63     void cancelTransfer(CURL*);
    64     void finalizeTransfer(CURL*, Function<void(CurlRequestSchedulerClient*)>);
     62    void completeTransfer(CurlRequestSchedulerClient*, CURLcode);
     63    void cancelTransfer(CurlRequestSchedulerClient*);
     64    void finalizeTransfer(CurlRequestSchedulerClient*, Function<void()>);
    6565
    66     mutable Lock m_mutex;
     66    Lock m_mutex;
    6767    RefPtr<Thread> m_thread;
    6868    bool m_runThread { false };
    6969
    7070    Vector<Function<void()>> m_taskQueue;
    71     HashMap<CURL*, CurlRequestSchedulerClient*> m_activeJobs;
     71    HashSet<CurlRequestSchedulerClient*> m_activeJobs;
     72    HashMap<CURL*, CurlRequestSchedulerClient*> m_clientMaps;
    7273
    7374    std::unique_ptr<CurlMultiHandle> m_curlMultiHandle;
Note: See TracChangeset for help on using the changeset viewer.