Changeset 247218 in webkit


Ignore:
Timestamp:
Jul 8, 2019 11:26:47 AM (5 years ago)
Author:
Chris Dumez
Message:

Add threading assertion to WTF::CompletionHandler
​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):

Location:
trunk/Source
Files:
15 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WTF/ChangeLog

    r247205 r247218  
     12019-07-08  Chris Dumez  <cdumez@apple.com>
     2
     3        Add threading assertion to WTF::CompletionHandler
     4        https://bugs.webkit.org/show_bug.cgi?id=199516
     5
     6        Reviewed by Alex Christensen.
     7
     8        Add threading assertion to WTF::CompletionHandler to try and catch common cases
     9        of unsafe usage (completion handler constructed on one thread but called on
     10        another).
     11
     12        * wtf/CompletionHandler.h:
     13        (WTF::CompletionHandler<Out):
     14
    1152019-07-08  Antoine Quint  <graouts@apple.com>
    216

  • trunk/Source/WTF/wtf/CompletionHandler.h

    r245024 r247218  
    2727
    2828#include <wtf/Function.h>
     29#include <wtf/MainThread.h>
    2930
    3031namespace WTF {
    … …  
    4142    CompletionHandler(CallableType&& callable)
    4243        : m_function(WTFMove(callable))
     44#if !ASSERT_DISABLED
     45        , m_wasConstructedOnMainThread(isMainThread())
     46#endif
    4347    {
    4448    }
    … …  
    5660    Out operator()(In... in)
    5761    {
     62        ASSERT(m_wasConstructedOnMainThread == isMainThread());
    5863        ASSERT_WITH_MESSAGE(m_function, "Completion handler should not be called more than once");
    5964        return std::exchange(m_function, nullptr)(std::forward<In>(in)...);
    … …  
    6267private:
    6368    Function<Out(In...)> m_function;
     69#if !ASSERT_DISABLED
     70    bool m_wasConstructedOnMainThread;
     71#endif
    6472};
    6573

  • trunk/Source/WebCore/ChangeLog

    r247215 r247218  
     12019-07-08  Chris Dumez  <cdumez@apple.com>
     2
     3        Add threading assertion to WTF::CompletionHandler
     4        https://bugs.webkit.org/show_bug.cgi?id=199516
     5
     6        Reviewed by Alex Christensen.
     7
     8        Update some MessagePort-related code to use WTF::Function instead of WTF::CompletionHandler
     9        since the callback is always called on the main thread, even when it was created on a
     10        worker thread. Ideally, this code would be refactored so that the callback gets called on
     11        the worker thread directly.
     12
     13        * dom/messageports/MessagePortChannel.cpp:
     14        (WebCore::MessagePortChannel::checkRemotePortForActivity):
     15        * dom/messageports/MessagePortChannel.h:
     16        * dom/messageports/MessagePortChannelProvider.h:
     17        * dom/messageports/MessagePortChannelProviderImpl.cpp:
     18        (WebCore::MessagePortChannelProviderImpl::checkRemotePortForActivity):
     19        * dom/messageports/MessagePortChannelProviderImpl.h:
     20        * dom/messageports/MessagePortChannelRegistry.cpp:
     21        (WebCore::MessagePortChannelRegistry::checkRemotePortForActivity):
     22        * dom/messageports/MessagePortChannelRegistry.h:
     23
    1242019-07-08  Charlie Turner  <cturner@igalia.com>
    225

  • trunk/Source/WebCore/dom/messageports/MessagePortChannel.cpp

    r239427 r247218  
    183183}
    184184
    185 void MessagePortChannel::checkRemotePortForActivity(const MessagePortIdentifier& remotePort, CompletionHandler<void(MessagePortChannelProvider::HasActivity)>&& callback)
     185void MessagePortChannel::checkRemotePortForActivity(const MessagePortIdentifier& remotePort, Function<void(MessagePortChannelProvider::HasActivity)>&& callback)
    186186{
    187187    ASSERT(isMainThread());
    … …  
    208208    }
    209209
    210     auto outerCallback = CompletionHandler<void(MessagePortChannelProvider::HasActivity)> { [this, protectedThis = makeRef(*this), callback = WTFMove(callback)] (MessagePortChannelProvider::HasActivity hasActivity) mutable {
     210    auto outerCallback = Function<void(MessagePortChannelProvider::HasActivity)> { [this, protectedThis = makeRef(*this), callback = WTFMove(callback)] (MessagePortChannelProvider::HasActivity hasActivity) mutable {
    211211        if (hasActivity == MessagePortChannelProvider::HasActivity::Yes) {
    212212            callback(hasActivity);

  • trunk/Source/WebCore/dom/messageports/MessagePortChannel.h

    r240640 r247218  
    5555
    5656    void takeAllMessagesForPort(const MessagePortIdentifier&, Function<void(Vector<MessageWithMessagePorts>&&, Function<void()>&&)>&&);
    57     void checkRemotePortForActivity(const MessagePortIdentifier&, CompletionHandler<void(MessagePortChannelProvider::HasActivity)>&& callback);
     57    void checkRemotePortForActivity(const MessagePortIdentifier&, Function<void(MessagePortChannelProvider::HasActivity)>&& callback);
    5858
    5959    WEBCORE_EXPORT bool hasAnyMessagesPendingOrInFlight() const;

  • trunk/Source/WebCore/dom/messageports/MessagePortChannelProvider.h

    r240640 r247218  
    4747    virtual void messagePortDisentangled(const MessagePortIdentifier& local) = 0;
    4848    virtual void messagePortClosed(const MessagePortIdentifier& local) = 0;
     49   
     50    // FIXME: Ideally the callback would be a CompletionHandler but it is always called on the caller's
     51    // thread at the moment.
    4952    virtual void takeAllMessagesForPort(const MessagePortIdentifier&, Function<void(Vector<MessageWithMessagePorts>&&, Function<void()>&&)>&&) = 0;
     53
    5054    virtual void postMessageToRemote(MessageWithMessagePorts&&, const MessagePortIdentifier& remoteTarget) = 0;
    5155
    … …  
    5458        No,
    5559    };
    56     virtual void checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, CompletionHandler<void(HasActivity)>&& callback) = 0;
     60    // FIXME: Ideally the callback would be a CompletionHandler but it is always called on the caller's
     61    // thread at the moment.
     62    virtual void checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, Function<void(HasActivity)>&& callback) = 0;
    5763
    5864    // Operations that the coordinating process performs (e.g. the UIProcess)

  • trunk/Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp

    r234332 r247218  
    101101}
    102102
    103 void MessagePortChannelProviderImpl::checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, CompletionHandler<void(HasActivity)>&& outerCallback)
     103void MessagePortChannelProviderImpl::checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, Function<void(HasActivity)>&& outerCallback)
    104104{
    105     auto callback = CompletionHandler<void(HasActivity)> { [outerCallback = WTFMove(outerCallback)](HasActivity hasActivity) mutable {
     105    auto callback = Function<void(HasActivity)> { [outerCallback = WTFMove(outerCallback)](HasActivity hasActivity) mutable {
    106106        ASSERT(isMainThread());
    107107        outerCallback(hasActivity);

  • trunk/Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h

    r227737 r247218  
    4343    void postMessageToRemote(MessageWithMessagePorts&&, const MessagePortIdentifier& remoteTarget) final;
    4444    void takeAllMessagesForPort(const MessagePortIdentifier&, Function<void(Vector<MessageWithMessagePorts>&&, Function<void()>&&)>&&) final;
    45     void checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, CompletionHandler<void(HasActivity)>&& callback) final;
     45    void checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, Function<void(HasActivity)>&& callback) final;
    4646
    4747    void checkProcessLocalPortForActivity(const MessagePortIdentifier&, ProcessIdentifier, CompletionHandler<void(HasActivity)>&&) final;

  • trunk/Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp

    r237828 r247218  
    158158}
    159159
    160 void MessagePortChannelRegistry::checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, CompletionHandler<void(MessagePortChannelProvider::HasActivity)>&& callback)
     160void MessagePortChannelRegistry::checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, Function<void(MessagePortChannelProvider::HasActivity)>&& callback)
    161161{
    162162    ASSERT(isMainThread());

  • trunk/Source/WebCore/dom/messageports/MessagePortChannelRegistry.h

    r240640 r247218  
    4545    WEBCORE_EXPORT bool didPostMessageToRemote(MessageWithMessagePorts&&, const MessagePortIdentifier& remoteTarget);
    4646    WEBCORE_EXPORT void takeAllMessagesForPort(const MessagePortIdentifier&, Function<void(Vector<MessageWithMessagePorts>&&, Function<void()>&&)>&&);
    47     WEBCORE_EXPORT void checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, CompletionHandler<void(MessagePortChannelProvider::HasActivity)>&& callback);
     47    WEBCORE_EXPORT void checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, Function<void(MessagePortChannelProvider::HasActivity)>&& callback);
    4848
    4949    WEBCORE_EXPORT MessagePortChannel* existingChannelContainingPort(const MessagePortIdentifier&);

  • trunk/Source/WebKit/ChangeLog

    r247212 r247218  
     12019-07-08  Chris Dumez  <cdumez@apple.com>
     2
     3        Add threading assertion to WTF::CompletionHandler
     4        https://bugs.webkit.org/show_bug.cgi?id=199516
     5
     6        Reviewed by Alex Christensen.
     7
     8        Update some MessagePort-related code to use WTF::Function instead of WTF::CompletionHandler
     9        since the callback is always called on the main thread, even when it was created on a
     10        worker thread. Ideally, this code would be refactored so that the callback gets called on
     11        the worker thread directly.
     12
     13        * UIProcess/UIMessagePortChannelProvider.cpp:
     14        (WebKit::UIMessagePortChannelProvider::checkRemotePortForActivity):
     15        * UIProcess/UIMessagePortChannelProvider.h:
     16        * WebProcess/WebCoreSupport/WebMessagePortChannelProvider.cpp:
     17        (WebKit::WebMessagePortChannelProvider::checkRemotePortForActivity):
     18        * WebProcess/WebCoreSupport/WebMessagePortChannelProvider.h:
     19
    1202019-07-08  Antoine Quint  <graouts@apple.com>
    221

  • trunk/Source/WebKit/UIProcess/UIMessagePortChannelProvider.cpp

    r235265 r247218  
    8686}
    8787
    88 void UIMessagePortChannelProvider::checkRemotePortForActivity(const MessagePortIdentifier&, CompletionHandler<void(HasActivity)>&&)
     88void UIMessagePortChannelProvider::checkRemotePortForActivity(const MessagePortIdentifier&, Function<void(HasActivity)>&&)
    8989{
    9090    // Should never be called in the UI process provider.

  • trunk/Source/WebKit/UIProcess/UIMessagePortChannelProvider.h

    r227340 r247218  
    4646    void takeAllMessagesForPort(const WebCore::MessagePortIdentifier&, Function<void(Vector<WebCore::MessageWithMessagePorts>&&, Function<void()>&&)>&&) final;
    4747    void postMessageToRemote(WebCore::MessageWithMessagePorts&&, const WebCore::MessagePortIdentifier& remoteTarget) final;
    48     void checkRemotePortForActivity(const WebCore::MessagePortIdentifier& remoteTarget, CompletionHandler<void(HasActivity)>&& callback) final;
     48    void checkRemotePortForActivity(const WebCore::MessagePortIdentifier& remoteTarget, Function<void(HasActivity)>&& callback) final;
    4949    void checkProcessLocalPortForActivity(const WebCore::MessagePortIdentifier&, WebCore::ProcessIdentifier, CompletionHandler<void(HasActivity)>&&) final;
    5050

  • trunk/Source/WebKit/WebProcess/WebCoreSupport/WebMessagePortChannelProvider.cpp

    r235205 r247218  
    129129}
    130130
    131 void WebMessagePortChannelProvider::checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, CompletionHandler<void(HasActivity)>&& completionHandler)
     131void WebMessagePortChannelProvider::checkRemotePortForActivity(const MessagePortIdentifier& remoteTarget, Function<void(HasActivity)>&& completionHandler)
    132132{
    133133    static std::atomic<uint64_t> currentHandlerIdentifier;
    … …  
    136136    {
    137137        Locker<Lock> locker(m_remoteActivityCallbackLock);
    138         auto result = m_remoteActivityCallbacks.ensure(identifier, [completionHandler = WTFMove(completionHandler)]() mutable {
    139             return WTFMove(completionHandler);
    140         });
    141         ASSERT_UNUSED(result, result.isNewEntry);
     138        ASSERT(!m_remoteActivityCallbacks.contains(identifier));
     139        m_remoteActivityCallbacks.set(identifier, WTFMove(completionHandler));
    142140    }
    143141

  • trunk/Source/WebKit/WebProcess/WebCoreSupport/WebMessagePortChannelProvider.h

    r227340 r247218  
    5353
    5454    // To be called only in the UI process
    55     void checkRemotePortForActivity(const WebCore::MessagePortIdentifier& remoteTarget, CompletionHandler<void(HasActivity)>&& callback) final;
     55    void checkRemotePortForActivity(const WebCore::MessagePortIdentifier& remoteTarget, Function<void(HasActivity)>&& callback) final;
    5656
    5757    Lock m_takeAllMessagesCallbackLock;
    58     HashMap<uint64_t, CompletionHandler<void(Vector<WebCore::MessageWithMessagePorts>&&, Function<void()>&&)>> m_takeAllMessagesCallbacks;
     58    HashMap<uint64_t, Function<void(Vector<WebCore::MessageWithMessagePorts>&&, Function<void()>&&)>> m_takeAllMessagesCallbacks;
    5959    Lock m_remoteActivityCallbackLock;
    60     HashMap<uint64_t, CompletionHandler<void(HasActivity)>> m_remoteActivityCallbacks;
     60    HashMap<uint64_t, Function<void(HasActivity)>> m_remoteActivityCallbacks;
    6161};
    6262
Note: See TracChangeset for help on using the changeset viewer.