Attachment 373549 Details for Bug 199516 – Patch

[patch] Patch

bug-199516-20190705155708.patch (text/plain), 17.46 KB, created by Chris Dumez on 2019-07-05 15:57:10 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-07-05 15:57:10 PDT

Size: 17.46 KB

patch

obsolete

>Subversion Revision: 247182
>diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
>index 8938a9905328a6f5a02cb7d80b2a79127ee40ccb..50dbf3eec59b2b2948406965d25f21b530973779 100644
>--- a/Source/WTF/ChangeLog
>+++ b/Source/WTF/ChangeLog
>@@ -1,3 +1,17 @@
>+2019-07-05  Chris Dumez  <cdumez@apple.com>
>+
>+        Add threading assertion to WTF::CompletionHandler
>+        https://bugs.webkit.org/show_bug.cgi?id=199516
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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-05  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Unreviewed, rolling out r247115.
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 8070eb5295f3ecf81ac26b8fad4c8203467efacf..b693d0e1dafbe998b455ee27b9fe63c4022f3a9c 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,26 @@
>+2019-07-05  Chris Dumez  <cdumez@apple.com>
>+
>+        Add threading assertion to WTF::CompletionHandler
>+        https://bugs.webkit.org/show_bug.cgi?id=199516
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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-05  Jer Noble  <jer.noble@apple.com>
> 
>         Revert change to block playback when process is ostensibly "suspended".
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index c65bf2c07b6f2fc61b8d3ab178beee012a845cda..46db4583cfce6e1a2cc18137b73f663dcb40895a 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,22 @@
>+2019-07-05  Chris Dumez  <cdumez@apple.com>
>+
>+        Add threading assertion to WTF::CompletionHandler
>+        https://bugs.webkit.org/show_bug.cgi?id=199516
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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-05  Ryosuke Niwa  <rniwa@webkit.org>
> 
>         [iOS] Crash in WebKit::WebPage::positionInformation via Range::startPosition
>diff --git a/Source/WTF/wtf/CompletionHandler.h b/Source/WTF/wtf/CompletionHandler.h
>index fda54a6aad00e87541ae16cc6a161e355ac9ce11..5fdd33249507a02f3df19defcea3cd1e805a8a9d 100644
>--- a/Source/WTF/wtf/CompletionHandler.h
>+++ b/Source/WTF/wtf/CompletionHandler.h
>@@ -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 {
>diff --git a/Source/WebCore/dom/messageports/MessagePortChannel.cpp b/Source/WebCore/dom/messageports/MessagePortChannel.cpp
>index 9cf54974cc5a4c222605d0d0d2af4132c94ff181..a5b60d98bb4a00e8a827e71d3d4f49aa8a4de449 100644
>--- a/Source/WebCore/dom/messageports/MessagePortChannel.cpp
>+++ b/Source/WebCore/dom/messageports/MessagePortChannel.cpp
>@@ -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;
>diff --git a/Source/WebCore/dom/messageports/MessagePortChannel.h b/Source/WebCore/dom/messageports/MessagePortChannel.h
>index 05f6945227dcccd75348fdc551118611af2ce485..cc2994d2f92d90f7a2aee2c3e9afe5e8fac17795 100644
>--- a/Source/WebCore/dom/messageports/MessagePortChannel.h
>+++ b/Source/WebCore/dom/messageports/MessagePortChannel.h
>@@ -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;
> 
>diff --git a/Source/WebCore/dom/messageports/MessagePortChannelProvider.h b/Source/WebCore/dom/messageports/MessagePortChannelProvider.h
>index 0c145a7ce0e0ae4947ce8a420d6ba972c5f8dcb8..238edaadaf5d91a418378567447fd2c2dab56ee3 100644
>--- a/Source/WebCore/dom/messageports/MessagePortChannelProvider.h
>+++ b/Source/WebCore/dom/messageports/MessagePortChannelProvider.h
>@@ -53,7 +53,7 @@ public:
>         Yes,
>         No,
>     };
>-    virtual void checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, CompletionHandler<void(HasActivity)>&& callback) = 0;
>+    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;
>diff --git a/Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp b/Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp
>index e74d99a5e9fab9e5d7d2a6a04b77a9f417afcd2c..0b07c2753a99ce0206d41ccb0ea49ed0416f14a6 100644
>--- a/Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp
>+++ b/Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp
>@@ -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);
>     } };
>diff --git a/Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h b/Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h
>index 28f6ad5bcfb55a2905f7de0952bd5a6334150228..d3554e1aa1df457b1b85d2fb82c7569f989d7b6b 100644
>--- a/Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h
>+++ b/Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h
>@@ -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;
> 
>diff --git a/Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp b/Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp
>index e1d2f82aba4ad17f76a130757f9e7a63aee8416a..1cb35546e33fc6610c46a2d0d9d28368894576ce 100644
>--- a/Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp
>+++ b/Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp
>@@ -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());
> 
>diff --git a/Source/WebCore/dom/messageports/MessagePortChannelRegistry.h b/Source/WebCore/dom/messageports/MessagePortChannelRegistry.h
>index 916e5e05a4128223a8fab416b3b3ab7c66cc703f..531aa9584843da75157611b13b74e410feb673a5 100644
>--- a/Source/WebCore/dom/messageports/MessagePortChannelRegistry.h
>+++ b/Source/WebCore/dom/messageports/MessagePortChannelRegistry.h
>@@ -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&);
> 
>diff --git a/Source/WebKit/UIProcess/UIMessagePortChannelProvider.cpp b/Source/WebKit/UIProcess/UIMessagePortChannelProvider.cpp
>index f990820ec26add28adeb851e227e160a107993bc..fb4c644be2adc77e557bf0e24c86cd97b5a09ee0 100644
>--- a/Source/WebKit/UIProcess/UIMessagePortChannelProvider.cpp
>+++ b/Source/WebKit/UIProcess/UIMessagePortChannelProvider.cpp
>@@ -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();
>diff --git a/Source/WebKit/UIProcess/UIMessagePortChannelProvider.h b/Source/WebKit/UIProcess/UIMessagePortChannelProvider.h
>index f1e0c49c1d3f5aab82f263d921a17d5d575e2d73..6c269604a28cc45635d4b36be35ae66b31064925 100644
>--- a/Source/WebKit/UIProcess/UIMessagePortChannelProvider.h
>+++ b/Source/WebKit/UIProcess/UIMessagePortChannelProvider.h
>@@ -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;
>diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebMessagePortChannelProvider.cpp b/Source/WebKit/WebProcess/WebCoreSupport/WebMessagePortChannelProvider.cpp
>index 4ba3c08c7d58fc6e0475423eb77120f8ffdb5959..6d2856a04eda292f6f6a687c0e16c2102a831551 100644
>--- a/Source/WebKit/WebProcess/WebCoreSupport/WebMessagePortChannelProvider.cpp
>+++ b/Source/WebKit/WebProcess/WebCoreSupport/WebMessagePortChannelProvider.cpp
>@@ -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);
>diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebMessagePortChannelProvider.h b/Source/WebKit/WebProcess/WebCoreSupport/WebMessagePortChannelProvider.h
>index 6c9cf864017250ff3ceb31f28a1f7e16c20c9594..533be840cad714555daf0aa20728a3c93355c9be 100644
>--- a/Source/WebKit/WebProcess/WebCoreSupport/WebMessagePortChannelProvider.h
>+++ b/Source/WebKit/WebProcess/WebCoreSupport/WebMessagePortChannelProvider.h
>@@ -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

Actions: View | Formatted Diff | Diff

Attachments on bug 199516: 373493 | 373497 | 373506 | 373509 | 373513 | 373514 | 373528 | 373530 | 373536 | 373549 | 373641