Attachment 318768 Details for Bug 175832 – Patch

[patch] Patch

bug-175832-20170822105221.patch (text/plain), 12.31 KB, created by Chris Dumez on 2017-08-22 10:52:22 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2017-08-22 10:52:22 PDT

Size: 12.31 KB

patch

obsolete

>Subversion Revision: 221019
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 860eb39c483f25e7fd644a9c9dad3a604f51796a..0fab88d79fbcb117412063c00d725c25cb2eeb77 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,36 @@
>+2017-08-22  Chris Dumez  <cdumez@apple.com>
>+
>+        Make NetworkDataTaskClient completion handlers less error prone by introducing a wrapper for them
>+        https://bugs.webkit.org/show_bug.cgi?id=175832
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Make NetworkDataTaskClient completion handlers less error prone by introducing a wrapper for them.
>+        This wrapper makes sure that:
>+        1. The completion handler is always called
>+        2. The completion handler is only called once
>+
>+        We've been caught by this several times in the past and it causes hard to diagnose issues such as
>+        memory leaks and network load hangs.
>+
>+        For now, I have only ported RedirectCompletionHandler. I'll port the other ones in a follow-up.
>+
>+        * NetworkProcess/NetworkDataTask.h:
>+        (WebKit::NetworkCompletionHandler<Out):
>+        (WebKit::makeRedirectCompletionHandler):
>+        * NetworkProcess/NetworkLoad.cpp:
>+        (WebKit::NetworkLoad::~NetworkLoad):
>+        (WebKit::NetworkLoad::continueWillSendRequest):
>+        * NetworkProcess/PingLoad.cpp:
>+        (WebKit::PingLoad::~PingLoad):
>+        (WebKit::PingLoad::willPerformHTTPRedirection):
>+        (WebKit::PingLoad::preflightSuccess):
>+        * NetworkProcess/capture/NetworkDataTaskReplay.cpp:
>+        (WebKit::NetworkCapture::NetworkDataTaskReplay::replayRedirectReceived):
>+        * NetworkProcess/cocoa/NetworkSessionCocoa.mm:
>+        (-[WKNetworkSessionDelegate URLSession:task:willPerformHTTPRedirection:newRequest:completionHandler:]):
>+        (-[WKNetworkSessionDelegate URLSession:task:_schemeUpgraded:completionHandler:]):
>+
> 2017-08-22  Brent Fulgham  <bfulgham@apple.com> and Pranjal Jumde  <pjumde@apple.com>
> 
>         Disable access to secure cookies if an HTTPS site loads mixed content
>diff --git a/Source/WebKit/NetworkProcess/NetworkDataTask.h b/Source/WebKit/NetworkProcess/NetworkDataTask.h
>index eda335e5f349c99c8ddf4008e66a5f553681b4b1..b79283eb4025273553fad82d8e2114001a15f934 100644
>--- a/Source/WebKit/NetworkProcess/NetworkDataTask.h
>+++ b/Source/WebKit/NetworkProcess/NetworkDataTask.h
>@@ -54,7 +54,64 @@ class NetworkSession;
> class PendingDownload;
> enum class AuthenticationChallengeDisposition;
> 
>-typedef Function<void(const WebCore::ResourceRequest&)> RedirectCompletionHandler;
>+// Wrapper for networking completion handlers that make sure that:
>+// 1. The underlying handler is always called.
>+// 2. The underlying handler is only called once
>+template<typename> class NetworkCompletionHandler;
>+template<typename Out, typename... In> class NetworkCompletionHandler<Out(In...)> {
>+public:
>+    using HandlerType = WTF::Function<Out(In...)>;
>+
>+    NetworkCompletionHandler() = default;
>+    NetworkCompletionHandler(HandlerType&& handler, WTF::Function<void(HandlerType&)>&& defaultHandler)
>+        : m_handler(WTFMove(handler))
>+        , m_defaultHandler(WTFMove(defaultHandler))
>+    {
>+    }
>+
>+    ~NetworkCompletionHandler()
>+    {
>+        if (m_defaultHandler)
>+            m_defaultHandler(m_handler);
>+    }
>+
>+    NetworkCompletionHandler(NetworkCompletionHandler&&) = default;
>+    NetworkCompletionHandler& operator=(NetworkCompletionHandler&& other)
>+    {
>+        if (m_defaultHandler)
>+            m_defaultHandler(m_handler);
>+
>+        m_handler = WTFMove(other.m_handler);
>+        m_defaultHandler = WTFMove(other.m_defaultHandler);
>+        return *this;
>+    }
>+
>+    Out operator()(In... in)
>+    {
>+        auto handler = WTFMove(m_handler);
>+        m_defaultHandler = nullptr;
>+        return handler(std::forward<In>(in)...);
>+    }
>+
>+    explicit operator bool() const { return !!m_handler; }
>+
>+private:
>+    HandlerType m_handler;
>+    WTF::Function<void(HandlerType&)> m_defaultHandler;
>+};
>+
>+using RedirectCompletionHandler = NetworkCompletionHandler<void(const WebCore::ResourceRequest&)>;
>+inline RedirectCompletionHandler makeRedirectCompletionHandler(WTF::Function<void(const WebCore::ResourceRequest&)>&& handler)
>+{
>+    return {
>+        WTFMove(handler),
>+        [](auto& wrappedHandler) {
>+            wrappedHandler({ });
>+        }
>+    };
>+}
>+
>+// FIXME: Use NetworkCompletionHandler<> instead of Function<>.
> typedef Function<void(AuthenticationChallengeDisposition, const WebCore::Credential&)> ChallengeCompletionHandler;
> typedef Function<void(WebCore::PolicyAction)> ResponseCompletionHandler;
> 
>diff --git a/Source/WebKit/NetworkProcess/NetworkLoad.cpp b/Source/WebKit/NetworkProcess/NetworkLoad.cpp
>index 4d1075ce8a29a14a09198c66a222038bce8bd750..54778536eb50b3d142fd66df57ccf2d13e856a95 100644
>--- a/Source/WebKit/NetworkProcess/NetworkLoad.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkLoad.cpp
>@@ -135,8 +135,6 @@ NetworkLoad::~NetworkLoad()
> #if USE(NETWORK_SESSION)
>     if (m_responseCompletionHandler)
>         m_responseCompletionHandler(PolicyIgnore);
>-    if (m_redirectCompletionHandler)
>-        m_redirectCompletionHandler({ });
> #if USE(PROTECTION_SPACE_AUTH_CALLBACK)
>     if (m_challengeCompletionHandler)
>         m_challengeCompletionHandler(AuthenticationChallengeDisposition::Cancel, { });
>@@ -199,7 +197,7 @@ void NetworkLoad::continueWillSendRequest(WebCore::ResourceRequest&& newRequest)
> #endif
> 
> #if USE(NETWORK_SESSION)
>-    auto redirectCompletionHandler = std::exchange(m_redirectCompletionHandler, nullptr);
>+    auto redirectCompletionHandler = WTFMove(m_redirectCompletionHandler);
>     ASSERT(redirectCompletionHandler);
>     if (m_currentRequest.isNull()) {
>         NetworkLoadMetrics emptyMetrics;
>diff --git a/Source/WebKit/NetworkProcess/PingLoad.cpp b/Source/WebKit/NetworkProcess/PingLoad.cpp
>index db68a089aed4df4c4d985fc3662f4996faffee4b..c20fbccd1d72851ebf5feb10b765daaacc5a3386 100644
>--- a/Source/WebKit/NetworkProcess/PingLoad.cpp
>+++ b/Source/WebKit/NetworkProcess/PingLoad.cpp
>@@ -68,9 +68,6 @@ PingLoad::PingLoad(NetworkResourceLoadParameters&& parameters, HTTPHeaderMap&& o
> 
> PingLoad::~PingLoad()
> {
>-    if (m_redirectHandler)
>-        m_redirectHandler({ });
>-
>     if (m_task) {
>         ASSERT(m_task->client() == this);
>         m_task->clearClient();
>@@ -105,7 +102,6 @@ void PingLoad::willPerformHTTPRedirection(ResourceResponse&& redirectResponse, R
> {
>     RELEASE_LOG_IF_ALLOWED("willPerformHTTPRedirection - shouldFollowRedirects? %d", m_parameters.shouldFollowRedirects);
>     if (!m_parameters.shouldFollowRedirects) {
>-        completionHandler({ });
>         didFinish(ResourceError { String(), 0, currentRequest().url(), ASCIILiteral("Not allowed to follow redirects"), ResourceError::Type::AccessControl });
>         return;
>     }
>@@ -114,7 +110,6 @@ void PingLoad::willPerformHTTPRedirection(ResourceResponse&& redirectResponse, R
>     if (processContentExtensionRulesForLoad(request)) {
>         RELEASE_LOG_IF_ALLOWED("willPerformHTTPRedirection - Redirect was blocked by content extensions");
>         m_lastRedirectionRequest = request;
>-        completionHandler({ });
>         didFinish(ResourceError { String(), 0, currentRequest().url(), ASCIILiteral("Blocked by content extension"), ResourceError::Type::AccessControl });
>         return;
>     }
>@@ -125,7 +120,6 @@ void PingLoad::willPerformHTTPRedirection(ResourceResponse&& redirectResponse, R
>     if (auto* contentSecurityPolicy = this->contentSecurityPolicy()) {
>         if (!contentSecurityPolicy->allowConnectToSource(request.url(), redirectResponse.isNull() ? ContentSecurityPolicy::RedirectResponseReceived::No : ContentSecurityPolicy::RedirectResponseReceived::Yes)) {
>             RELEASE_LOG_IF_ALLOWED("willPerformHTTPRedirection - Redirect was blocked by CSP");
>-            completionHandler({ });
>             didFinish(ResourceError { String(), 0, currentRequest().url(), ASCIILiteral("Blocked by Content Security Policy"), ResourceError::Type::AccessControl });
>             return;
>         }
>@@ -303,7 +297,7 @@ void PingLoad::preflightSuccess(ResourceRequest&& request)
>     ResourceRequest actualRequest = WTFMove(request);
>     updateRequestForAccessControl(actualRequest, securityOrigin(), m_parameters.allowStoredCredentials);
> 
>-    if (auto redirectHandler = std::exchange(m_redirectHandler, nullptr))
>+    if (auto redirectHandler = WTFMove(m_redirectHandler))
>         redirectHandler(WTFMove(actualRequest));
>     else
>         loadRequest(WTFMove(actualRequest));
>diff --git a/Source/WebKit/NetworkProcess/capture/NetworkDataTaskReplay.cpp b/Source/WebKit/NetworkProcess/capture/NetworkDataTaskReplay.cpp
>index 30d7302527929644449e08d39b6c04080c54756d..4253bc29feed9a14fd592bec4d3b3747058d0b0c 100644
>--- a/Source/WebKit/NetworkProcess/capture/NetworkDataTaskReplay.cpp
>+++ b/Source/WebKit/NetworkProcess/capture/NetworkDataTaskReplay.cpp
>@@ -203,12 +203,13 @@ void NetworkDataTaskReplay::replayRedirectReceived(const RedirectReceivedEvent&
>     WebCore::ResourceRequest receivedRequest = event.request;
> 
>     ASSERT(m_client);
>-    m_client->willPerformHTTPRedirection(WTFMove(receivedResponse), WTFMove(receivedRequest), [this, protectedThis = makeRef(*this)] (const WebCore::ResourceRequest& updatedRequest) {
>+    RedirectCompletionHandler handler = makeRedirectCompletionHandler([this, protectedThis = makeRef(*this)] (const WebCore::ResourceRequest& updatedRequest) {
>         DEBUG_LOG("replayRedirectReceived callback: URL = " STRING_SPECIFIER, DEBUG_STR(m_firstRequest.url().string()));
> 
>         m_currentRequest = updatedRequest;
>         enqueueEventHandler();
>     });
>+    m_client->willPerformHTTPRedirection(WTFMove(receivedResponse), WTFMove(receivedRequest), WTFMove(handler));
> }
> 
> void NetworkDataTaskReplay::replayRedirectSent(const RedirectSentEvent& event)
>diff --git a/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm b/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm
>index 8754821736614221cd550ea3fab521d334971d6b..5dd1e66487ffa90f0642951a5639bebb792b66af 100644
>--- a/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm
>+++ b/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm
>@@ -165,7 +165,7 @@ static WebCore::NetworkLoadPriority toNetworkLoadPriority(float priority)
> 
>     if (auto* networkDataTask = [self existingTask:task]) {
>         auto completionHandlerCopy = Block_copy(completionHandler);
>-        networkDataTask->willPerformHTTPRedirection(response, request, [completionHandlerCopy, taskIdentifier](auto& request) {
>+        networkDataTask->willPerformHTTPRedirection(response, request, makeRedirectCompletionHandler([completionHandlerCopy, taskIdentifier](auto& request) {
> #if !LOG_DISABLED
>             LOG(NetworkSession, "%llu willPerformHTTPRedirection completionHandler (%s)", taskIdentifier, request.url().string().utf8().data());
> #else
>@@ -173,7 +173,7 @@ static WebCore::NetworkLoadPriority toNetworkLoadPriority(float priority)
> #endif
>             completionHandlerCopy(request.nsURLRequest(WebCore::UpdateHTTPBody));
>             Block_release(completionHandlerCopy);
>-        });
>+        }));
>     } else {
>         LOG(NetworkSession, "%llu willPerformHTTPRedirection completionHandler (nil)", taskIdentifier);
>         completionHandler(nil);
>@@ -187,7 +187,7 @@ static WebCore::NetworkLoadPriority toNetworkLoadPriority(float priority)
>     
>     if (auto* networkDataTask = [self existingTask:task]) {
>         auto completionHandlerCopy = Block_copy(completionHandler);
>-        networkDataTask->willPerformHTTPRedirection(WebCore::synthesizeRedirectResponseIfNecessary([task currentRequest], request, nil), request, [completionHandlerCopy, taskIdentifier](auto& request) {
>+        networkDataTask->willPerformHTTPRedirection(WebCore::synthesizeRedirectResponseIfNecessary([task currentRequest], request, nil), request, makeRedirectCompletionHandler([completionHandlerCopy, taskIdentifier](auto& request) {
> #if !LOG_DISABLED
>             LOG(NetworkSession, "%llu _schemeUpgraded completionHandler (%s)", taskIdentifier, request.url().string().utf8().data());
> #else
>@@ -195,7 +195,7 @@ static WebCore::NetworkLoadPriority toNetworkLoadPriority(float priority)
> #endif
>             completionHandlerCopy(request.nsURLRequest(WebCore::UpdateHTTPBody));
>             Block_release(completionHandlerCopy);
>-        });
>+        }));
>     } else {
>         LOG(NetworkSession, "%llu _schemeUpgraded completionHandler (nil)", taskIdentifier);
>         completionHandler(nil);

Actions: View | Formatted Diff | Diff

Attachments on bug 175832: 318768 | 318809