Attachment 349815 Details for Bug 189530 – PATCH

[patch] PATCH

189530.diff (text/plain), 14.28 KB, created by Basuke Suzuki on 2018-09-14 15:17:47 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Basuke Suzuki

Created: 2018-09-14 15:17:47 PDT

Size: 14.28 KB

patch

obsolete

>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 593804b6fda..818b785887a 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,34 @@
>+2018-09-14  Basuke Suzuki  <Basuke.Suzuki@sony.com>
>+
>+        [Curl] Bug fix on some inaccurate values in NetworkLoadMetrics.
>+        https://bugs.webkit.org/show_bug.cgi?id=189530
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Curl port uses the start time libcurl provided. But there's a lug between main thread and Curl thread.
>+        Record the start time of request instead of libcurl's start timing and use it to measure the metrics.
>+        Also respondEnd was not correctly recorded and fixed.
>+
>+        No new tests because it cannot be measured from DRT.
>+
>+        * platform/network/ResourceHandleInternal.h:
>+        * platform/network/curl/CurlContext.cpp:
>+        (WebCore::CurlHandle::getNetworkLoadMetrics):
>+        * platform/network/curl/CurlContext.h:
>+        * platform/network/curl/CurlRequest.cpp:
>+        (WebCore::CurlRequest::start):
>+        (WebCore::CurlRequest::setupTransfer):
>+        (WebCore::CurlRequest::didCompleteTransfer):
>+        (WebCore::CurlRequest::updateNetworkLoadMetrics):
>+        * platform/network/curl/CurlRequest.h:
>+        (WebCore::CurlRequest::setStartTime):
>+        * platform/network/curl/ResourceHandleCurl.cpp:
>+        (WebCore::ResourceHandle::start):
>+        (WebCore::ResourceHandle::restartRequestWithCredential):
>+        (WebCore::ResourceHandle::platformLoadResourceSynchronously):
>+        (WebCore::ResourceHandle::willSendRequest):
>+        (WebCore::ResourceHandle::continueAfterWillSendRequest):
>+
> 2018-09-10  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
> 
>         [WTF] Add Markable<T, Traits>
>diff --git a/Source/WebCore/platform/network/ResourceHandleInternal.h b/Source/WebCore/platform/network/ResourceHandleInternal.h
>index 77e26e7317b..243de9f8265 100644
>--- a/Source/WebCore/platform/network/ResourceHandleInternal.h
>+++ b/Source/WebCore/platform/network/ResourceHandleInternal.h
>@@ -40,6 +40,7 @@
> #if USE(CURL)
> #include "CurlRequest.h"
> #include <wtf/MessageQueue.h>
>+#include <wtf/MonotonicTime.h>
> #endif
> 
> #if PLATFORM(COCOA)
>@@ -125,6 +126,7 @@ public:
>     bool m_addedCacheValidationHeaders { false };
>     RefPtr<CurlRequest> m_curlRequest;
>     MessageQueue<WTF::Function<void()>>* m_messageQueue { };
>+    MonotonicTime m_startTime;
> #endif
> 
> #if PLATFORM(COCOA)
>diff --git a/Source/WebCore/platform/network/curl/CurlContext.cpp b/Source/WebCore/platform/network/curl/CurlContext.cpp
>index 64b838c9d5d..d36478ebfce 100644
>--- a/Source/WebCore/platform/network/curl/CurlContext.cpp
>+++ b/Source/WebCore/platform/network/curl/CurlContext.cpp
>@@ -731,7 +731,7 @@ std::optional<long> CurlHandle::getHttpVersion()
>     return version;
> }
> 
>-std::optional<NetworkLoadMetrics> CurlHandle::getNetworkLoadMetrics()
>+std::optional<NetworkLoadMetrics> CurlHandle::getNetworkLoadMetrics(const WTF::Seconds& domainLookupStart)
> {
>     double nameLookup = 0.0;
>     double connect = 0.0;
>@@ -795,18 +795,18 @@ std::optional<NetworkLoadMetrics> CurlHandle::getNetworkLoadMetrics()
> 
>     NetworkLoadMetrics networkLoadMetrics;
> 
>-    networkLoadMetrics.domainLookupStart = Seconds(0);
>-    networkLoadMetrics.domainLookupEnd = Seconds(nameLookup);
>-    networkLoadMetrics.connectStart = Seconds(nameLookup);
>-    networkLoadMetrics.connectEnd = Seconds(connect);
>+    networkLoadMetrics.domainLookupStart = domainLookupStart;
>+    networkLoadMetrics.domainLookupEnd = domainLookupStart + Seconds(nameLookup);
>+    networkLoadMetrics.connectStart = domainLookupStart + Seconds(nameLookup);
>+    networkLoadMetrics.connectEnd = domainLookupStart + Seconds(connect);
> 
>     if (appConnect > 0.0) {
>-        networkLoadMetrics.secureConnectionStart = Seconds(connect);
>-        networkLoadMetrics.connectEnd = Seconds(appConnect);
>+        networkLoadMetrics.secureConnectionStart = domainLookupStart + Seconds(connect);
>+        networkLoadMetrics.connectEnd = domainLookupStart + Seconds(appConnect);
>     }
> 
>     networkLoadMetrics.requestStart = networkLoadMetrics.connectEnd;
>-    networkLoadMetrics.responseStart = Seconds(startTransfer);
>+    networkLoadMetrics.responseStart = domainLookupStart + Seconds(startTransfer);
> 
>     networkLoadMetrics.requestHeaderBytesSent = requestHeaderSize;
>     networkLoadMetrics.requestBodyBytesSent = requestBodySize;
>diff --git a/Source/WebCore/platform/network/curl/CurlContext.h b/Source/WebCore/platform/network/curl/CurlContext.h
>index 5c126af1101..8a94520e67f 100644
>--- a/Source/WebCore/platform/network/curl/CurlContext.h
>+++ b/Source/WebCore/platform/network/curl/CurlContext.h
>@@ -282,7 +282,7 @@ public:
>     std::optional<long> getHttpAuthAvail();
>     std::optional<long> getProxyAuthAvail();
>     std::optional<long> getHttpVersion();
>-    std::optional<NetworkLoadMetrics> getNetworkLoadMetrics();
>+    std::optional<NetworkLoadMetrics> getNetworkLoadMetrics(const WTF::Seconds& domainLookupStart);
> 
>     int sslErrors() const;
>     std::optional<CertificateInfo> certificateInfo() const;
>diff --git a/Source/WebCore/platform/network/curl/CurlRequest.cpp b/Source/WebCore/platform/network/curl/CurlRequest.cpp
>index 5d061ca1c38..2d8aff2c9b7 100644
>--- a/Source/WebCore/platform/network/curl/CurlRequest.cpp
>+++ b/Source/WebCore/platform/network/curl/CurlRequest.cpp
>@@ -80,6 +80,9 @@ void CurlRequest::start()
> 
>     auto url = m_request.url().isolatedCopy();
> 
>+    if (std::isnan(m_requestStartTime))
>+        m_requestStartTime = MonotonicTime::now();
>+
>     if (url.isLocalFile())
>         invokeDidReceiveResponseForFile(url);
>     else
>@@ -195,6 +198,8 @@ CURL* CurlRequest::setupTransfer()
>     if (m_shouldSuspend)
>         setRequestPaused(true);
> 
>+    m_performStartTime = MonotonicTime::now();
>+
>     return m_curlHandle->handle();
> }
> 
>@@ -405,7 +410,10 @@ void CurlRequest::didCompleteTransfer(CURLcode result)
>         updateNetworkLoadMetrics();
> 
>         finalizeTransfer();
>-        callClient([](CurlRequest& request, CurlRequestClient& client) {
>+        callClient([this, protectedThis = makeRef(*this)](CurlRequest& request, CurlRequestClient& client) {
>+            m_networkLoadMetrics.responseEnd = MonotonicTime::now() - m_requestStartTime;
>+            m_networkLoadMetrics.markComplete();
>+
>             client.curlDidComplete(request);
>         });
>     } else {
>@@ -638,7 +646,9 @@ bool CurlRequest::isHandlePaused() const
> 
> void CurlRequest::updateNetworkLoadMetrics()
> {
>-    if (auto metrics = m_curlHandle->getNetworkLoadMetrics())
>+    auto domainLookupStart = m_performStartTime - m_requestStartTime;
>+
>+    if (auto metrics = m_curlHandle->getNetworkLoadMetrics(domainLookupStart))
>         m_networkLoadMetrics = *metrics;
> 
>     m_networkLoadMetrics.requestHeaders = m_request.httpHeaderFields();
>diff --git a/Source/WebCore/platform/network/curl/CurlRequest.h b/Source/WebCore/platform/network/curl/CurlRequest.h
>index d611b113c03..958c777b775 100644
>--- a/Source/WebCore/platform/network/curl/CurlRequest.h
>+++ b/Source/WebCore/platform/network/curl/CurlRequest.h
>@@ -36,6 +36,7 @@
> #include "NetworkLoadMetrics.h"
> #include "ResourceRequest.h"
> #include <wtf/MessageQueue.h>
>+#include <wtf/MonotonicTime.h>
> #include <wtf/Noncopyable.h>
> 
> namespace WebCore {
>@@ -67,6 +68,7 @@ public:
> 
>     void invalidateClient();
>     WEBCORE_EXPORT void setUserPass(const String&, const String&);
>+    void setStartTime(const MonotonicTime& startTime) { m_requestStartTime = startTime; }
> 
>     void start();
>     void cancel();
>@@ -197,6 +199,8 @@ private:
> 
>     CertificateInfo m_certificateInfo;
>     NetworkLoadMetrics m_networkLoadMetrics;
>+    MonotonicTime m_requestStartTime { MonotonicTime::nan() };
>+    MonotonicTime m_performStartTime;
>     size_t m_totalReceivedSize { 0 };
> };
> 
>diff --git a/Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp b/Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp
>index c42adc378a6..99fe1c1448a 100644
>--- a/Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp
>+++ b/Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp
>@@ -81,11 +81,14 @@ bool ResourceHandle::start()
>         return true;
>     }
> 
>+    d->m_startTime = MonotonicTime::now();
>+
>     d->m_curlRequest = createCurlRequest(WTFMove(request));
> 
>     if (auto credential = getCredential(d->m_firstRequest, false))
>         d->m_curlRequest->setUserPass(credential->first, credential->second);
> 
>+    d->m_curlRequest->setStartTime(d->m_startTime);
>     d->m_curlRequest->start();
> 
>     return true;
>@@ -375,6 +378,7 @@ void ResourceHandle::restartRequestWithCredential(const String& user, const Stri
> 
>     d->m_curlRequest = createCurlRequest(WTFMove(previousRequest), RequestStatus::ReusedRequest);
>     d->m_curlRequest->setUserPass(user, password);
>+    d->m_curlRequest->setStartTime(d->m_startTime);
>     d->m_curlRequest->start();
> }
> 
>@@ -388,6 +392,7 @@ void ResourceHandle::platformLoadResourceSynchronously(NetworkingContext* contex
>     bool shouldContentEncodingSniff = true;
>     RefPtr<ResourceHandle> handle = adoptRef(new ResourceHandle(context, request, &client, defersLoading, shouldContentSniff, shouldContentEncodingSniff));
>     handle->d->m_messageQueue = &client.messageQueue();
>+    handle->d->m_startTime = MonotonicTime::now();
> 
>     if (request.url().protocolIsData()) {
>         handle->handleDataURL();
>@@ -396,6 +401,7 @@ void ResourceHandle::platformLoadResourceSynchronously(NetworkingContext* contex
> 
>     auto requestCopy = handle->firstRequest();
>     handle->d->m_curlRequest = handle->createCurlRequest(WTFMove(requestCopy));
>+    handle->d->m_curlRequest->setStartTime(handle->d->m_startTime);
>     handle->d->m_curlRequest->start();
> 
>     do {
>@@ -483,6 +489,7 @@ void ResourceHandle::willSendRequest()
>         // in a cross-origin redirect, we want to clear those headers here. 
>         newRequest.clearHTTPAuthorization();
>         newRequest.clearHTTPOrigin();
>+        d->m_startTime = WTF::MonotonicTime::now();
>     }
> 
>     ResourceResponse responseCopy = delegate()->response();
>@@ -513,6 +520,7 @@ void ResourceHandle::continueAfterWillSendRequest(ResourceRequest&& request)
>     if (shouldForwardCredential && credential)
>         d->m_curlRequest->setUserPass(credential->first, credential->second);
> 
>+    d->m_curlRequest->setStartTime(d->m_startTime);
>     d->m_curlRequest->start();
> }
> 
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index b33404c0fba..bd430f319fa 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,19 @@
>+2018-09-14  Basuke Suzuki  <Basuke.Suzuki@sony.com>
>+
>+        [Curl] Bug fix on some inaccurate values in NetworkLoadMetrics.
>+        https://bugs.webkit.org/show_bug.cgi?id=189530
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Curl port uses the start time libcurl provided. But there's a lug between main thread and Curl thread.
>+        Record the start time of request instead of libcurl's start timing and use it to measure the metrics.
>+
>+        * NetworkProcess/curl/NetworkDataTaskCurl.cpp:
>+        (WebKit::NetworkDataTaskCurl::NetworkDataTaskCurl):
>+        (WebKit::NetworkDataTaskCurl::willPerformHTTPRedirection):
>+        (WebKit::NetworkDataTaskCurl::restartWithCredential):
>+        * NetworkProcess/curl/NetworkDataTaskCurl.h:
>+
> 2018-09-10  Tim Horton  <timothy_horton@apple.com>
> 
>         Try to fix the build after r235850
>diff --git a/Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp b/Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp
>index f7d49c282b1..98a77b2cbb4 100644
>--- a/Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp
>+++ b/Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp
>@@ -46,6 +46,8 @@ NetworkDataTaskCurl::NetworkDataTaskCurl(NetworkSession& session, NetworkDataTas
>     if (m_scheduledFailureType != NoFailure)
>         return;
> 
>+    m_startTime = MonotonicTime::now();
>+
>     auto request = requestWithCredentials;
>     if (request.url().protocolIsInHTTPFamily()) {
>         if (m_storedCredentialsPolicy == StoredCredentialsPolicy::Use) {
>@@ -64,6 +66,7 @@ NetworkDataTaskCurl::NetworkDataTaskCurl(NetworkSession& session, NetworkDataTas
>     m_curlRequest = createCurlRequest(WTFMove(request));
>     if (!m_initialCredential.isEmpty())
>         m_curlRequest->setUserPass(m_initialCredential.user(), m_initialCredential.password());
>+    m_curlRequest->setStartTime(m_startTime);
>     m_curlRequest->start();
> }
> 
>@@ -291,17 +294,21 @@ void NetworkDataTaskCurl::willPerformHTTPRedirection()
>     }
> 
>     auto response = ResourceResponse(m_response);
>-    m_client->willPerformHTTPRedirection(WTFMove(response), WTFMove(request), [this, protectedThis = makeRef(*this), didChangeCredential](const ResourceRequest& newRequest) {
>+    m_client->willPerformHTTPRedirection(WTFMove(response), WTFMove(request), [this, protectedThis = makeRef(*this), didChangeCredential, isCrossOrigin](const ResourceRequest& newRequest) {
>         if (newRequest.isNull() || m_state == State::Canceling)
>             return;
> 
>         if (m_curlRequest)
>             m_curlRequest->cancel();
> 
>+        if (newRequest.url().protocolIsInHTTPFamily() && isCrossOrigin)
>+            m_startTime = MonotonicTime::now();
>+
>         auto requestCopy = newRequest;
>         m_curlRequest = createCurlRequest(WTFMove(requestCopy));
>         if (didChangeCredential && !m_initialCredential.isEmpty())
>             m_curlRequest->setUserPass(m_initialCredential.user(), m_initialCredential.password());
>+        m_curlRequest->setStartTime(m_startTime);
>         m_curlRequest->start();
> 
>         if (m_state != State::Suspended) {
>@@ -393,6 +400,7 @@ void NetworkDataTaskCurl::restartWithCredential(const Credential& credential)
> 
>     m_curlRequest = createCurlRequest(WTFMove(previousRequest), RequestStatus::ReusedRequest);
>     m_curlRequest->setUserPass(credential.user(), credential.password());
>+    m_curlRequest->setStartTime(m_startTime);
>     m_curlRequest->start();
> 
>     if (m_state != State::Suspended) {
>diff --git a/Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.h b/Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.h
>index fcc4d34e4d6..f0db8d3b857 100644
>--- a/Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.h
>+++ b/Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.h
>@@ -85,6 +85,8 @@ private:
>     WebCore::ResourceResponse m_response;
>     unsigned m_redirectCount { 0 };
>     unsigned m_authFailureCount { 0 };
>+
>+    MonotonicTime m_startTime;
> };
> 
> } // namespace WebKit

Actions: View | Formatted Diff | Diff

Attachments on bug 189530: 349815 | 349819