Introduce a new CompletionHandler type and use it for NetworkDataTaskClient's complet...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 23 Aug 2017 00:09:40 +0000 (00:09 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 23 Aug 2017 00:09:40 +0000 (00:09 +0000)
https://bugs.webkit.org/show_bug.cgi?id=175832

Reviewed by Alex Christensen.

Source/WebKit:

Use new CompletionHandler type for NetworkDataTaskClient's completion handlers to help catch bugs.
It actually already found a bug in our HTTP 0.9 error handling which is fixed in this patch
as well.

* NetworkProcess/NetworkDataTask.cpp:
(WebKit::NetworkDataTask::didReceiveResponse):
* NetworkProcess/NetworkDataTask.h:
* NetworkProcess/cocoa/NetworkDataTaskCocoa.h:
* NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:
(WebKit::NetworkDataTaskCocoa::tryPasswordBasedAuthentication):
* Shared/Authentication/AuthenticationManager.cpp:
(WebKit::AuthenticationManager::tryUseCertificateInfoForChallenge):
* Shared/Authentication/AuthenticationManager.h:
* Shared/Authentication/mac/AuthenticationManager.mac.mm:
(WebKit::AuthenticationManager::tryUseCertificateInfoForChallenge):

Source/WTF:

Introduce a new CompletionHandler type which wraps a WTF::Function and ensures via assertions
that the function is always called once and only once.

* WTF.xcodeproj/project.pbxproj:
* wtf/CompletionHandler.h: Added.
(WTF::CompletionHandler<Out):

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

Source/WTF/ChangeLog
Source/WTF/WTF.xcodeproj/project.pbxproj
Source/WTF/wtf/CompletionHandler.h [new file with mode: 0644]
Source/WebKit/ChangeLog
Source/WebKit/NetworkProcess/NetworkDataTask.cpp
Source/WebKit/NetworkProcess/NetworkDataTask.h
Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.h
Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm
Source/WebKit/Shared/Authentication/AuthenticationManager.cpp
Source/WebKit/Shared/Authentication/AuthenticationManager.h
Source/WebKit/Shared/Authentication/mac/AuthenticationManager.mac.mm

index e6db9f7..13996d3 100644 (file)
@@ -1,3 +1,17 @@
+2017-08-22  Chris Dumez  <cdumez@apple.com>
+
+        Introduce a new CompletionHandler type and use it for NetworkDataTaskClient's completion handlers to help catch bugs
+        https://bugs.webkit.org/show_bug.cgi?id=175832
+
+        Reviewed by Alex Christensen.
+
+        Introduce a new CompletionHandler type which wraps a WTF::Function and ensures via assertions
+        that the function is always called once and only once.
+
+        * WTF.xcodeproj/project.pbxproj:
+        * wtf/CompletionHandler.h: Added.
+        (WTF::CompletionHandler<Out):
+
 2017-08-22  Alex Christensen  <achristensen@webkit.org>
 
         Fix Windows build after r221017.
index 4317b45..0526951 100644 (file)
                313EDEC9778E49C9BEA91CFC /* StackTrace.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = StackTrace.cpp; sourceTree = "<group>"; };
                37C7CC291EA40A73007BD956 /* WeakLinking.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WeakLinking.h; sourceTree = "<group>"; };
                430B47871AAAAC1A001223DA /* StringCommon.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = StringCommon.h; sourceTree = "<group>"; };
+               46BA9EAB1F4CD61E009A2BBC /* CompletionHandler.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = CompletionHandler.h; sourceTree = "<group>"; };
                513E170A1CD7D5BF00E3650B /* LoggingAccumulator.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = LoggingAccumulator.h; sourceTree = "<group>"; };
                515F794B1CFC9F4A00CCED93 /* CrossThreadCopier.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = CrossThreadCopier.cpp; sourceTree = "<group>"; };
                515F794C1CFC9F4A00CCED93 /* CrossThreadCopier.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = CrossThreadCopier.h; sourceTree = "<group>"; };
                                0F8F2B8F172E00F0007DBDA5 /* CompilationThread.cpp */,
                                0F8F2B90172E00F0007DBDA5 /* CompilationThread.h */,
                                A8A47270151A825A004123FF /* Compiler.h */,
+                               46BA9EAB1F4CD61E009A2BBC /* CompletionHandler.h */,
                                0FDB698D1B7C643A000C1078 /* Condition.h */,
                                E38C41261EB4E0680042957D /* CPUTime.cpp */,
                                E38C41271EB4E0680042957D /* CPUTime.h */,
                                0FF4B4C41E88939C00DBBE86 /* Liveness.h */,
                                0FE164681B6FFC9600400E7C /* Lock.cpp */,
                                0FE164691B6FFC9600400E7C /* Lock.h */,
-                               0F31DD701F1308BC0072EB4A /* LockAlgorithmInlines.h */,
                                0F0FCDDD1DD167F900CCAB53 /* LockAlgorithm.h */,
+                               0F31DD701F1308BC0072EB4A /* LockAlgorithmInlines.h */,
                                0F60F32D1DFCBD1B00416D6C /* LockedPrintStream.cpp */,
                                0F60F32E1DFCBD1B00416D6C /* LockedPrintStream.h */,
                                A8A472C3151A825A004123FF /* Locker.h */,
                                A8A473A8151A825B004123FF /* bignum-dtoa.cc in Sources */,
                                A8A473AA151A825B004123FF /* bignum.cc in Sources */,
                                A8A47451151A825B004123FF /* BinarySemaphore.cpp in Sources */,
-                               51F1752D1F3D486000C74950 /* PersistentEncoder.cpp in Sources */,
                                A8A4738B151A825B004123FF /* BitVector.cpp in Sources */,
-                               A3E4DD931F3A803400DED0B4 /* TextStream.cpp in Sources */,
                                DCEE22011CEA7551000C2396 /* BlockObjCExceptions.mm in Sources */,
                                A8A473AC151A825B004123FF /* cached-powers.cc in Sources */,
                                0F66B28A1DC97BAB004A1D3F /* ClockType.cpp in Sources */,
                                A8A47460151A825B004123FF /* CollatorDefault.cpp in Sources */,
                                A8A47463151A825B004123FF /* CollatorICU.cpp in Sources */,
                                0F8F2B92172E0103007DBDA5 /* CompilationThread.cpp in Sources */,
-                               51F1752C1F3D486000C74950 /* PersistentDecoder.cpp in Sources */,
                                E38C41281EB4E0680042957D /* CPUTime.cpp in Sources */,
                                E38C41251EB4E04C0042957D /* CPUTimeCocoa.mm in Sources */,
                                515F794E1CFC9F4A00CCED93 /* CrossThreadCopier.cpp in Sources */,
                                A8A473AE151A825B004123FF /* diy-fp.cc in Sources */,
                                A8A473B0151A825B004123FF /* double-conversion.cc in Sources */,
                                A8A473BA151A825B004123FF /* dtoa.cpp in Sources */,
-                               51F1752B1F3D486000C74950 /* PersistentCoders.cpp in Sources */,
                                A8A473B3151A825B004123FF /* fast-dtoa.cc in Sources */,
                                0F7C5FB61D885CF20044F5E2 /* FastBitVector.cpp in Sources */,
                                A8A473C3151A825B004123FF /* FastMalloc.cpp in Sources */,
                                0F9D3360165DBA73005AD387 /* FilePrintStream.cpp in Sources */,
                                A8A473B5151A825B004123FF /* fixed-dtoa.cc in Sources */,
                                1A1D8B9E1731879800141DA4 /* FunctionDispatcher.cpp in Sources */,
+                               0F5BF1761F23D49A0029D91D /* Gigacage.cpp in Sources */,
                                0F30BA901E78708E002CA847 /* GlobalVersion.cpp in Sources */,
                                2CCD892A15C0390200285083 /* GregorianDateTime.cpp in Sources */,
                                A8A473D8151A825B004123FF /* HashTable.cpp in Sources */,
                                0FE1646A1B6FFC9600400E7C /* Lock.cpp in Sources */,
                                0F60F32F1DFCBD1B00416D6C /* LockedPrintStream.cpp in Sources */,
-                               0FEC3C5E1F368A9700F59B6C /* ReadWriteLock.cpp in Sources */,
                                53534F2A1EC0E10E00141B2F /* MachExceptions.defs in Sources */,
                                A8A473E5151A825B004123FF /* MainThread.cpp in Sources */,
                                A8A473E4151A825B004123FF /* MainThreadMac.mm in Sources */,
                                A8A47402151A825B004123FF /* PageBlock.cpp in Sources */,
                                0FFF19DC1BB334EB00886D91 /* ParallelHelperPool.cpp in Sources */,
                                0F824A681B7443A0002E345D /* ParkingLot.cpp in Sources */,
+                               51F1752B1F3D486000C74950 /* PersistentCoders.cpp in Sources */,
+                               51F1752C1F3D486000C74950 /* PersistentDecoder.cpp in Sources */,
+                               51F1752D1F3D486000C74950 /* PersistentEncoder.cpp in Sources */,
                                DCEE22031CEA7551000C2396 /* PlatformUserPreferredLanguagesMac.mm in Sources */,
                                0F9D3362165DBA73005AD387 /* PrintStream.cpp in Sources */,
                                143F611F1565F0F900DB514A /* RAMSize.cpp in Sources */,
                                A3B725EC987446AD93F1A440 /* RandomDevice.cpp in Sources */,
                                A8A47414151A825B004123FF /* RandomNumber.cpp in Sources */,
+                               0FEC3C5E1F368A9700F59B6C /* ReadWriteLock.cpp in Sources */,
                                A8A4741A151A825B004123FF /* RefCountedLeakCounter.cpp in Sources */,
                                2CDED0F318115C85004DBA70 /* RunLoop.cpp in Sources */,
                                2CDED0EF18115C38004DBA70 /* RunLoopCF.cpp in Sources */,
                                A8A47440151A825B004123FF /* StringImpl.cpp in Sources */,
                                A5BA15FC182435A600A82E69 /* StringImplCF.cpp in Sources */,
                                A5BA15F51824348000A82E69 /* StringImplMac.mm in Sources */,
-                               0F5BF1761F23D49A0029D91D /* Gigacage.cpp in Sources */,
                                A5BA15F3182433A900A82E69 /* StringMac.mm in Sources */,
                                0FDDBFA71666DFA300C55FEF /* StringPrintStream.cpp in Sources */,
                                93F1993E19D7958D00C2390B /* StringView.cpp in Sources */,
                                70A993FE1AD7151300FA615B /* SymbolRegistry.cpp in Sources */,
                                1C181C7F1D3078DA00F5FA16 /* TextBreakIterator.cpp in Sources */,
                                1C181C961D30800A00F5FA16 /* TextBreakIteratorInternalICUMac.mm in Sources */,
+                               A3E4DD931F3A803400DED0B4 /* TextStream.cpp in Sources */,
+                               E311FB171F0A568B003C08DE /* ThreadGroup.cpp in Sources */,
                                A8A4744A151A825B004123FF /* Threading.cpp in Sources */,
                                A8A4744E151A825B004123FF /* ThreadingPthreads.cpp in Sources */,
-                               E311FB171F0A568B003C08DE /* ThreadGroup.cpp in Sources */,
                                5311BD5C1EA822F900525281 /* ThreadMessage.cpp in Sources */,
                                0F66B2901DC97BAB004A1D3F /* TimeWithDynamicClockType.cpp in Sources */,
                                1C181C8F1D307AB800F5FA16 /* UTextProvider.cpp in Sources */,
diff --git a/Source/WTF/wtf/CompletionHandler.h b/Source/WTF/wtf/CompletionHandler.h
new file mode 100644 (file)
index 0000000..36b1cb6
--- /dev/null
@@ -0,0 +1,69 @@
+/*
+ * Copyright (C) 2017 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#pragma once
+
+#include "Function.h"
+
+namespace WTF {
+
+template<typename> class CompletionHandler;
+
+// Wraps a WTF::Function to make sure it is always called once and only once.
+template <typename Out, typename... In>
+class CompletionHandler<Out(In...)> {
+public:
+    CompletionHandler() = default;
+
+    template<typename CallableType, class = typename std::enable_if<std::is_rvalue_reference<CallableType&&>::value>::type>
+    CompletionHandler(CallableType&& callable)
+        : m_function(WTFMove(callable))
+    {
+    }
+
+    CompletionHandler(CompletionHandler&&) = default;
+    CompletionHandler& operator=(CompletionHandler&&) = default;
+
+    ~CompletionHandler()
+    {
+        ASSERT_WITH_MESSAGE(!m_function, "Completion handler should always be called");
+    }
+
+    explicit operator bool() const { return !!m_function; }
+
+    Out operator()(In... in)
+    {
+        ASSERT_WITH_MESSAGE(m_function, "Completion handler should not be called more than once");
+        auto function = WTFMove(m_function);
+        return function(std::forward<In>(in)...);
+    }
+
+private:
+    WTF::Function<Out(In...)> m_function;
+};
+
+} // namespace WTF
+
+using WTF::CompletionHandler;
index 88dca66..a7ee7cb 100644 (file)
@@ -1,3 +1,26 @@
+2017-08-22  Chris Dumez  <cdumez@apple.com>
+
+        Introduce a new CompletionHandler type and use it for NetworkDataTaskClient's completion handlers to help catch bugs
+        https://bugs.webkit.org/show_bug.cgi?id=175832
+
+        Reviewed by Alex Christensen.
+
+        Use new CompletionHandler type for NetworkDataTaskClient's completion handlers to help catch bugs.
+        It actually already found a bug in our HTTP 0.9 error handling which is fixed in this patch
+        as well.
+
+        * NetworkProcess/NetworkDataTask.cpp:
+        (WebKit::NetworkDataTask::didReceiveResponse):
+        * NetworkProcess/NetworkDataTask.h:
+        * NetworkProcess/cocoa/NetworkDataTaskCocoa.h:
+        * NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:
+        (WebKit::NetworkDataTaskCocoa::tryPasswordBasedAuthentication):
+        * Shared/Authentication/AuthenticationManager.cpp:
+        (WebKit::AuthenticationManager::tryUseCertificateInfoForChallenge):
+        * Shared/Authentication/AuthenticationManager.h:
+        * Shared/Authentication/mac/AuthenticationManager.mac.mm:
+        (WebKit::AuthenticationManager::tryUseCertificateInfoForChallenge):
+
 2017-08-22  Alex Christensen  <achristensen@webkit.org>
 
         Add UIDelegatePrivate SPI corresponding to WKPageUIClient.showPage
index a9b18b8..a033dc4 100644 (file)
@@ -103,6 +103,7 @@ void NetworkDataTask::didReceiveResponse(ResourceResponse&& response, ResponseCo
         auto url = response.url();
         std::optional<uint16_t> port = url.port();
         if (port && !isDefaultPortForProtocol(port.value(), url.protocol())) {
+            completionHandler({ });
             cancel();
             m_client->didCompleteWithError({ String(), 0, url, "Cancelled load from '" + url.stringCenterEllipsizedToLength() + "' because it is using HTTP/0.9." });
             return;
index eda335e..6a7df90 100644 (file)
@@ -36,7 +36,7 @@
 #include <WebCore/ResourceLoaderOptions.h>
 #include <WebCore/ResourceRequest.h>
 #include <WebCore/Timer.h>
-#include <wtf/Function.h>
+#include <wtf/CompletionHandler.h>
 #include <wtf/text/WTFString.h>
 
 namespace WebCore {
@@ -54,9 +54,9 @@ class NetworkSession;
 class PendingDownload;
 enum class AuthenticationChallengeDisposition;
 
-typedef Function<void(const WebCore::ResourceRequest&)> RedirectCompletionHandler;
-typedef Function<void(AuthenticationChallengeDisposition, const WebCore::Credential&)> ChallengeCompletionHandler;
-typedef Function<void(WebCore::PolicyAction)> ResponseCompletionHandler;
+using RedirectCompletionHandler = CompletionHandler<void(const WebCore::ResourceRequest&)>;
+using ChallengeCompletionHandler = CompletionHandler<void(AuthenticationChallengeDisposition, const WebCore::Credential&)>;
+using ResponseCompletionHandler = CompletionHandler<void(WebCore::PolicyAction)>;
 
 class NetworkDataTaskClient {
 public:
index ceb5963..dfaf66d 100644 (file)
@@ -73,7 +73,7 @@ public:
 private:
     NetworkDataTaskCocoa(NetworkSession&, NetworkDataTaskClient&, const WebCore::ResourceRequest&, WebCore::StoredCredentials, WebCore::ContentSniffingPolicy, bool shouldClearReferrerOnHTTPSToHTTPRedirect);
 
-    bool tryPasswordBasedAuthentication(const WebCore::AuthenticationChallenge&, const ChallengeCompletionHandler&);
+    bool tryPasswordBasedAuthentication(const WebCore::AuthenticationChallenge&, ChallengeCompletionHandler&);
 
     RefPtr<SandboxExtension> m_sandboxExtension;
     RetainPtr<NSURLSessionDataTask> m_task;
index b69bf08..db33da2 100644 (file)
@@ -254,7 +254,7 @@ void NetworkDataTaskCocoa::setPendingDownloadLocation(const WTF::String& filenam
         WebCore::deleteFile(filename);
 }
 
-bool NetworkDataTaskCocoa::tryPasswordBasedAuthentication(const WebCore::AuthenticationChallenge& challenge, const ChallengeCompletionHandler& completionHandler)
+bool NetworkDataTaskCocoa::tryPasswordBasedAuthentication(const WebCore::AuthenticationChallenge& challenge, ChallengeCompletionHandler& completionHandler)
 {
     if (!challenge.protectionSpace().isPasswordBased())
         return false;
index 7df7231..941b76a 100644 (file)
@@ -189,7 +189,7 @@ void AuthenticationManager::didReceiveAuthenticationChallenge(Download& download
 
 // Currently, only Mac knows how to respond to authentication challenges with certificate info.
 #if !HAVE(SEC_IDENTITY)
-bool AuthenticationManager::tryUseCertificateInfoForChallenge(const WebCore::AuthenticationChallenge&, const CertificateInfo&, const ChallengeCompletionHandler&)
+bool AuthenticationManager::tryUseCertificateInfoForChallenge(const WebCore::AuthenticationChallenge&, const CertificateInfo&, ChallengeCompletionHandler&)
 {
     return false;
 }
index 3636ff1..6c49cc7 100644 (file)
@@ -30,8 +30,8 @@
 #include "NetworkProcessSupplement.h"
 #include "WebProcessSupplement.h"
 #include <WebCore/AuthenticationChallenge.h>
+#include <wtf/CompletionHandler.h>
 #include <wtf/Forward.h>
-#include <wtf/Function.h>
 #include <wtf/HashMap.h>
 
 namespace WebCore {
@@ -54,7 +54,7 @@ enum class AuthenticationChallengeDisposition {
     Cancel,
     RejectProtectionSpace
 };
-typedef Function<void(AuthenticationChallengeDisposition, const WebCore::Credential&)> ChallengeCompletionHandler;
+using ChallengeCompletionHandler = CompletionHandler<void(AuthenticationChallengeDisposition, const WebCore::Credential&)>;
 
 class AuthenticationManager : public WebProcessSupplement, public NetworkProcessSupplement, public IPC::MessageReceiver {
     WTF_MAKE_NONCOPYABLE(AuthenticationManager);
@@ -105,7 +105,7 @@ private:
     // IPC::MessageReceiver
     void didReceiveMessage(IPC::Connection&, IPC::Decoder&) override;
 
-    bool tryUseCertificateInfoForChallenge(const WebCore::AuthenticationChallenge&, const WebCore::CertificateInfo&, const ChallengeCompletionHandler&);
+    bool tryUseCertificateInfoForChallenge(const WebCore::AuthenticationChallenge&, const WebCore::CertificateInfo&, ChallengeCompletionHandler&);
 
     uint64_t addChallengeToChallengeMap(Challenge&&);
     bool shouldCoalesceChallenge(uint64_t pageID, uint64_t challengeID, const WebCore::AuthenticationChallenge&) const;
index 6398007..4558c2e 100644 (file)
@@ -69,7 +69,7 @@ static NSArray *chain(const CertificateInfo& certificateInfo)
 }
 
 // FIXME: This function creates an identity from a certificate, which should not be needed. We should pass an identity over IPC (as we do on iOS).
-bool AuthenticationManager::tryUseCertificateInfoForChallenge(const AuthenticationChallenge& challenge, const CertificateInfo& certificateInfo, const ChallengeCompletionHandler& completionHandler)
+bool AuthenticationManager::tryUseCertificateInfoForChallenge(const AuthenticationChallenge& challenge, const CertificateInfo& certificateInfo, ChallengeCompletionHandler& completionHandler)
 {
     if (certificateInfo.isEmpty())
         return false;