Changeset 229028 in webkit


Ignore:
Timestamp:
Feb 26, 2018 10:01:12 AM (6 years ago)
Author:
commit-queue@webkit.org
Message:

MessagePort is not always destroyed in the right thread
https://bugs.webkit.org/show_bug.cgi?id=183053

Patch by Youenn Fablet <youenn@apple.com> on 2018-02-26
Reviewed by Chris Dumez.

Source/WebCore:

Make existingMessagePortForIdentifier take a lambda so that we hold the lock until there
is no longer a need to keep the MessagePort around.
This is very time sensitive and does not happen a lot when running WPT tests.

Update existing call sites to pass a lambda.

  • dom/MessagePort.cpp:

(WebCore::MessagePort::existingMessagePortForIdentifier):

  • dom/MessagePort.h:
  • dom/messageports/MessagePortChannelProviderImpl.cpp:

(WebCore::MessagePortChannelProviderImpl::postMessageToRemote):
(WebCore::MessagePortChannelProviderImpl::checkProcessLocalPortForActivity):

Source/WebKit:

Update code to pass a lambda to MessagePort::existingMessagePortForIdentifier.

  • WebProcess/WebCoreSupport/WebMessagePortChannelProvider.cpp:

(WebKit::WebMessagePortChannelProvider::checkProcessLocalPortForActivity):

  • WebProcess/WebProcess.cpp:

(WebKit::WebProcess::messagesAvailableForPort):

Location:
trunk/Source
Files:
7 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebCore/ChangeLog

    r229027 r229028  
     12018-02-26  Youenn Fablet  <youenn@apple.com>
     2
     3        MessagePort is not always destroyed in the right thread
     4        https://bugs.webkit.org/show_bug.cgi?id=183053
     5
     6        Reviewed by Chris Dumez.
     7
     8        Make existingMessagePortForIdentifier take a lambda so that we hold the lock until there
     9        is no longer a need to keep the MessagePort around.
     10        This is very time sensitive and does not happen a lot when running WPT tests.
     11
     12        Update existing call sites to pass a lambda.
     13
     14        * dom/MessagePort.cpp:
     15        (WebCore::MessagePort::existingMessagePortForIdentifier):
     16        * dom/MessagePort.h:
     17        * dom/messageports/MessagePortChannelProviderImpl.cpp:
     18        (WebCore::MessagePortChannelProviderImpl::postMessageToRemote):
     19        (WebCore::MessagePortChannelProviderImpl::checkProcessLocalPortForActivity):
     20
    1212018-02-26  Commit Queue  <commit-queue@webkit.org>
    222
  • trunk/Source/WebCore/dom/MessagePort.cpp

    r228260 r229028  
    5757void MessagePort::deref() const
    5858{
    59     // MessagePort::existingMessagePortForIdentifier() is unique in that it holds a raw pointer to a MessagePort
    60     // but might create a RefPtr from it.
    61     // If that happens on one thread at the same time that a MessagePort is being deref'ed and destroyed on a
    62     // different thread then Bad Things could happen.
    63     // This custom deref() function is designed to handle that contention by guaranteeing that nobody can be
    64     // creating a RefPtr inside existingMessagePortForIdentifier while the object is mid-deletion.
     59    // This custom deref() function ensures that as long as the lock to allMessagePortsLock is taken, no MessagePort will be destroyed.
     60    // This allows isExistingMessagePortLocallyReachable and notifyMessageAvailable to easily query the map and manipulate MessagePort instances.
    6561
    6662    if (!--m_refCount) {
     
    7571}
    7672
    77 RefPtr<MessagePort> MessagePort::existingMessagePortForIdentifier(const MessagePortIdentifier& identifier)
     73bool MessagePort::isExistingMessagePortLocallyReachable(const MessagePortIdentifier& identifier)
    7874{
    7975    Locker<Lock> locker(allMessagePortsLock());
    80 
    81     return allMessagePorts().get(identifier);
     76    auto* port = allMessagePorts().get(identifier);
     77    return port && port->isLocallyReachable();
     78}
     79
     80void MessagePort::notifyMessageAvailable(const MessagePortIdentifier& identifier)
     81{
     82    Locker<Lock> locker(allMessagePortsLock());
     83    if (auto* port = allMessagePorts().get(identifier))
     84        port->messageAvailable();
     85
    8286}
    8387
  • trunk/Source/WebCore/dom/MessagePort.h

    r227340 r229028  
    5858    static ExceptionOr<TransferredMessagePortArray> disentanglePorts(Vector<RefPtr<MessagePort>>&&);
    5959    static Vector<RefPtr<MessagePort>> entanglePorts(ScriptExecutionContext&, TransferredMessagePortArray&&);
    60     WEBCORE_EXPORT static RefPtr<MessagePort> existingMessagePortForIdentifier(const MessagePortIdentifier&);
     60
     61    WEBCORE_EXPORT static bool isExistingMessagePortLocallyReachable(const MessagePortIdentifier&);
     62    WEBCORE_EXPORT static void notifyMessageAvailable(const MessagePortIdentifier&);
    6163
    6264    WEBCORE_EXPORT void messageAvailable();
  • trunk/Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp

    r227737 r229028  
    8383{
    8484    performActionOnMainThread([registry = &m_registry, message = WTFMove(message), remoteTarget]() mutable {
    85         bool wasFirstMessageInQueue = registry->didPostMessageToRemote(WTFMove(message), remoteTarget);
    86         if (wasFirstMessageInQueue) {
    87             if (auto remotePort = MessagePort::existingMessagePortForIdentifier(remoteTarget))
    88                 remotePort->messageAvailable();
    89         }
     85        if (registry->didPostMessageToRemote(WTFMove(message), remoteTarget))
     86            MessagePort::notifyMessageAvailable(remoteTarget);
    9087    });
    9188}
     
    120117    ASSERT(isMainThread());
    121118
    122     auto port = MessagePort::existingMessagePortForIdentifier(identifier);
    123     if (!port) {
    124         callback(MessagePortChannelProvider::HasActivity::No);
    125         return;
    126     }
    127 
    128     callback(port->isLocallyReachable() ? HasActivity::Yes : HasActivity::No);
     119    callback(MessagePort::isExistingMessagePortLocallyReachable(identifier) ? HasActivity::Yes : HasActivity::No);
    129120}
    130121
  • trunk/Source/WebKit/ChangeLog

    r228982 r229028  
     12018-02-26  Youenn Fablet  <youenn@apple.com>
     2
     3        MessagePort is not always destroyed in the right thread
     4        https://bugs.webkit.org/show_bug.cgi?id=183053
     5
     6        Reviewed by Chris Dumez.
     7
     8        Update code to pass a lambda to MessagePort::existingMessagePortForIdentifier.
     9
     10        * WebProcess/WebCoreSupport/WebMessagePortChannelProvider.cpp:
     11        (WebKit::WebMessagePortChannelProvider::checkProcessLocalPortForActivity):
     12        * WebProcess/WebProcess.cpp:
     13        (WebKit::WebProcess::messagesAvailableForPort):
     14
    1152018-02-25  Alexey Proskuryakov  <ap@apple.com>
    216
  • trunk/Source/WebKit/WebProcess/WebCoreSupport/WebMessagePortChannelProvider.cpp

    r227340 r229028  
    121121void WebMessagePortChannelProvider::checkProcessLocalPortForActivity(const MessagePortIdentifier& identifier, uint64_t callbackIdentifier)
    122122{
    123     auto port = MessagePort::existingMessagePortForIdentifier(identifier);
    124     WebProcess::singleton().parentProcessConnection()->send(Messages::WebProcessProxy::DidCheckProcessLocalPortForActivity(callbackIdentifier, port && port->isLocallyReachable()), 0);
     123    WebProcess::singleton().parentProcessConnection()->send(Messages::WebProcessProxy::DidCheckProcessLocalPortForActivity { callbackIdentifier, MessagePort::isExistingMessagePortLocallyReachable(identifier) }, 0);
    125124}
    126125
  • trunk/Source/WebKit/WebProcess/WebProcess.cpp

    r228982 r229028  
    10611061void WebProcess::messagesAvailableForPort(const MessagePortIdentifier& identifier)
    10621062{
    1063     auto port = MessagePort::existingMessagePortForIdentifier(identifier);
    1064     if (port)
    1065         port->messageAvailable();
     1063    MessagePort::notifyMessageAvailable(identifier);
    10661064}
    10671065
Note: See TracChangeset for help on using the changeset viewer.