[Curl] Implement missing async method in RecourceHandle and make it actually async
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 2 Oct 2017 23:34:18 +0000 (23:34 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 2 Oct 2017 23:34:18 +0000 (23:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=173964

Patch by Basuke Suzuki <Basuke.Suzuki@sony.com> on 2017-10-02
Reviewed by Alex Christensen.

* platform/network/ResourceHandle.cpp:
(WebCore::ResourceHandle::continueWillSendRequest): Deleted.
(WebCore::ResourceHandle::continueDidReceiveResponse): Deleted.
(WebCore::ResourceHandle::continueCanAuthenticateAgainstProtectionSpace): Deleted.
* platform/network/curl/CurlRequest.cpp:
(WebCore::CurlRequest::start):
(WebCore::CurlRequest::cancel):
(WebCore::CurlRequest::suspend):
(WebCore::CurlRequest::resume):
(WebCore::CurlRequest::didReceiveHeader):
(WebCore::CurlRequest::didReceiveData):
(WebCore::CurlRequest::didCompleteTransfer):
(WebCore::CurlRequest::didCancelTransfer):
(WebCore::CurlRequest::finalizeTransfer):
(WebCore::CurlRequest::invokeDidReceiveResponseForFile):
(WebCore::CurlRequest::invokeDidReceiveResponse):
(WebCore::CurlRequest::completeDidReceiveResponse):
(WebCore::CurlRequest::setRequestPaused):
(WebCore::CurlRequest::setCallbackPaused):
(WebCore::CurlRequest::pausedStatusChanged):
(WebCore::CurlRequest::setPaused): Deleted.
* platform/network/curl/CurlRequest.h:
(WebCore::CurlRequest::needToInvokeDidReceiveResponse const):
(WebCore::CurlRequest::isPaused const):
* platform/network/curl/ResourceHandleCurl.cpp:
(WebCore::ResourceHandle::receivedRequestToContinueWithoutCredential):
(WebCore::ResourceHandle::continueDidReceiveResponse):
(WebCore::ResourceHandle::platformContinueSynchronousDidReceiveResponse):
* platform/network/curl/ResourceHandleCurlDelegate.cpp:
(WebCore::ResourceHandleCurlDelegate::curlDidReceiveResponse):
(WebCore::ResourceHandleCurlDelegate::continueDidReceiveResponse):
(WebCore::ResourceHandleCurlDelegate::platformContinueSynchronousDidReceiveResponse):
(WebCore::ResourceHandleCurlDelegate::continueAfterDidReceiveResponse):
(WebCore::ResourceHandleCurlDelegate::shouldRedirectAsGET):
* platform/network/curl/ResourceHandleCurlDelegate.h:
* platform/network/curl/ResourceResponseCurl.cpp:
(WebCore::ResourceResponse::shouldRedirect):
(WebCore::ResourceResponse::isMovedPermanently const):
(WebCore::ResourceResponse::isFound const):
(WebCore::ResourceResponse::isSeeOther const):
(WebCore::ResourceResponse::isNotModified const):
(WebCore::ResourceResponse::isUnauthorized const):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@222756 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/platform/network/ResourceHandle.cpp
Source/WebCore/platform/network/curl/CurlRequest.cpp
Source/WebCore/platform/network/curl/CurlRequest.h
Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp
Source/WebCore/platform/network/curl/ResourceHandleCurlDelegate.cpp
Source/WebCore/platform/network/curl/ResourceHandleCurlDelegate.h
Source/WebCore/platform/network/curl/ResourceResponseCurl.cpp

index ce4c6e9..7e359e8 100644 (file)
@@ -1,3 +1,53 @@
+2017-10-02  Basuke Suzuki  <Basuke.Suzuki@sony.com>
+
+        [Curl] Implement missing async method in RecourceHandle and make it actually async
+        https://bugs.webkit.org/show_bug.cgi?id=173964
+
+        Reviewed by Alex Christensen.
+
+        * platform/network/ResourceHandle.cpp:
+        (WebCore::ResourceHandle::continueWillSendRequest): Deleted.
+        (WebCore::ResourceHandle::continueDidReceiveResponse): Deleted.
+        (WebCore::ResourceHandle::continueCanAuthenticateAgainstProtectionSpace): Deleted.
+        * platform/network/curl/CurlRequest.cpp:
+        (WebCore::CurlRequest::start):
+        (WebCore::CurlRequest::cancel):
+        (WebCore::CurlRequest::suspend):
+        (WebCore::CurlRequest::resume):
+        (WebCore::CurlRequest::didReceiveHeader):
+        (WebCore::CurlRequest::didReceiveData):
+        (WebCore::CurlRequest::didCompleteTransfer):
+        (WebCore::CurlRequest::didCancelTransfer):
+        (WebCore::CurlRequest::finalizeTransfer):
+        (WebCore::CurlRequest::invokeDidReceiveResponseForFile):
+        (WebCore::CurlRequest::invokeDidReceiveResponse):
+        (WebCore::CurlRequest::completeDidReceiveResponse):
+        (WebCore::CurlRequest::setRequestPaused):
+        (WebCore::CurlRequest::setCallbackPaused):
+        (WebCore::CurlRequest::pausedStatusChanged):
+        (WebCore::CurlRequest::setPaused): Deleted.
+        * platform/network/curl/CurlRequest.h:
+        (WebCore::CurlRequest::needToInvokeDidReceiveResponse const):
+        (WebCore::CurlRequest::isPaused const):
+        * platform/network/curl/ResourceHandleCurl.cpp:
+        (WebCore::ResourceHandle::receivedRequestToContinueWithoutCredential):
+        (WebCore::ResourceHandle::continueDidReceiveResponse):
+        (WebCore::ResourceHandle::platformContinueSynchronousDidReceiveResponse):
+        * platform/network/curl/ResourceHandleCurlDelegate.cpp:
+        (WebCore::ResourceHandleCurlDelegate::curlDidReceiveResponse):
+        (WebCore::ResourceHandleCurlDelegate::continueDidReceiveResponse):
+        (WebCore::ResourceHandleCurlDelegate::platformContinueSynchronousDidReceiveResponse):
+        (WebCore::ResourceHandleCurlDelegate::continueAfterDidReceiveResponse):
+        (WebCore::ResourceHandleCurlDelegate::shouldRedirectAsGET):
+        * platform/network/curl/ResourceHandleCurlDelegate.h:
+        * platform/network/curl/ResourceResponseCurl.cpp:
+        (WebCore::ResourceResponse::shouldRedirect):
+        (WebCore::ResourceResponse::isMovedPermanently const):
+        (WebCore::ResourceResponse::isFound const):
+        (WebCore::ResourceResponse::isSeeOther const):
+        (WebCore::ResourceResponse::isNotModified const):
+        (WebCore::ResourceResponse::isUnauthorized const):
+
 2017-10-02  Ryosuke Niwa  <rniwa@webkit.org>
 
         PasteImage tests are failing on debug builds
index 45da881..bebcf88 100644 (file)
@@ -170,27 +170,7 @@ void ResourceHandle::didReceiveResponse(ResourceResponse&& response)
     }
 }
 
-#if !PLATFORM(COCOA) && !USE(CFURLCONNECTION) && !USE(SOUP) && !USE(CURL)
-// ResourceHandle never uses async client calls on these platforms yet.
-void ResourceHandle::continueWillSendRequest(ResourceRequest&&)
-{
-    notImplemented();
-}
-
-void ResourceHandle::continueDidReceiveResponse()
-{
-    notImplemented();
-}
-
-#if USE(PROTECTION_SPACE_AUTH_CALLBACK)
-void ResourceHandle::continueCanAuthenticateAgainstProtectionSpace(bool)
-{
-    notImplemented();
-}
-#endif
-#endif
-
-#if !USE(SOUP)
+#if !USE(SOUP) && !USE(CURL)
 void ResourceHandle::platformContinueSynchronousDidReceiveResponse()
 {
     // Do nothing.
index 1c84bdb..adea73c 100644 (file)
@@ -56,6 +56,15 @@ void CurlRequest::setUserPass(const String& user, const String& password)
 
 void CurlRequest::start(bool isSyncRequest)
 {
+    // The pausing of transfer does not work with protocols, like file://.
+    // Therefore, PAUSE can not be done in didReceiveData().
+    // It means that the same logic as http:// can not be used.
+    // In the file scheme, invokeDidReceiveResponse() is done first. 
+    // Then StartWithJobManager is called with completeDidReceiveResponse and start transfer with libcurl.
+
+    // http : didReceiveHeader => didReceiveData[PAUSE] => invokeDidReceiveResponse => (MainThread)curlDidReceiveResponse => completeDidReceiveResponse[RESUME] => didReceiveData
+    // file : invokeDidReceiveResponseForFile => (MainThread)curlDidReceiveResponse => completeDidReceiveResponse => didReceiveData
+
     ASSERT(isMainThread());
 
     m_isSyncRequest = isSyncRequest;
@@ -66,8 +75,8 @@ void CurlRequest::start(bool isSyncRequest)
         // For asynchronous, use CurlJobManager. Curl processes runs on sub thread.
         if (url.isLocalFile())
             invokeDidReceiveResponseForFile(url);
-
-        startWithJobManager();
+        else
+            startWithJobManager();
     } else {
         // For synchronous, does not use CurlJobManager. Curl processes runs on main thread.
         // curl_easy_perform blocks until the transfer is finished.
@@ -101,21 +110,22 @@ void CurlRequest::cancel()
     if (!m_isSyncRequest)
         CurlJobManager::singleton().cancel(this);
 
-    setPaused(false);
+    setRequestPaused(false);
+    setCallbackPaused(false);
 }
 
 void CurlRequest::suspend()
 {
     ASSERT(isMainThread());
 
-    setPaused(true);
+    setRequestPaused(true);
 }
 
 void CurlRequest::resume()
 {
     ASSERT(isMainThread());
 
-    setPaused(false);
+    setRequestPaused(false);
 }
 
 /* `this` is protected inside this method. */
@@ -288,7 +298,8 @@ size_t CurlRequest::didReceiveHeader(String&& header)
     if (auto metrics = m_curlHandle->getNetworkLoadMetrics())
         m_networkLoadMetrics = *metrics;
 
-    invokeDidReceiveResponse();
+    // Response will send at didReceiveData() or didCompleteTransfer()
+    // to receive continueDidRceiveResponse() for asynchronously.
 
     return receiveBytes;
 }
@@ -300,6 +311,19 @@ size_t CurlRequest::didReceiveData(Ref<SharedBuffer>&& buffer)
     if (m_cancelled)
         return 0;
 
+    if (needToInvokeDidReceiveResponse()) {
+        if (!m_isSyncRequest) {
+            // For asynchronous, pause until completeDidReceiveResponse() is called.
+            setCallbackPaused(true);
+            invokeDidReceiveResponse(Action::ReceiveData);
+            return CURL_WRITEFUNC_PAUSE;
+        }
+
+        // For synchronous, completeDidReceiveResponse() is called in invokeDidReceiveResponse().
+        // In this case, pause is unnecessary.
+        invokeDidReceiveResponse(Action::None);
+    }
+
     auto receiveBytes = buffer->size();
 
     if (receiveBytes) {
@@ -320,30 +344,42 @@ void CurlRequest::didCompleteTransfer(CURLcode result)
     }
 
     if (result == CURLE_OK) {
-        if (auto metrics = m_curlHandle->getNetworkLoadMetrics())
-            m_networkLoadMetrics = *metrics;
-
-        callDelegate([this](CurlRequestDelegate* delegate) {
-            if (delegate)
-                delegate->curlDidComplete();
-        });
+        if (needToInvokeDidReceiveResponse()) {
+            // Processing of didReceiveResponse() has not been completed. (For example, HEAD method)
+            // When completeDidReceiveResponse() is called, didCompleteTransfer() will be called again.
+
+            m_finishedResultCode = result;
+            invokeDidReceiveResponse(Action::FinishTransfer);
+        } else {
+            if (auto metrics = m_curlHandle->getNetworkLoadMetrics())
+                m_networkLoadMetrics = *metrics;
+
+            finalizeTransfer();
+            callDelegate([this](CurlRequestDelegate* delegate) {
+                if (delegate)
+                    delegate->curlDidComplete();
+            });
+        }
     } else {
         auto resourceError = ResourceError::httpError(result, m_request.url());
         if (m_sslVerifier.sslErrors())
             resourceError.setSslErrors(m_sslVerifier.sslErrors());
 
+        finalizeTransfer();
         callDelegate([this, error = resourceError.isolatedCopy()](CurlRequestDelegate* delegate) {
             if (delegate)
                 delegate->curlDidFailWithError(error);
         });
     }
-
-    m_formDataStream = nullptr;
-    m_curlHandle = nullptr;
 }
 
 void CurlRequest::didCancelTransfer()
 {
+    finalizeTransfer();
+}
+
+void CurlRequest::finalizeTransfer()
+{
     m_formDataStream = nullptr;
     m_curlHandle = nullptr;
 }
@@ -454,35 +490,90 @@ void CurlRequest::invokeDidReceiveResponseForFile(URL& url)
     if (!m_isSyncRequest) {
         // DidReceiveResponse must not be called immediately
         CurlJobManager::singleton().callOnJobThread([protectedThis = makeRef(*this)]() {
-            protectedThis->invokeDidReceiveResponse();
+            protectedThis->invokeDidReceiveResponse(Action::StartTransfer);
         });
-    } else
-        invokeDidReceiveResponse();
+    } else {
+        // For synchronous, completeDidReceiveResponse() is called in platformContinueSynchronousDidReceiveResponse().
+        invokeDidReceiveResponse(Action::None);
+    }
 }
 
-void CurlRequest::invokeDidReceiveResponse()
+void CurlRequest::invokeDidReceiveResponse(Action behaviorAfterInvoke)
 {
+    ASSERT(!m_didNotifyResponse);
+
+    m_didNotifyResponse = true;
+    m_actionAfterInvoke = behaviorAfterInvoke;
+
     callDelegate([this, response = m_response.isolatedCopy()](CurlRequestDelegate* delegate) {
         if (delegate)
             delegate->curlDidReceiveResponse(response);
     });
 }
 
-void CurlRequest::setPaused(bool paused)
+void CurlRequest::completeDidReceiveResponse()
 {
+    ASSERT(isMainThread());
+    ASSERT(m_didNotifyResponse);
+    ASSERT(!m_didReturnFromNotify);
+
     if (m_cancelled)
         return;
 
-    if (paused == m_isPaused)
+    m_didReturnFromNotify = true;
+
+    if (m_actionAfterInvoke == Action::ReceiveData) {
+        // Resume transfer
+        setCallbackPaused(false);
+    } else if (m_actionAfterInvoke == Action::StartTransfer) {
+        // Start transfer for file scheme
+        startWithJobManager();
+    } else if (m_actionAfterInvoke == Action::FinishTransfer) {
+        // Keep the calling thread of didCompleteTransfer()
+        if (!m_isSyncRequest) {
+            CurlJobManager::singleton().callOnJobThread([protectedThis = makeRef(*this), finishedResultCode = m_finishedResultCode]() {
+                protectedThis->didCompleteTransfer(finishedResultCode);
+            });
+        } else
+            didCompleteTransfer(m_finishedResultCode);
+    }
+}
+
+void CurlRequest::setRequestPaused(bool paused)
+{
+    auto wasPaused = isPaused();
+
+    m_isPausedOfRequest = paused;
+
+    if (isPaused() == wasPaused)
         return;
 
-    m_isPaused = paused;
+    pausedStatusChanged();
+}
 
-    if (!m_curlHandle)
+void CurlRequest::setCallbackPaused(bool paused)
+{
+    auto wasPaused = isPaused();
+
+    m_isPausedOfCallback = paused;
+
+    if (isPaused() == wasPaused)
+        return;
+
+    // In this case, PAUSE will be executed within didReceiveData(). Change pause state and return.
+    if (paused)
+        return;
+
+    pausedStatusChanged();
+}
+
+void CurlRequest::pausedStatusChanged()
+{
+    if (m_cancelled || !m_curlHandle)
         return;
 
     if (!m_isSyncRequest && isMainThread()) {
-        CurlJobManager::singleton().callOnJobThread([protectedThis = makeRef(*this), paused = m_isPaused]() {
+        CurlJobManager::singleton().callOnJobThread([protectedThis = makeRef(*this), paused = isPaused()]() {
             if (protectedThis->m_cancelled)
                 return;
 
@@ -493,8 +584,8 @@ void CurlRequest::setPaused(bool paused)
             }
         });
     } else {
-        auto error = m_curlHandle->pause(m_isPaused ? CURLPAUSE_ALL : CURLPAUSE_CONT);
-        if ((error != CURLE_OK) && !m_isPaused)
+        auto error = m_curlHandle->pause(isPaused() ? CURLPAUSE_ALL : CURLPAUSE_CONT);
+        if ((error != CURLE_OK) && !isPaused())
             cancel();
     }
 }
index e8a9c36..61f2e23 100644 (file)
@@ -56,9 +56,19 @@ public:
 
     bool isSyncRequest() { return m_isSyncRequest; }
 
+    // Processing for DidReceiveResponse
+    void completeDidReceiveResponse();
+
     NetworkLoadMetrics getNetworkLoadMetrics() { return m_networkLoadMetrics.isolatedCopy(); }
 
 private:
+    enum class Action {
+        None,
+        ReceiveData,
+        StartTransfer,
+        FinishTransfer
+    };
+
     void retain() override { ref(); }
     void release() override { deref(); }
     CURL* handle() override { return m_curlHandle ? m_curlHandle->handle() : nullptr; }
@@ -76,6 +86,7 @@ private:
     size_t didReceiveData(Ref<SharedBuffer>&&);
     void didCompleteTransfer(CURLcode) override;
     void didCancelTransfer() override;
+    void finalizeTransfer();
 
     // For POST and PUT method 
     void resolveBlobReferences(ResourceRequest&);
@@ -83,10 +94,14 @@ private:
     void setupPUT(ResourceRequest&);
     void setupFormData(ResourceRequest&, bool);
 
-    // Processing for DidResourceResponse
+    // Processing for DidReceiveResponse
+    bool needToInvokeDidReceiveResponse() const { return !m_didNotifyResponse || !m_didReturnFromNotify; }
     void invokeDidReceiveResponseForFile(URL&);
-    void invokeDidReceiveResponse();
-    void setPaused(bool);
+    void invokeDidReceiveResponse(Action);
+    void setRequestPaused(bool);
+    void setCallbackPaused(bool);
+    void pausedStatusChanged();
+    bool isPaused() const { return m_isPausedOfRequest || m_isPausedOfCallback; };
 
     // Callback functions for curl
     static CURLcode willSetupSslCtxCallback(CURL*, void*, void*);
@@ -111,7 +126,13 @@ private:
     CurlSSLVerifier m_sslVerifier;
     CurlResponse m_response;
 
-    bool m_isPaused { false };
+    bool m_didNotifyResponse { false };
+    bool m_didReturnFromNotify { false };
+    Action m_actionAfterInvoke { Action::None };
+    CURLcode m_finishedResultCode { CURLE_OK };
+
+    bool m_isPausedOfRequest { false };
+    bool m_isPausedOfCallback { false };
 
     NetworkLoadMetrics m_networkLoadMetrics;
 };
index 2e7f0cf..a34b301 100644 (file)
@@ -213,10 +213,10 @@ void ResourceHandle::receivedRequestToContinueWithoutCredential(const Authentica
     if (challenge != d->m_currentWebChallenge)
         return;
 
-    if (d->m_delegate)
-        d->m_delegate->setAuthentication("", "");
-
     clearAuthentication();
+
+    auto protectedThis = makeRef(*this);
+    didReceiveResponse(ResourceResponse(d->m_response));
 }
 
 void ResourceHandle::receivedCancellation(const AuthenticationChallenge& challenge)
@@ -259,7 +259,18 @@ void ResourceHandle::platformLoadResourceSynchronously(NetworkingContext* contex
 
 void ResourceHandle::continueDidReceiveResponse()
 {
-    notImplemented();
+    ASSERT(isMainThread());
+
+    if (d->m_delegate)
+        d->m_delegate->continueDidReceiveResponse();
+}
+
+void ResourceHandle::platformContinueSynchronousDidReceiveResponse()
+{
+    ASSERT(isMainThread());
+
+    if (d->m_delegate)
+        d->m_delegate->platformContinueSynchronousDidReceiveResponse();
 }
 
 void ResourceHandle::continueWillSendRequest(ResourceRequest&& request)
index fff0fef..0fab7ba 100644 (file)
@@ -227,7 +227,9 @@ void ResourceHandleCurlDelegate::curlDidReceiveResponse(const CurlResponse& rece
         }
 
         CurlCacheManager::getInstance().didReceiveResponse(*m_handle, response());
-        m_handle->client()->didReceiveResponse(m_handle, ResourceResponse(response()));
+
+        auto protectedThis = makeRef(*m_handle);
+        m_handle->didReceiveResponse(ResourceResponse(response()));
     }
 }
 
@@ -276,9 +278,34 @@ void ResourceHandleCurlDelegate::curlDidFailWithError(const ResourceError& resou
     m_handle->client()->didFail(m_handle, resourceError);
 }
 
+void ResourceHandleCurlDelegate::continueDidReceiveResponse()
+{
+    ASSERT(isMainThread());
+
+    continueAfterDidReceiveResponse();
+}
+
+void ResourceHandleCurlDelegate::platformContinueSynchronousDidReceiveResponse()
+{
+    ASSERT(isMainThread());
+
+    continueAfterDidReceiveResponse();
+}
+
+void ResourceHandleCurlDelegate::continueAfterDidReceiveResponse()
+{
+    ASSERT(isMainThread());
+
+    // continueDidReceiveResponse might cancel the load.
+    if (cancelledOrClientless() || !m_curlRequest)
+        return;
+
+    m_curlRequest->completeDidReceiveResponse();
+}
+
 bool ResourceHandleCurlDelegate::shouldRedirectAsGET(const ResourceRequest& request, bool crossOrigin)
 {
-    if ((request.httpMethod() == "GET") || (request.httpMethod() == "HEAD"))
+    if (request.httpMethod() == "GET" || request.httpMethod() == "HEAD")
         return false;
 
     if (!request.url().protocolIsInHTTPFamily())
index 7364a54..0e48107 100644 (file)
@@ -55,6 +55,9 @@ public:
 
     void dispatchSynchronousJob();
 
+    void continueDidReceiveResponse();
+    void platformContinueSynchronousDidReceiveResponse();
+
     void continueWillSendRequest(ResourceRequest&&);
 
 private:
@@ -71,6 +74,8 @@ private:
     void curlDidComplete() override;
     void curlDidFailWithError(const ResourceError&) override;
 
+    void continueAfterDidReceiveResponse();
+
     bool shouldRedirectAsGET(const ResourceRequest&, bool crossOrigin);
     void willSendRequest();
     void continueAfterWillSendRequest(ResourceRequest&&);
index 89b76d4..081974d 100644 (file)
@@ -136,7 +136,7 @@ String ResourceResponse::platformSuggestedFilename() const
 bool ResourceResponse::shouldRedirect()
 {
     auto statusCode = httpStatusCode();
-    if ((statusCode < 300) || (400 <= statusCode))
+    if (statusCode < 300 || 400 <= statusCode)
         return false;
 
     // Some 3xx status codes aren't actually redirects.
@@ -151,27 +151,27 @@ bool ResourceResponse::shouldRedirect()
 
 bool ResourceResponse::isMovedPermanently() const
 {
-    return (httpStatusCode() == 301);
+    return httpStatusCode() == 301;
 }
 
 bool ResourceResponse::isFound() const
 {
-    return (httpStatusCode() == 302);
+    return httpStatusCode() == 302;
 }
 
 bool ResourceResponse::isSeeOther() const
 {
-    return (httpStatusCode() == 303);
+    return httpStatusCode() == 303;
 }
 
 bool ResourceResponse::isNotModified() const
 {
-    return (httpStatusCode() == 304);
+    return httpStatusCode() == 304;
 }
 
 bool ResourceResponse::isUnauthorized() const
 {
-    return (httpStatusCode() == 401);
+    return httpStatusCode() == 401;
 }
 
 }