Add threading assertion to WTF::CompletionHandler
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 8 Jul 2019 18:26:47 +0000 (18:26 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 8 Jul 2019 18:26:47 +0000 (18:26 +0000)
https://bugs.webkit.org/show_bug.cgi?id=199516

Reviewed by Alex Christensen.

Source/WebCore:

Update some MessagePort-related code to use WTF::Function instead of WTF::CompletionHandler
since the callback is always called on the main thread, even when it was created on a
worker thread. Ideally, this code would be refactored so that the callback gets called on
the worker thread directly.

* dom/messageports/MessagePortChannel.cpp:
(WebCore::MessagePortChannel::checkRemotePortForActivity):
* dom/messageports/MessagePortChannel.h:
* dom/messageports/MessagePortChannelProvider.h:
* dom/messageports/MessagePortChannelProviderImpl.cpp:
(WebCore::MessagePortChannelProviderImpl::checkRemotePortForActivity):
* dom/messageports/MessagePortChannelProviderImpl.h:
* dom/messageports/MessagePortChannelRegistry.cpp:
(WebCore::MessagePortChannelRegistry::checkRemotePortForActivity):
* dom/messageports/MessagePortChannelRegistry.h:

Source/WebKit:

Update some MessagePort-related code to use WTF::Function instead of WTF::CompletionHandler
since the callback is always called on the main thread, even when it was created on a
worker thread. Ideally, this code would be refactored so that the callback gets called on
the worker thread directly.

* UIProcess/UIMessagePortChannelProvider.cpp:
(WebKit::UIMessagePortChannelProvider::checkRemotePortForActivity):
* UIProcess/UIMessagePortChannelProvider.h:
* WebProcess/WebCoreSupport/WebMessagePortChannelProvider.cpp:
(WebKit::WebMessagePortChannelProvider::checkRemotePortForActivity):
* WebProcess/WebCoreSupport/WebMessagePortChannelProvider.h:

Source/WTF:

Add threading assertion to WTF::CompletionHandler to try and catch common cases
of unsafe usage (completion handler constructed on one thread but called on
another).

* wtf/CompletionHandler.h:
(WTF::CompletionHandler<Out):

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

15 files changed:
Source/WTF/ChangeLog
Source/WTF/wtf/CompletionHandler.h
Source/WebCore/ChangeLog
Source/WebCore/dom/messageports/MessagePortChannel.cpp
Source/WebCore/dom/messageports/MessagePortChannel.h
Source/WebCore/dom/messageports/MessagePortChannelProvider.h
Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp
Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h
Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp
Source/WebCore/dom/messageports/MessagePortChannelRegistry.h
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/UIMessagePortChannelProvider.cpp
Source/WebKit/UIProcess/UIMessagePortChannelProvider.h
Source/WebKit/WebProcess/WebCoreSupport/WebMessagePortChannelProvider.cpp
Source/WebKit/WebProcess/WebCoreSupport/WebMessagePortChannelProvider.h

index 71ce2ab..80acbc3 100644 (file)
@@ -1,3 +1,17 @@
+2019-07-08  Chris Dumez  <cdumez@apple.com>
+
+        Add threading assertion to WTF::CompletionHandler
+        https://bugs.webkit.org/show_bug.cgi?id=199516
+
+        Reviewed by Alex Christensen.
+
+        Add threading assertion to WTF::CompletionHandler to try and catch common cases
+        of unsafe usage (completion handler constructed on one thread but called on
+        another).
+
+        * wtf/CompletionHandler.h:
+        (WTF::CompletionHandler<Out):
+
 2019-07-08  Antoine Quint  <graouts@apple.com>
 
         [Pointer Events] Enable only on the most recent version of the supported iOS family
index fda54a6..5fdd332 100644 (file)
@@ -26,6 +26,7 @@
 #pragma once
 
 #include <wtf/Function.h>
+#include <wtf/MainThread.h>
 
 namespace WTF {
 
@@ -40,6 +41,9 @@ public:
     template<typename CallableType, class = typename std::enable_if<std::is_rvalue_reference<CallableType&&>::value>::type>
     CompletionHandler(CallableType&& callable)
         : m_function(WTFMove(callable))
+#if !ASSERT_DISABLED
+        , m_wasConstructedOnMainThread(isMainThread())
+#endif
     {
     }
 
@@ -55,12 +59,16 @@ public:
 
     Out operator()(In... in)
     {
+        ASSERT(m_wasConstructedOnMainThread == isMainThread());
         ASSERT_WITH_MESSAGE(m_function, "Completion handler should not be called more than once");
         return std::exchange(m_function, nullptr)(std::forward<In>(in)...);
     }
 
 private:
     Function<Out(In...)> m_function;
+#if !ASSERT_DISABLED
+    bool m_wasConstructedOnMainThread;
+#endif
 };
 
 namespace Detail {
index 28a27f3..2e95a9b 100644 (file)
@@ -1,3 +1,26 @@
+2019-07-08  Chris Dumez  <cdumez@apple.com>
+
+        Add threading assertion to WTF::CompletionHandler
+        https://bugs.webkit.org/show_bug.cgi?id=199516
+
+        Reviewed by Alex Christensen.
+
+        Update some MessagePort-related code to use WTF::Function instead of WTF::CompletionHandler
+        since the callback is always called on the main thread, even when it was created on a
+        worker thread. Ideally, this code would be refactored so that the callback gets called on
+        the worker thread directly.
+
+        * dom/messageports/MessagePortChannel.cpp:
+        (WebCore::MessagePortChannel::checkRemotePortForActivity):
+        * dom/messageports/MessagePortChannel.h:
+        * dom/messageports/MessagePortChannelProvider.h:
+        * dom/messageports/MessagePortChannelProviderImpl.cpp:
+        (WebCore::MessagePortChannelProviderImpl::checkRemotePortForActivity):
+        * dom/messageports/MessagePortChannelProviderImpl.h:
+        * dom/messageports/MessagePortChannelRegistry.cpp:
+        (WebCore::MessagePortChannelRegistry::checkRemotePortForActivity):
+        * dom/messageports/MessagePortChannelRegistry.h:
+
 2019-07-08  Charlie Turner  <cturner@igalia.com>
 
         REGRESSION(r243197): [GStreamer] Web process hangs when scrolling twitter timeline which contains HLS videos
index 9cf5497..a5b60d9 100644 (file)
@@ -182,7 +182,7 @@ void MessagePortChannel::takeAllMessagesForPort(const MessagePortIdentifier& por
     });
 }
 
-void MessagePortChannel::checkRemotePortForActivity(const MessagePortIdentifier& remotePort, CompletionHandler<void(MessagePortChannelProvider::HasActivity)>&& callback)
+void MessagePortChannel::checkRemotePortForActivity(const MessagePortIdentifier& remotePort, Function<void(MessagePortChannelProvider::HasActivity)>&& callback)
 {
     ASSERT(isMainThread());
     ASSERT(remotePort == m_ports[0] || remotePort == m_ports[1]);
@@ -207,7 +207,7 @@ void MessagePortChannel::checkRemotePortForActivity(const MessagePortIdentifier&
         return;
     }
 
-    auto outerCallback = CompletionHandler<void(MessagePortChannelProvider::HasActivity)> { [this, protectedThis = makeRef(*this), callback = WTFMove(callback)] (MessagePortChannelProvider::HasActivity hasActivity) mutable {
+    auto outerCallback = Function<void(MessagePortChannelProvider::HasActivity)> { [this, protectedThis = makeRef(*this), callback = WTFMove(callback)] (MessagePortChannelProvider::HasActivity hasActivity) mutable {
         if (hasActivity == MessagePortChannelProvider::HasActivity::Yes) {
             callback(hasActivity);
             return;
index 05f6945..cc2994d 100644 (file)
@@ -54,7 +54,7 @@ public:
     bool postMessageToRemote(MessageWithMessagePorts&&, const MessagePortIdentifier& remoteTarget);
 
     void takeAllMessagesForPort(const MessagePortIdentifier&, Function<void(Vector<MessageWithMessagePorts>&&, Function<void()>&&)>&&);
-    void checkRemotePortForActivity(const MessagePortIdentifier&, CompletionHandler<void(MessagePortChannelProvider::HasActivity)>&& callback);
+    void checkRemotePortForActivity(const MessagePortIdentifier&, Function<void(MessagePortChannelProvider::HasActivity)>&& callback);
 
     WEBCORE_EXPORT bool hasAnyMessagesPendingOrInFlight() const;
 
index 0c145a7..8a3aba2 100644 (file)
@@ -46,14 +46,20 @@ public:
     virtual void entangleLocalPortInThisProcessToRemote(const MessagePortIdentifier& local, const MessagePortIdentifier& remote) = 0;
     virtual void messagePortDisentangled(const MessagePortIdentifier& local) = 0;
     virtual void messagePortClosed(const MessagePortIdentifier& local) = 0;
+    
+    // FIXME: Ideally the callback would be a CompletionHandler but it is always called on the caller's
+    // thread at the moment.
     virtual void takeAllMessagesForPort(const MessagePortIdentifier&, Function<void(Vector<MessageWithMessagePorts>&&, Function<void()>&&)>&&) = 0;
+
     virtual void postMessageToRemote(MessageWithMessagePorts&&, const MessagePortIdentifier& remoteTarget) = 0;
 
     enum class HasActivity {
         Yes,
         No,
     };
-    virtual void checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, CompletionHandler<void(HasActivity)>&& callback) = 0;
+    // FIXME: Ideally the callback would be a CompletionHandler but it is always called on the caller's
+    // thread at the moment.
+    virtual void checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, Function<void(HasActivity)>&& callback) = 0;
 
     // Operations that the coordinating process performs (e.g. the UIProcess)
     virtual void checkProcessLocalPortForActivity(const MessagePortIdentifier&, ProcessIdentifier, CompletionHandler<void(HasActivity)>&&) = 0;
index e74d99a..0b07c27 100644 (file)
@@ -100,9 +100,9 @@ void MessagePortChannelProviderImpl::takeAllMessagesForPort(const MessagePortIde
     });
 }
 
-void MessagePortChannelProviderImpl::checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, CompletionHandler<void(HasActivity)>&& outerCallback)
+void MessagePortChannelProviderImpl::checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, Function<void(HasActivity)>&& outerCallback)
 {
-    auto callback = CompletionHandler<void(HasActivity)> { [outerCallback = WTFMove(outerCallback)](HasActivity hasActivity) mutable {
+    auto callback = Function<void(HasActivity)> { [outerCallback = WTFMove(outerCallback)](HasActivity hasActivity) mutable {
         ASSERT(isMainThread());
         outerCallback(hasActivity);
     } };
index 28f6ad5..d3554e1 100644 (file)
@@ -42,7 +42,7 @@ private:
     void messagePortClosed(const MessagePortIdentifier& local) final;
     void postMessageToRemote(MessageWithMessagePorts&&, const MessagePortIdentifier& remoteTarget) final;
     void takeAllMessagesForPort(const MessagePortIdentifier&, Function<void(Vector<MessageWithMessagePorts>&&, Function<void()>&&)>&&) final;
-    void checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, CompletionHandler<void(HasActivity)>&& callback) final;
+    void checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, Function<void(HasActivity)>&& callback) final;
 
     void checkProcessLocalPortForActivity(const MessagePortIdentifier&, ProcessIdentifier, CompletionHandler<void(HasActivity)>&&) final;
 
index e1d2f82..1cb3554 100644 (file)
@@ -157,7 +157,7 @@ void MessagePortChannelRegistry::takeAllMessagesForPort(const MessagePortIdentif
     channel->takeAllMessagesForPort(port, WTFMove(callback));
 }
 
-void MessagePortChannelRegistry::checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, CompletionHandler<void(MessagePortChannelProvider::HasActivity)>&& callback)
+void MessagePortChannelRegistry::checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, Function<void(MessagePortChannelProvider::HasActivity)>&& callback)
 {
     ASSERT(isMainThread());
 
index 916e5e0..531aa95 100644 (file)
@@ -44,7 +44,7 @@ public:
     WEBCORE_EXPORT void didCloseMessagePort(const MessagePortIdentifier& local);
     WEBCORE_EXPORT bool didPostMessageToRemote(MessageWithMessagePorts&&, const MessagePortIdentifier& remoteTarget);
     WEBCORE_EXPORT void takeAllMessagesForPort(const MessagePortIdentifier&, Function<void(Vector<MessageWithMessagePorts>&&, Function<void()>&&)>&&);
-    WEBCORE_EXPORT void checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, CompletionHandler<void(MessagePortChannelProvider::HasActivity)>&& callback);
+    WEBCORE_EXPORT void checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, Function<void(MessagePortChannelProvider::HasActivity)>&& callback);
 
     WEBCORE_EXPORT MessagePortChannel* existingChannelContainingPort(const MessagePortIdentifier&);
 
index 86ae6f5..5c5f0df 100644 (file)
@@ -1,3 +1,22 @@
+2019-07-08  Chris Dumez  <cdumez@apple.com>
+
+        Add threading assertion to WTF::CompletionHandler
+        https://bugs.webkit.org/show_bug.cgi?id=199516
+
+        Reviewed by Alex Christensen.
+
+        Update some MessagePort-related code to use WTF::Function instead of WTF::CompletionHandler
+        since the callback is always called on the main thread, even when it was created on a
+        worker thread. Ideally, this code would be refactored so that the callback gets called on
+        the worker thread directly.
+
+        * UIProcess/UIMessagePortChannelProvider.cpp:
+        (WebKit::UIMessagePortChannelProvider::checkRemotePortForActivity):
+        * UIProcess/UIMessagePortChannelProvider.h:
+        * WebProcess/WebCoreSupport/WebMessagePortChannelProvider.cpp:
+        (WebKit::WebMessagePortChannelProvider::checkRemotePortForActivity):
+        * WebProcess/WebCoreSupport/WebMessagePortChannelProvider.h:
+
 2019-07-08  Antoine Quint  <graouts@apple.com>
 
         [Pointer Events] "touch-action: none" does not prevent double-tap-to-zoom
index f990820..fb4c644 100644 (file)
@@ -85,7 +85,7 @@ void UIMessagePortChannelProvider::postMessageToRemote(MessageWithMessagePorts&&
     ASSERT_NOT_REACHED();
 }
 
-void UIMessagePortChannelProvider::checkRemotePortForActivity(const MessagePortIdentifier&, CompletionHandler<void(HasActivity)>&&)
+void UIMessagePortChannelProvider::checkRemotePortForActivity(const MessagePortIdentifier&, Function<void(HasActivity)>&&)
 {
     // Should never be called in the UI process provider.
     ASSERT_NOT_REACHED();
index f1e0c49..6c26960 100644 (file)
@@ -45,7 +45,7 @@ private:
     void messagePortClosed(const WebCore::MessagePortIdentifier& local) final;
     void takeAllMessagesForPort(const WebCore::MessagePortIdentifier&, Function<void(Vector<WebCore::MessageWithMessagePorts>&&, Function<void()>&&)>&&) final;
     void postMessageToRemote(WebCore::MessageWithMessagePorts&&, const WebCore::MessagePortIdentifier& remoteTarget) final;
-    void checkRemotePortForActivity(const WebCore::MessagePortIdentifier& remoteTarget, CompletionHandler<void(HasActivity)>&& callback) final;
+    void checkRemotePortForActivity(const WebCore::MessagePortIdentifier& remoteTarget, Function<void(HasActivity)>&& callback) final;
     void checkProcessLocalPortForActivity(const WebCore::MessagePortIdentifier&, WebCore::ProcessIdentifier, CompletionHandler<void(HasActivity)>&&) final;
 
     WebCore::MessagePortChannelRegistry m_registry;
index 4ba3c08..6d2856a 100644 (file)
@@ -128,17 +128,15 @@ void WebMessagePortChannelProvider::checkProcessLocalPortForActivity(const Messa
     ASSERT_NOT_REACHED();
 }
 
-void WebMessagePortChannelProvider::checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, CompletionHandler<void(HasActivity)>&& completionHandler)
+void WebMessagePortChannelProvider::checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, Function<void(HasActivity)>&& completionHandler)
 {
     static std::atomic<uint64_t> currentHandlerIdentifier;
     uint64_t identifier = ++currentHandlerIdentifier;
 
     {
         Locker<Lock> locker(m_remoteActivityCallbackLock);
-        auto result = m_remoteActivityCallbacks.ensure(identifier, [completionHandler = WTFMove(completionHandler)]() mutable {
-            return WTFMove(completionHandler);
-        });
-        ASSERT_UNUSED(result, result.isNewEntry);
+        ASSERT(!m_remoteActivityCallbacks.contains(identifier));
+        m_remoteActivityCallbacks.set(identifier, WTFMove(completionHandler));
     }
 
     WebProcess::singleton().parentProcessConnection()->send(Messages::WebProcessProxy::CheckRemotePortForActivity(remoteTarget, identifier), 0);
index 6c9cf86..533be84 100644 (file)
@@ -52,12 +52,12 @@ private:
     void checkProcessLocalPortForActivity(const WebCore::MessagePortIdentifier&, WebCore::ProcessIdentifier, CompletionHandler<void(HasActivity)>&&) final;
 
     // To be called only in the UI process
-    void checkRemotePortForActivity(const WebCore::MessagePortIdentifier& remoteTarget, CompletionHandler<void(HasActivity)>&& callback) final;
+    void checkRemotePortForActivity(const WebCore::MessagePortIdentifier& remoteTarget, Function<void(HasActivity)>&& callback) final;
 
     Lock m_takeAllMessagesCallbackLock;
-    HashMap<uint64_t, CompletionHandler<void(Vector<WebCore::MessageWithMessagePorts>&&, Function<void()>&&)>> m_takeAllMessagesCallbacks;
+    HashMap<uint64_t, Function<void(Vector<WebCore::MessageWithMessagePorts>&&, Function<void()>&&)>> m_takeAllMessagesCallbacks;
     Lock m_remoteActivityCallbackLock;
-    HashMap<uint64_t, CompletionHandler<void(HasActivity)>> m_remoteActivityCallbacks;
+    HashMap<uint64_t, Function<void(HasActivity)>> m_remoteActivityCallbacks;
 };
 
 } // namespace WebKit