[Curl] Refactor and improve methods in the CurlHandle
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 25 Sep 2017 23:24:37 +0000 (23:24 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 25 Sep 2017 23:24:37 +0000 (23:24 +0000)
https://bugs.webkit.org/show_bug.cgi?id=177373

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

* platform/network/curl/CurlContext.cpp:
(WebCore::CurlShareHandle::~CurlShareHandle):
(WebCore::CurlMultiHandle::~CurlMultiHandle):
(WebCore::CurlHandle::~CurlHandle):
(WebCore::CurlHandle::initialize):
(WebCore::CurlHandle::pause):
(WebCore::CurlHandle::setUrl):
(WebCore::CurlHandle::appendRequestHeaders):
(WebCore::CurlHandle::appendRequestHeader):
(WebCore::CurlHandle::removeRequestHeader):
(WebCore::CurlHandle::enableRequestHeaders):
(WebCore::CurlHandle::getContentLength):
(WebCore::CurlHandle::getNetworkLoadMetrics):
(WebCore::CurlHandle::errorDescription const): Deleted.
(WebCore::CurlHandle::clearUrl): Deleted.
(WebCore::CurlHandle::getContentLenghtDownload): Deleted.
(WebCore::CurlHandle::getTimes): Deleted.
* platform/network/curl/CurlContext.h:
(WebCore::CurlHandle::privateData const): Deleted.
(WebCore::CurlHandle::setPrivateData): Deleted.
(WebCore::CurlHandle::url const): Deleted.
* platform/network/curl/CurlDownload.cpp:
(WebCore::CurlDownload::setupRequest):
* platform/network/curl/CurlSSLVerifier.cpp:
(WebCore::CurlSSLVerifier::certVerifyCallback):
* platform/network/curl/ResourceHandleCurlDelegate.cpp:
(WebCore::ResourceHandleCurlDelegate::setupRequest):
(WebCore::ResourceHandleCurlDelegate::setupPUT):
(WebCore::ResourceHandleCurlDelegate::getNetworkLoadMetrics):
(WebCore::ResourceHandleCurlDelegate::didReceiveHeader):

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

Source/WebCore/ChangeLog
Source/WebCore/platform/network/curl/CurlContext.cpp
Source/WebCore/platform/network/curl/CurlContext.h
Source/WebCore/platform/network/curl/CurlDownload.cpp
Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp
Source/WebCore/platform/network/curl/ResourceHandleCurlDelegate.cpp

index bfa49fb..d934b90 100644 (file)
@@ -1,3 +1,41 @@
+2017-09-25  Basuke Suzuki  <Basuke.Suzuki@sony.com>
+
+        [Curl] Refactor and improve methods in the CurlHandle
+        https://bugs.webkit.org/show_bug.cgi?id=177373
+
+        Reviewed by Alex Christensen.
+
+        * platform/network/curl/CurlContext.cpp:
+        (WebCore::CurlShareHandle::~CurlShareHandle):
+        (WebCore::CurlMultiHandle::~CurlMultiHandle):
+        (WebCore::CurlHandle::~CurlHandle):
+        (WebCore::CurlHandle::initialize):
+        (WebCore::CurlHandle::pause):
+        (WebCore::CurlHandle::setUrl):
+        (WebCore::CurlHandle::appendRequestHeaders):
+        (WebCore::CurlHandle::appendRequestHeader):
+        (WebCore::CurlHandle::removeRequestHeader):
+        (WebCore::CurlHandle::enableRequestHeaders):
+        (WebCore::CurlHandle::getContentLength):
+        (WebCore::CurlHandle::getNetworkLoadMetrics):
+        (WebCore::CurlHandle::errorDescription const): Deleted.
+        (WebCore::CurlHandle::clearUrl): Deleted.
+        (WebCore::CurlHandle::getContentLenghtDownload): Deleted.
+        (WebCore::CurlHandle::getTimes): Deleted.
+        * platform/network/curl/CurlContext.h:
+        (WebCore::CurlHandle::privateData const): Deleted.
+        (WebCore::CurlHandle::setPrivateData): Deleted.
+        (WebCore::CurlHandle::url const): Deleted.
+        * platform/network/curl/CurlDownload.cpp:
+        (WebCore::CurlDownload::setupRequest):
+        * platform/network/curl/CurlSSLVerifier.cpp:
+        (WebCore::CurlSSLVerifier::certVerifyCallback):
+        * platform/network/curl/ResourceHandleCurlDelegate.cpp:
+        (WebCore::ResourceHandleCurlDelegate::setupRequest):
+        (WebCore::ResourceHandleCurlDelegate::setupPUT):
+        (WebCore::ResourceHandleCurlDelegate::getNetworkLoadMetrics):
+        (WebCore::ResourceHandleCurlDelegate::didReceiveHeader):
+
 2017-09-25  Youenn Fablet  <youenn@apple.com>
 
         WebRTC video does not resume receiving when switching back to Safari 11 on iOS
index 89e4230..0fb9b86 100644 (file)
@@ -25,9 +25,9 @@
  */
 
 #include "config.h"
+#include "CurlContext.h"
 
 #if USE(CURL)
-#include "CurlContext.h"
 #include "HTTPHeaderMap.h"
 #include <NetworkLoadMetrics.h>
 #include <wtf/MainThread.h>
@@ -167,10 +167,8 @@ CurlShareHandle::CurlShareHandle()
 
 CurlShareHandle::~CurlShareHandle()
 {
-    if (m_shareHandle) {
+    if (m_shareHandle)
         curl_share_cleanup(m_shareHandle);
-        m_shareHandle = nullptr;
-    }
 }
 
 void CurlShareHandle::lockCallback(CURL*, curl_lock_data data, curl_lock_access, void*)
@@ -213,10 +211,8 @@ CurlMultiHandle::CurlMultiHandle()
 
 CurlMultiHandle::~CurlMultiHandle()
 {
-    if (m_multiHandle) {
+    if (m_multiHandle)
         curl_multi_cleanup(m_multiHandle);
-        m_multiHandle = nullptr;
-    }
 }
 
 CURLMcode CurlMultiHandle::addHandle(CURL* handle)
@@ -258,9 +254,8 @@ CurlHandle::CurlHandle()
 
 CurlHandle::~CurlHandle()
 {
-    clearUrl();
-
-    curl_easy_cleanup(m_handle);
+    if (m_handle)
+        curl_easy_cleanup(m_handle);
 }
 
 const String CurlHandle::errorDescription(CURLcode errorCode)
@@ -268,15 +263,9 @@ const String CurlHandle::errorDescription(CURLcode errorCode)
     return String(curl_easy_strerror(errorCode));
 }
 
-const String CurlHandle::errorDescription() const
-{
-    return errorDescription(m_errorCode);
-}
-
 void CurlHandle::initialize()
 {
     curl_easy_setopt(m_handle, CURLOPT_ERRORBUFFER, m_errorBuffer);
-    curl_easy_setopt(m_handle, CURLOPT_PRIVATE, this);
 }
 
 CURLcode CurlHandle::perform()
@@ -287,7 +276,7 @@ CURLcode CurlHandle::perform()
 
 CURLcode CurlHandle::pause(int bitmask)
 {
-    m_errorCode = curl_easy_pause(m_handle, CURLPAUSE_ALL);
+    m_errorCode = curl_easy_pause(m_handle, bitmask);
     return m_errorCode;
 }
 
@@ -296,21 +285,22 @@ void CurlHandle::enableShareHandle()
     curl_easy_setopt(m_handle, CURLOPT_SHARE, CurlContext::singleton().shareHandle().handle());
 }
 
-void CurlHandle::setUrl(const String& url)
+void CurlHandle::setUrl(const URL& url)
 {
-    clearUrl();
+    URL curlUrl = url;
 
-    // url is in ASCII so latin1() will only convert it to char* without character translation.
-    m_url = fastStrDup(url.latin1().data());
-    curl_easy_setopt(m_handle, CURLOPT_URL, m_url);
-}
+    // Remove any fragment part, otherwise curl will send it as part of the request.
+    curlUrl.removeFragmentIdentifier();
 
-void CurlHandle::clearUrl()
-{
-    if (m_url) {
-        fastFree(m_url);
-        m_url = nullptr;
+    // Remove any query part sent to a local file.
+    if (curlUrl.isLocalFile()) {
+        // By setting the query to a null string it'll be removed.
+        if (!curlUrl.query().isEmpty())
+            curlUrl.setQuery(String());
     }
+
+    // url is in ASCII so latin1() will only convert it to char* without character translation.
+    curl_easy_setopt(m_handle, CURLOPT_URL, curlUrl.string().latin1().data());
 }
 
 void CurlHandle::appendRequestHeaders(const HTTPHeaderMap& headers)
@@ -320,8 +310,6 @@ void CurlHandle::appendRequestHeaders(const HTTPHeaderMap& headers)
             auto& value = entry.value;
             appendRequestHeader(entry.key, entry.value);
         }
-
-        enableRequestHeaders();
     }
 }
 
@@ -340,17 +328,32 @@ void CurlHandle::appendRequestHeader(const String& name, const String& value)
     appendRequestHeader(header);
 }
 
+void CurlHandle::removeRequestHeader(const String& name)
+{
+    // Add a header with no content, the internally used header will get disabled. 
+    String header(name);
+    header.append(":");
+
+    appendRequestHeader(header);
+}
+
 void CurlHandle::appendRequestHeader(const String& header)
 {
+    bool needToEnable = m_requestHeaders.isEmpty();
+
     m_requestHeaders.append(header);
+
+    if (needToEnable)
+        enableRequestHeaders();
 }
 
 void CurlHandle::enableRequestHeaders()
 {
-    if (!m_requestHeaders.isEmpty()) {
-        const struct curl_slist* headers = m_requestHeaders.head();
-        curl_easy_setopt(m_handle, CURLOPT_HTTPHEADER, headers);
-    }
+    if (m_requestHeaders.isEmpty())
+        return;
+
+    const struct curl_slist* headers = m_requestHeaders.head();
+    curl_easy_setopt(m_handle, CURLOPT_HTTPHEADER, headers);
 }
 
 void CurlHandle::enableHttpGetRequest()
@@ -585,7 +588,7 @@ std::optional<long> CurlHandle::getResponseCode()
     return responseCode;
 }
 
-std::optional<long long> CurlHandle::getContentLenghtDownload()
+std::optional<long long> CurlHandle::getContentLength()
 {
     if (!m_handle) {
         m_errorCode = CURLE_FAILED_INIT;
@@ -616,12 +619,11 @@ std::optional<long> CurlHandle::getHttpAuthAvail()
     return httpAuthAvailable;
 }
 
-std::optional<NetworkLoadMetrics> CurlHandle::getTimes()
+std::optional<NetworkLoadMetrics> CurlHandle::getNetworkLoadMetrics()
 {
     double nameLookup = 0.0;
     double connect = 0.0;
     double appConnect = 0.0;
-    double preTransfer = 0.0;
     double startTransfer = 0.0;
 
     if (!m_handle) {
@@ -641,10 +643,6 @@ std::optional<NetworkLoadMetrics> CurlHandle::getTimes()
     if (m_errorCode != CURLE_OK)
         return std::nullopt;
 
-    m_errorCode = curl_easy_getinfo(m_handle, CURLINFO_PRETRANSFER_TIME, &preTransfer);
-    if (m_errorCode != CURLE_OK)
-        return std::nullopt;
-
     m_errorCode = curl_easy_getinfo(m_handle, CURLINFO_STARTTRANSFER_TIME, &startTransfer);
     if (m_errorCode != CURLE_OK)
         return std::nullopt;
index 4ddcfb1..72e3db3 100644 (file)
@@ -204,18 +204,18 @@ class CurlHandle {
     WTF_MAKE_NONCOPYABLE(CurlHandle);
 
 public:
-    enum VerifyPeer {
-        VerifyPeerDisable = 0L,
-        VerifyPeerEnable = 1L
+    enum class VerifyPeer {
+        Disable = 0L,
+        Enable = 1L
     };
 
-    enum VerifyHost {
-        VerifyHostLooseNameCheck = 0,
-        VerifyHostStrictNameCheck = 2
+    enum class VerifyHost {
+        LooseNameCheck = 0,
+        StrictNameCheck = 2
     };
 
     CurlHandle();
-    ~CurlHandle();
+    virtual ~CurlHandle();
 
     CURL* handle() const { return m_handle; }
 
@@ -228,20 +228,15 @@ public:
     void setErrorCode(CURLcode errorCode) { m_errorCode = errorCode; }
 
     static const String errorDescription(CURLcode);
-    const String errorDescription() const;
 
     void enableShareHandle();
 
-    void* privateData() const { return m_privateData; }
-    void setPrivateData(void* userData) { m_privateData = userData; }
-
-    void setUrl(const String&);
-    const char* url() const { return m_url; }
+    void setUrl(const URL&);
 
     void appendRequestHeaders(const HTTPHeaderMap&);
-    void appendRequestHeader(const String&, const String&);
-    void appendRequestHeader(const String&);
-    void enableRequestHeaders();
+    void appendRequestHeader(const String& name, const String& value);
+    void appendRequestHeader(const String& name);
+    void removeRequestHeader(const String& name);
 
     void enableHttpGetRequest();
     void enableHttpHeadRequest();
@@ -286,9 +281,9 @@ public:
     URL getEffectiveURL();
     std::optional<uint16_t> getPrimaryPort();
     std::optional<long> getResponseCode();
-    std::optional<long long> getContentLenghtDownload();
+    std::optional<long long> getContentLength();
     std::optional<long> getHttpAuthAvail();
-    std::optional<NetworkLoadMetrics> getTimes();
+    std::optional<NetworkLoadMetrics> getNetworkLoadMetrics();
 
     static long long maxCurlOffT();
 
@@ -298,16 +293,13 @@ public:
 #endif
 
 private:
-    void clearUrl();
-
+    void enableRequestHeaders();
     static int expectedSizeOfCurlOffT();
 
     CURL* m_handle { nullptr };
     char m_errorBuffer[CURL_ERROR_SIZE] { };
     CURLcode m_errorCode;
 
-    char* m_url { nullptr };
-    void* m_privateData { nullptr };
     CurlSList m_requestHeaders;
 };
 
index bbf4da1..f5b33a0 100644 (file)
@@ -115,7 +115,6 @@ void CurlDownload::setupRequest()
     m_curlHandle.enableShareHandle();
 
     m_curlHandle.setUrl(m_url);
-    m_curlHandle.setPrivateData(this);
     m_curlHandle.setHeaderCallbackFunction(headerCallback, this);
     m_curlHandle.setWriteCallbackFunction(writeCallback, this);
     m_curlHandle.enableFollowLocation();
index d38c920..3e682b2 100644 (file)
@@ -72,7 +72,7 @@ int CurlSSLVerifier::certVerifyCallback(int ok, X509_STORE_CTX* storeCtx)
         // if the host and the certificate are stored for the current handle that means is enabled,
         // so don't need to curl verifies the authenticity of the peer's certificate
         if (verifier->m_curlHandle)
-            verifier->m_curlHandle->setSslVerifyPeer(CurlHandle::VerifyPeerDisable);
+            verifier->m_curlHandle->setSslVerifyPeer(CurlHandle::VerifyPeer::Disable);
     }
 
     return ok;
index ef2997b..fc7019c 100644 (file)
@@ -180,24 +180,8 @@ void ResourceHandleCurlDelegate::setupRequest()
 {
     CurlContext& context = CurlContext::singleton();
 
-    URL url = m_firstRequest.url();
-
-    // Remove any fragment part, otherwise curl will send it as part of the request.
-    url.removeFragmentIdentifier();
-
-    String urlString = url.string();
-
     m_curlHandle.initialize();
 
-    if (url.isLocalFile()) {
-        // Remove any query part sent to a local file.
-        if (!url.query().isEmpty()) {
-            // By setting the query to a null string it'll be removed.
-            url.setQuery(String());
-            urlString = url.string();
-        }
-    }
-
     if (m_defersLoading) {
         CURLcode error = m_curlHandle.pause(CURLPAUSE_ALL);
         // If we did not pause the handle, we would ASSERT in the
@@ -212,9 +196,8 @@ void ResourceHandleCurlDelegate::setupRequest()
 
     auto& sslHandle = CurlContext::singleton().sslHandle();
 
-    m_curlHandle.setSslVerifyPeer(CurlHandle::VerifyPeerEnable);
-    m_curlHandle.setSslVerifyHost(CurlHandle::VerifyHostStrictNameCheck);
-    m_curlHandle.setPrivateData(this);
+    m_curlHandle.setSslVerifyPeer(CurlHandle::VerifyPeer::Enable);
+    m_curlHandle.setSslVerifyHost(CurlHandle::VerifyHost::StrictNameCheck);
     m_curlHandle.setWriteCallbackFunction(didReceiveDataCallback, this);
     m_curlHandle.setHeaderCallbackFunction(didReceiveHeaderCallback, this);
     m_curlHandle.enableAutoReferer();
@@ -224,7 +207,7 @@ void ResourceHandleCurlDelegate::setupRequest()
     m_curlHandle.enableTimeout();
     m_curlHandle.enableAllowedProtocols();
 
-    auto sslClientCertificate = sslHandle.getSSLClientCertificate(url.host());
+    auto sslClientCertificate = sslHandle.getSSLClientCertificate(m_firstRequest.url().host());
     if (sslClientCertificate) {
         m_curlHandle.setSslCert(sslClientCertificate->first.utf8().data());
         m_curlHandle.setSslCertType("P12");
@@ -232,14 +215,14 @@ void ResourceHandleCurlDelegate::setupRequest()
     }
 
     if (sslHandle.shouldIgnoreSSLErrors())
-        m_curlHandle.setSslVerifyPeer(CurlHandle::VerifyPeerDisable);
+        m_curlHandle.setSslVerifyPeer(CurlHandle::VerifyPeer::Disable);
     else
         m_curlHandle.setSslCtxCallbackFunction(willSetupSslCtxCallback, this);
 
     m_curlHandle.setCACertPath(sslHandle.getCACertPath());
 
     m_curlHandle.enableAcceptEncoding();
-    m_curlHandle.setUrl(urlString);
+    m_curlHandle.setUrl(m_firstRequest.url());
     m_curlHandle.enableCookieJarIfExists();
 
     if (m_customHTTPHeaderFields.size())
@@ -259,8 +242,6 @@ void ResourceHandleCurlDelegate::setupRequest()
         setupPUT();
     }
 
-    m_curlHandle.enableRequestHeaders();
-
     applyAuthentication();
 
     m_curlHandle.enableProxyIfExists();
@@ -563,7 +544,7 @@ void ResourceHandleCurlDelegate::setupPUT()
     m_curlHandle.enableHttpPutRequest();
 
     // Disable the Expect: 100 continue header
-    m_curlHandle.appendRequestHeader("Expect:");
+    m_curlHandle.removeRequestHeader("Expect");
 
     size_t numElements = getFormElementsCount();
     if (!numElements)
@@ -651,7 +632,7 @@ void ResourceHandleCurlDelegate::applyAuthentication()
 NetworkLoadMetrics ResourceHandleCurlDelegate::getNetworkLoadMetrics()
 {
     NetworkLoadMetrics networkLoadMetrics;
-    if (auto metrics = m_curlHandle.getTimes())
+    if (auto metrics = m_curlHandle.getNetworkLoadMetrics())
         networkLoadMetrics = *metrics;
 
     return networkLoadMetrics;
@@ -699,6 +680,7 @@ size_t ResourceHandleCurlDelegate::didReceiveHeader(String&& header)
             // Comes here when receiving 200 Connection Established. Just return.
             return header.length();
         }
+
         if (isHttpInfo(httpCode)) {
             // Just return when receiving http info, e.g. HTTP/1.1 100 Continue.
             // If not, the request might be cancelled, because the MIME type will be empty for this response.
@@ -706,7 +688,7 @@ size_t ResourceHandleCurlDelegate::didReceiveHeader(String&& header)
         }
 
         long long contentLength = 0;
-        if (auto length = m_curlHandle.getContentLenghtDownload())
+        if (auto length = m_curlHandle.getContentLength())
             contentLength = *length;
 
         uint16_t connectPort = 0;