Attachment 346294 Details for Bug 188206 – PATCH

[patch] PATCH

188206.diff (text/plain), 17.17 KB, created by Basuke Suzuki on 2018-08-01 13:43:26 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Basuke Suzuki

Created: 2018-08-01 13:43:26 PDT

Size: 17.17 KB

patch

obsolete

>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index bfef6390a7b..8305b7fadf8 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,44 @@
>+2018-08-01  Basuke Suzuki  <Basuke.Suzuki@sony.com>
>+
>+        [Curl] Change synchronous request logic using MessageQueue to match with Mac port.
>+        https://bugs.webkit.org/show_bug.cgi?id=188206
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Port synchronous request logic from ResourceHandleMac to use MessageQueue for
>+        client callback invocation. This makes simplify the logic of CurlRequest because
>+        now every requests are handled in Curl thread and there's no difference between
>+        sync and async requests.
>+
>+        Test: Covered by these tests:
>+        - http\tests\xmlhttprequest\simple-sync.html
>+        - http\tests\xmlhttprequest\xmlhttprequest-unsafe-redirect.html
>+
>+        * platform/network/ResourceHandleInternal.h:
>+        * platform/network/curl/CurlRequest.cpp: Remove synchronous request logics.
>+        (WebCore::CurlRequest::CurlRequest):
>+        (WebCore::CurlRequest::invalidateClient):
>+        (WebCore::CurlRequest::start):
>+        (WebCore::CurlRequest::cancel):
>+        (WebCore::CurlRequest::runOnMainThread): Added message queue handling.
>+        (WebCore::CurlRequest::runOnWorkerThreadIfRequired):
>+        (WebCore::CurlRequest::didReceiveData):
>+        (WebCore::CurlRequest::invokeDidReceiveResponseForFile):
>+        (WebCore::CurlRequest::completeDidReceiveResponse):
>+        (WebCore::CurlRequest::updateHandlePauseState):
>+        (WebCore::CurlRequest::isHandlePaused const):
>+        * platform/network/curl/CurlRequest.h:
>+        (WebCore::CurlRequest::create):
>+        (WebCore::CurlRequest::resourceRequest const):
>+        * platform/network/curl/ResourceHandleCurl.cpp:
>+        (WebCore::ResourceHandle::createCurlRequest):
>+        (WebCore::ResourceHandle::restartRequestWithCredential):
>+        (WebCore::ResourceHandle::platformLoadResourceSynchronously):
>+        (WebCore::ResourceHandle::platformContinueSynchronousDidReceiveResponse):
>+        (WebCore::ResourceHandle::continueAfterDidReceiveResponse):
>+        (WebCore::ResourceHandle::continueAfterWillSendRequest):
>+        (WebCore::ResourceHandle::handleDataURL):
>+
> 2018-07-28  Darin Adler  <darin@apple.com>
> 
>         [Cocoa] Update more WebCore Objective-C code to be ARC compatible
>diff --git a/Source/WebCore/platform/network/ResourceHandleInternal.h b/Source/WebCore/platform/network/ResourceHandleInternal.h
>index c66601a15f0..77e26e7317b 100644
>--- a/Source/WebCore/platform/network/ResourceHandleInternal.h
>+++ b/Source/WebCore/platform/network/ResourceHandleInternal.h
>@@ -39,6 +39,7 @@
> 
> #if USE(CURL)
> #include "CurlRequest.h"
>+#include <wtf/MessageQueue.h>
> #endif
> 
> #if PLATFORM(COCOA)
>@@ -123,6 +124,7 @@ public:
>     unsigned m_authFailureCount { 0 };
>     bool m_addedCacheValidationHeaders { false };
>     RefPtr<CurlRequest> m_curlRequest;
>+    MessageQueue<WTF::Function<void()>>* m_messageQueue { };
> #endif
> 
> #if PLATFORM(COCOA)
>diff --git a/Source/WebCore/platform/network/curl/CurlRequest.cpp b/Source/WebCore/platform/network/curl/CurlRequest.cpp
>index fab138ffdca..7e4907620bd 100644
>--- a/Source/WebCore/platform/network/curl/CurlRequest.cpp
>+++ b/Source/WebCore/platform/network/curl/CurlRequest.cpp
>@@ -38,14 +38,13 @@
> 
> namespace WebCore {
> 
>-static void runOnMainThread(Function<void()>&& task);
>-
>-CurlRequest::CurlRequest(const ResourceRequest&request, CurlRequestClient* client, bool shouldSuspend, bool enableMultipart)
>+CurlRequest::CurlRequest(const ResourceRequest&request, CurlRequestClient* client, bool shouldSuspend, bool enableMultipart, MessageQueue<Function<void()>>* messageQueue)
>     : m_request(request.isolatedCopy())
>     , m_client(client)
>     , m_shouldSuspend(shouldSuspend)
>     , m_enableMultipart(enableMultipart)
>     , m_formDataStream(m_request.httpBody())
>+    , m_messageQueue(messageQueue)
> {
>     ASSERT(isMainThread());
> }
>@@ -55,6 +54,7 @@ void CurlRequest::invalidateClient()
>     ASSERT(isMainThread());
> 
>     m_client = nullptr;
>+    m_messageQueue = nullptr;
> }
> 
> void CurlRequest::setUserPass(const String& user, const String& password)
>@@ -65,7 +65,7 @@ void CurlRequest::setUserPass(const String& user, const String& password)
>     m_password = password.isolatedCopy();
> }
> 
>-void CurlRequest::start(bool isSyncRequest)
>+void CurlRequest::start()
> {
>     // The pausing of transfer does not work with protocols, like file://.
>     // Therefore, PAUSE can not be done in didReceiveData().
>@@ -78,28 +78,12 @@ void CurlRequest::start(bool isSyncRequest)
> 
>     ASSERT(isMainThread());
> 
>-    m_isSyncRequest = isSyncRequest;
>-
>     auto url = m_request.url().isolatedCopy();
> 
>-    if (!m_isSyncRequest) {
>-        // For asynchronous, use CurlRequestScheduler. Curl processes runs on sub thread.
>-        if (url.isLocalFile())
>-            invokeDidReceiveResponseForFile(url);
>-        else
>-            startWithJobManager();
>-    } else {
>-        // For synchronous, does not use CurlRequestScheduler. Curl processes runs on main thread.
>-        // curl_easy_perform blocks until the transfer is finished.
>-        retain();
>-        if (url.isLocalFile())
>-            invokeDidReceiveResponseForFile(url);
>-
>-        setupTransfer();
>-        CURLcode resultCode = m_curlHandle->perform();
>-        didCompleteTransfer(resultCode);
>-        release();
>-    }
>+    if (url.isLocalFile())
>+        invokeDidReceiveResponseForFile(url);
>+    else
>+        startWithJobManager();
> }
> 
> void CurlRequest::startWithJobManager()
>@@ -118,19 +102,14 @@ void CurlRequest::cancel()
> 
>     m_cancelled = true;
> 
>-    if (!m_isSyncRequest) {
>-        auto& scheduler = CurlContext::singleton().scheduler();
>+    auto& scheduler = CurlContext::singleton().scheduler();
> 
>-        if (needToInvokeDidCancelTransfer()) {
>-            runOnWorkerThreadIfRequired([this, protectedThis = makeRef(*this)]() {
>-                didCancelTransfer();
>-            });
>-        } else
>-            scheduler.cancel(this);
>-    } else {
>-        if (needToInvokeDidCancelTransfer())
>+    if (needToInvokeDidCancelTransfer()) {
>+        runOnWorkerThreadIfRequired([this, protectedThis = makeRef(*this)]() {
>             didCancelTransfer();
>-    }
>+        });
>+    } else
>+        scheduler.cancel(this);
> 
>     invalidateClient();
> }
>@@ -158,9 +137,11 @@ void CurlRequest::callClient(Function<void(CurlRequest&, CurlRequestClient&)>&&
>     });
> }
> 
>-static void runOnMainThread(Function<void()>&& task)
>+void CurlRequest::runOnMainThread(Function<void()>&& task)
> {
>-    if (isMainThread())
>+    if (m_messageQueue)
>+        m_messageQueue->append(std::make_unique<Function<void()>>(WTFMove(task)));
>+    else if (isMainThread())
>         task();
>     else
>         callOnMainThread(WTFMove(task));
>@@ -168,7 +149,7 @@ static void runOnMainThread(Function<void()>&& task)
> 
> void CurlRequest::runOnWorkerThreadIfRequired(Function<void()>&& task)
> {
>-    if (isMainThread() && !m_isSyncRequest)
>+    if (isMainThread())
>         CurlContext::singleton().scheduler().callOnWorkerThread(WTFMove(task));
>     else
>         task();
>@@ -332,19 +313,13 @@ size_t CurlRequest::didReceiveData(Ref<SharedBuffer>&& buffer)
>         return 0;
> 
>     if (needToInvokeDidReceiveResponse()) {
>-        if (!m_isSyncRequest) {
>-            // 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;
>-        }
>-
>-        // For synchronous, completeDidReceiveResponse() is called in invokeDidReceiveResponse().
>-        // In this case, pause is unnecessary.
>-        invokeDidReceiveResponse(m_response, Action::None);
>+        // 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;
>     }
> 
>     auto receiveBytes = buffer->size();
>@@ -517,15 +492,10 @@ void CurlRequest::invokeDidReceiveResponseForFile(URL& url)
>     // Determine the MIME type based on the path.
>     m_response.headers.append(String("Content-Type: " + MIMETypeRegistry::getMIMETypeForPath(m_response.url.path())));
> 
>-    if (!m_isSyncRequest) {
>-        // DidReceiveResponse must not be called immediately
>-        runOnWorkerThreadIfRequired([this, protectedThis = makeRef(*this)]() {
>-            invokeDidReceiveResponse(m_response, Action::StartTransfer);
>-        });
>-    } else {
>-        // For synchronous, completeDidReceiveResponse() is called in platformContinueSynchronousDidReceiveResponse().
>-        invokeDidReceiveResponse(m_response, Action::None);
>-    }
>+    // DidReceiveResponse must not be called immediately
>+    runOnWorkerThreadIfRequired([this, protectedThis = makeRef(*this)]() {
>+        invokeDidReceiveResponse(m_response, Action::StartTransfer);
>+    });
> }
> 
> void CurlRequest::invokeDidReceiveResponse(const CurlResponse& response, Action behaviorAfterInvoke)
>@@ -561,12 +531,9 @@ void CurlRequest::completeDidReceiveResponse()
>         // Start transfer for file scheme
>         startWithJobManager();
>     } else if (m_actionAfterInvoke == Action::FinishTransfer) {
>-        if (!m_isSyncRequest) {
>-            runOnWorkerThreadIfRequired([this, protectedThis = makeRef(*this), finishedResultCode = m_finishedResultCode]() {
>-                didCompleteTransfer(finishedResultCode);
>-            });
>-        } else
>-            didCompleteTransfer(m_finishedResultCode);
>+        runOnWorkerThreadIfRequired([this, protectedThis = makeRef(*this), finishedResultCode = m_finishedResultCode]() {
>+            didCompleteTransfer(finishedResultCode);
>+        });
>     }
> }
> 
>@@ -642,13 +609,13 @@ void CurlRequest::pausedStatusChanged()
> 
> void CurlRequest::updateHandlePauseState(bool paused)
> {
>-    ASSERT(!isMainThread() || m_isSyncRequest);
>+    ASSERT(!isMainThread());
>     m_isHandlePaused = paused;
> }
> 
> bool CurlRequest::isHandlePaused() const
> {
>-    ASSERT(!isMainThread() || m_isSyncRequest);
>+    ASSERT(!isMainThread());
>     return m_isHandlePaused;
> }
> 
>diff --git a/Source/WebCore/platform/network/curl/CurlRequest.h b/Source/WebCore/platform/network/curl/CurlRequest.h
>index 5d1445566bc..8ef83a7fb49 100644
>--- a/Source/WebCore/platform/network/curl/CurlRequest.h
>+++ b/Source/WebCore/platform/network/curl/CurlRequest.h
>@@ -35,6 +35,7 @@
> #include "FileSystem.h"
> #include "NetworkLoadMetrics.h"
> #include "ResourceRequest.h"
>+#include <wtf/MessageQueue.h>
> #include <wtf/Noncopyable.h>
> 
> namespace WebCore {
>@@ -57,9 +58,9 @@ public:
>         Yes = true
>     };
> 
>-    static Ref<CurlRequest> create(const ResourceRequest& request, CurlRequestClient& client, ShouldSuspend shouldSuspend = ShouldSuspend::No, EnableMultipart enableMultipart = EnableMultipart::No)
>+    static Ref<CurlRequest> create(const ResourceRequest& request, CurlRequestClient& client, ShouldSuspend shouldSuspend = ShouldSuspend::No, EnableMultipart enableMultipart = EnableMultipart::No, MessageQueue<Function<void()>>* messageQueue = nullptr)
>     {
>-        return adoptRef(*new CurlRequest(request, &client, shouldSuspend == ShouldSuspend::Yes, enableMultipart == EnableMultipart::Yes));
>+        return adoptRef(*new CurlRequest(request, &client, shouldSuspend == ShouldSuspend::Yes, enableMultipart == EnableMultipart::Yes, messageQueue));
>     }
> 
>     virtual ~CurlRequest() = default;
>@@ -67,13 +68,12 @@ public:
>     void invalidateClient();
>     WEBCORE_EXPORT void setUserPass(const String&, const String&);
> 
>-    void start(bool isSyncRequest = false);
>+    void start();
>     void cancel();
>     WEBCORE_EXPORT void suspend();
>     WEBCORE_EXPORT void resume();
> 
>     const ResourceRequest& resourceRequest() const { return m_request; }
>-    bool isSyncRequest() const { return m_isSyncRequest; }
>     bool isCompleted() const { return !m_curlHandle; }
>     bool isCancelled() const { return m_cancelled; }
>     bool isCompletedOrCancelled() const { return isCompleted() || isCancelled(); }
>@@ -99,7 +99,7 @@ private:
>         FinishTransfer
>     };
> 
>-    CurlRequest(const ResourceRequest&, CurlRequestClient*, bool shouldSuspend, bool enableMultipart);
>+    CurlRequest(const ResourceRequest&, CurlRequestClient*, bool, bool, MessageQueue<Function<void()>>*);
> 
>     void retain() override { ref(); }
>     void release() override { deref(); }
>@@ -108,6 +108,7 @@ private:
>     void startWithJobManager();
> 
>     void callClient(Function<void(CurlRequest&, CurlRequestClient&)>&&);
>+    void runOnMainThread(Function<void()>&&);
>     void runOnWorkerThreadIfRequired(Function<void()>&&);
> 
>     // Transfer processing of Request body, Response header/body
>@@ -153,10 +154,9 @@ private:
> 
> 
>     CurlRequestClient* m_client { };
>-    bool m_isSyncRequest { false };
>     bool m_cancelled { false };
>+    MessageQueue<Function<void()>>* m_messageQueue { };
> 
>-    // Used by worker thread in case of async, and main thread in case of sync.
>     ResourceRequest m_request;
>     String m_user;
>     String m_password;
>diff --git a/Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp b/Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp
>index 92b7c4e69d1..6e0a6e45be5 100644
>--- a/Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp
>+++ b/Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp
>@@ -149,7 +149,7 @@ Ref<CurlRequest> ResourceHandle::createCurlRequest(ResourceRequest& request, Req
>     }
> 
>     CurlRequest::ShouldSuspend shouldSuspend = d->m_defersLoading ? CurlRequest::ShouldSuspend::Yes : CurlRequest::ShouldSuspend::No;
>-    auto curlRequest = CurlRequest::create(request, *delegate(), shouldSuspend, CurlRequest::EnableMultipart::Yes);
>+    auto curlRequest = CurlRequest::create(request, *delegate(), shouldSuspend, CurlRequest::EnableMultipart::Yes, d->m_messageQueue);
>     
>     return curlRequest;
> }
>@@ -370,16 +370,15 @@ void ResourceHandle::restartRequestWithCredential(const String& user, const Stri
>     if (!d->m_curlRequest)
>         return;
>     
>-    auto wasSyncRequest = d->m_curlRequest->isSyncRequest();
>     auto previousRequest = d->m_curlRequest->resourceRequest();
>     d->m_curlRequest->cancel();
> 
>     d->m_curlRequest = createCurlRequest(previousRequest, RequestStatus::ReusedRequest);
>     d->m_curlRequest->setUserPass(user, password);
>-    d->m_curlRequest->start(wasSyncRequest);
>+    d->m_curlRequest->start();
> }
> 
>-void ResourceHandle::platformLoadResourceSynchronously(NetworkingContext* context, const ResourceRequest& request, StoredCredentialsPolicy, ResourceError& error, ResourceResponse& response, Vector<char>& data)
>+void ResourceHandle::platformLoadResourceSynchronously(NetworkingContext* context, const ResourceRequest& request, StoredCredentialsPolicy storedCredentialsPolicy, ResourceError& error, ResourceResponse& response, Vector<char>& data)
> {
>     ASSERT(isMainThread());
> 
>@@ -389,19 +388,20 @@ void ResourceHandle::platformLoadResourceSynchronously(NetworkingContext* contex
>     bool shouldContentSniff = true;
>     bool shouldContentEncodingSniff = true;
>     RefPtr<ResourceHandle> handle = adoptRef(new ResourceHandle(context, request, &client, defersLoading, shouldContentSniff, shouldContentEncodingSniff));
>+    handle->d->m_messageQueue = &client.messageQueue();
> 
>     if (localRequest.url().protocolIsData()) {
>         handle->handleDataURL();
>         return;
>     }
> 
>-    // If defersLoading is true and we call curl_easy_perform
>-    // on a paused handle, libcURL would do the transfer anyway
>-    // and we would assert so force defersLoading to be false.
>-    handle->d->m_defersLoading = false;
>-
>     handle->d->m_curlRequest = handle->createCurlRequest(localRequest);
>-    handle->d->m_curlRequest->start(true);
>+    handle->d->m_curlRequest->start();
>+
>+    do {
>+        if (auto task = client.messageQueue().waitForMessage())
>+            (*task)();
>+    } while (!client.messageQueue().killed() && !handle->cancelledOrClientless());
> 
>     error = client.error();
>     data.swap(client.mutableData());
>@@ -496,12 +496,15 @@ void ResourceHandle::continueAfterWillSendRequest(ResourceRequest&& request)
>     ASSERT(isMainThread());
> 
>     // willSendRequest might cancel the load.
>-    if (cancelledOrClientless() || !d->m_curlRequest || request.isNull())
>+    if (cancelledOrClientless() || !d->m_curlRequest)
>         return;
> 
>-    auto wasSyncRequest = d->m_curlRequest->isSyncRequest();
>-    d->m_curlRequest->cancel();
>+    if (request.isNull()) {
>+        cancel();
>+        return;
>+    }
> 
>+    d->m_curlRequest->cancel();
>     d->m_curlRequest = createCurlRequest(request);
> 
>     if (protocolHostAndPortAreEqual(request.url(), delegate()->response().url())) {
>@@ -509,7 +512,7 @@ void ResourceHandle::continueAfterWillSendRequest(ResourceRequest&& request)
>             d->m_curlRequest->setUserPass(credential->first, credential->second);
>     }
> 
>-    d->m_curlRequest->start(wasSyncRequest);
>+    d->m_curlRequest->start();
> }
> 
> void ResourceHandle::handleDataURL()

Actions: View | Formatted Diff | Diff

Attachments on bug 188206: 346294