⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 278395 in webkit


Ignore:
Timestamp:
Jun 2, 2021, 10:09:02 PM (5 years ago)
Author:
Chris Dumez
Message:

Stop using a RefPtr<IPC::Connection> as HashMap key in DisplayLink
​https://bugs.webkit.org/show_bug.cgi?id=226561

Reviewed by Simon Fraser.

Stop using a RefPtr<IPC::Connection> as HashMap key in DisplayLink. Using a RefPtr as key is suboptimal
and could leak to memory leaks. The reason this needed a RefPtr<IPC::Connection> was because we needed
to send IPC from a background thread. To support this, I have added a static IPC::Connection::send()
function that takes an IPC::Connection::UniqueID and that is thread safe. The function looks up the
IPC::Connection from its UniqueID and sends the IPC while still holding the lock.

As a result, DisplayLink can use IPC::Connection::UniqueID as key instead.

Note that I am planning to use the new static IPC::Connection::send() in other cases where we could
send IPC directly from a background thread instead of having to hop to the main thread to look up
the IPC::Connection from its UniqueID. StorageArea::dispatchEvents() is an example of where this will
be useful.

  • Platform/IPC/Connection.cpp:

(IPC::Connection::Connection):
(IPC::Connection::~Connection):

  • Platform/IPC/Connection.h:

(IPC::Connection::send):

  • UIProcess/mac/DisplayLink.cpp:

(WebKit::DisplayLink::addObserver):
(WebKit::DisplayLink::removeObserver):
(WebKit::DisplayLink::removeObservers):
(WebKit::DisplayLink::removeInfoForConnectionIfPossible):
(WebKit::DisplayLink::incrementFullSpeedRequestClientCount):
(WebKit::DisplayLink::decrementFullSpeedRequestClientCount):
(WebKit::DisplayLink::setPreferredFramesPerSecond):
(WebKit::DisplayLink::notifyObserversDisplayWasRefreshed):

  • UIProcess/mac/DisplayLink.h:
Location:
trunk/Source/WebKit
Files:
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r278394 r278395  
     12021-06-02  Chris Dumez  <cdumez@apple.com>
     2
     3        Stop using a RefPtr<IPC::Connection> as HashMap key in DisplayLink
     4        https://bugs.webkit.org/show_bug.cgi?id=226561
     5
     6        Reviewed by Simon Fraser.
     7
     8        Stop using a RefPtr<IPC::Connection> as HashMap key in DisplayLink. Using a RefPtr as key is suboptimal
     9        and could leak to memory leaks. The reason this needed a RefPtr<IPC::Connection> was because we needed
     10        to send IPC from a background thread. To support this, I have added a static IPC::Connection::send()
     11        function that takes an IPC::Connection::UniqueID and that is thread safe. The function looks up the
     12        IPC::Connection from its UniqueID and sends the IPC while still holding the lock.
     13
     14        As a result, DisplayLink can use IPC::Connection::UniqueID as key instead.
     15
     16        Note that I am planning to use the new static IPC::Connection::send() in other cases where we could
     17        send IPC directly from a background thread instead of having to hop to the main thread to look up
     18        the IPC::Connection from its UniqueID. StorageArea::dispatchEvents() is an example of where this will
     19        be useful.
     20
     21        * Platform/IPC/Connection.cpp:
     22        (IPC::Connection::Connection):
     23        (IPC::Connection::~Connection):
     24        * Platform/IPC/Connection.h:
     25        (IPC::Connection::send):
     26        * UIProcess/mac/DisplayLink.cpp:
     27        (WebKit::DisplayLink::addObserver):
     28        (WebKit::DisplayLink::removeObserver):
     29        (WebKit::DisplayLink::removeObservers):
     30        (WebKit::DisplayLink::removeInfoForConnectionIfPossible):
     31        (WebKit::DisplayLink::incrementFullSpeedRequestClientCount):
     32        (WebKit::DisplayLink::decrementFullSpeedRequestClientCount):
     33        (WebKit::DisplayLink::setPreferredFramesPerSecond):
     34        (WebKit::DisplayLink::notifyObserversDisplayWasRefreshed):
     35        * UIProcess/mac/DisplayLink.h:
     36
    1372021-06-02  Wenson Hsieh  <wenson_hsieh@apple.com>
    238

  • trunk/Source/WebKit/Platform/IPC/Connection.cpp

    r277958 r278395  
    5858std::atomic<unsigned> UnboundedSynchronousIPCScope::unboundedSynchronousIPCCount = 0;
    5959
     60Lock Connection::s_connectionMapLock;
     61
    6062struct Connection::WaitForMessageState {
    6163    WaitForMessageState(MessageName messageName, uint64_t destinationID, OptionSet<WaitForOption> waitForOptions)
    … …  
    269271}
    270272
    271 static HashMap<IPC::Connection::UniqueID, Connection*>& allConnections()
     273HashMap<IPC::Connection::UniqueID, Connection*>& Connection::connectionMap()
    272274{
    273275    static NeverDestroyed<HashMap<IPC::Connection::UniqueID, Connection*>> map;
    … …  
    292294{
    293295    ASSERT(RunLoop::isMain());
    294     allConnections().add(m_uniqueID, this);
     296
     297    {
     298        Locker locker { s_connectionMapLock };
     299        connectionMap().add(m_uniqueID, this);
     300    }
    295301
    296302    platformInitialize(identifier);
    … …  
    302308    ASSERT(!isValid());
    303309
    304     allConnections().remove(m_uniqueID);
     310    {
     311        Locker locker { s_connectionMapLock };
     312        connectionMap().remove(m_uniqueID);
     313    }
    305314
    306315    clearAsyncReplyHandlers(*this);
    307316}
    308317
    309 Connection* Connection::connection(UniqueID uniqueID)
     318// WTF_IGNORES_THREAD_SAFETY_ANALYSIS because this function accesses connectionMap() without locking.
     319// It is safe because this function is only called on the main thread and Connection objects are only
     320// constructed / destroyed on the main thread.
     321Connection* Connection::connection(UniqueID uniqueID) WTF_IGNORES_THREAD_SAFETY_ANALYSIS
    310322{
    311323    ASSERT(RunLoop::isMain());
    312     return allConnections().get(uniqueID);
     324    return connectionMap().get(uniqueID);
    313325}
    314326

  • trunk/Source/WebKit/Platform/IPC/Connection.h

    r278253 r278395  
    244244    template<typename T, typename C> uint64_t sendWithAsyncReply(T&& message, C&& completionHandler, uint64_t destinationID = 0, OptionSet<SendOption> = { }); // Thread-safe.
    245245    template<typename T> bool send(T&& message, uint64_t destinationID, OptionSet<SendOption> sendOptions = { }); // Thread-safe.
     246    template<typename T> static bool send(UniqueID, T&& message, uint64_t destinationID, OptionSet<SendOption> sendOptions = { }); // Thread-safe.
     247
    246248    // Sync senders should check the SendSyncResult for true/false in case they need to know if the result was really received.
    247249    // Sync senders should hold on to the SendSyncResult in case they reference the contents of the reply via DataRefererence / ArrayReference.
    … …  
    326328
    327329    bool isIncomingMessagesThrottlingEnabled() const { return !!m_incomingMessagesThrottler; }
     330
     331    static HashMap<IPC::Connection::UniqueID, Connection*>& connectionMap() WTF_REQUIRES_LOCK(s_connectionMapLock);
    328332   
    329333    std::unique_ptr<Decoder> waitForMessage(MessageName, uint64_t destinationID, Timeout, OptionSet<WaitForOption>);
    … …  
    383387    };
    384388
     389    static Lock s_connectionMapLock;
    385390    Client& m_client;
    386391    UniqueID m_uniqueID;
    … …  
    514519   
    515520    return sendMessage(WTFMove(encoder), sendOptions);
     521}
     522
     523template<typename T>
     524bool Connection::send(UniqueID connectionID, T&& message, uint64_t destinationID, OptionSet<SendOption> sendOptions)
     525{
     526    Locker locker { s_connectionMapLock };
     527    auto* connection = connectionMap().get(connectionID);
     528    if (!connection)
     529        return false;
     530    return connection->send(WTFMove(message), destinationID, sendOptions);
    516531}
    517532

  • trunk/Source/WebKit/UIProcess/mac/DisplayLink.cpp

    r278253 r278395  
    9191    {
    9292        Locker locker { m_observersLock };
    93         m_observers.ensure(&connection, [] {
     93        m_observers.ensure(connection.uniqueID(), [] {
    9494            return ConnectionClientInfo { };
    9595        }).iterator->value.observers.append({ observerID, preferredFramesPerSecond });
    … …  
    112112    Locker locker { m_observersLock };
    113113
    114     auto it = m_observers.find(&connection);
     114    auto it = m_observers.find(connection.uniqueID());
    115115    if (it == m_observers.end())
    116116        return;
    … …  
    137137
    138138    Locker locker { m_observersLock };
    139     m_observers.remove(&connection);
     139    m_observers.remove(connection.uniqueID());
    140140
    141141    // We do not stop the display link right away when |m_observers| becomes empty. Instead, we
    … …  
    146146void DisplayLink::removeInfoForConnectionIfPossible(IPC::Connection& connection)
    147147{
    148     auto it = m_observers.find(&connection);
     148    auto it = m_observers.find(connection.uniqueID());
    149149    if (it == m_observers.end())
    150150        return;
    … …  
    159159    Locker locker { m_observersLock };
    160160
    161     auto& connectionInfo = m_observers.ensure(&connection, [] {
     161    auto& connectionInfo = m_observers.ensure(connection.uniqueID(), [] {
    162162        return ConnectionClientInfo { };
    163163    }).iterator->value;
    … …  
    170170    Locker locker { m_observersLock };
    171171
    172     auto it = m_observers.find(&connection);
     172    auto it = m_observers.find(connection.uniqueID());
    173173    if (it == m_observers.end())
    174174        return;
    … …  
    186186    Locker locker { m_observersLock };
    187187
    188     auto it = m_observers.find(&connection);
     188    auto it = m_observers.find(connection.uniqueID());
    189189    if (it == m_observers.end())
    190190        return;
    … …  
    219219
    220220    bool anyConnectionHadObservers = false;
    221     for (auto& [connection, connectionInfo] : m_observers) {
     221    for (auto& [connectionID, connectionInfo] : m_observers) {
    222222        if (connectionInfo.observers.isEmpty())
    223223            continue;
    … …  
    232232
    233233        if (connectionInfo.fullSpeedUpdatesClientCount) {
    234             connection->send(Messages::EventDispatcher::DisplayWasRefreshed(m_displayID, m_currentUpdate, mainThreadWantsUpdate), 0);
     234            IPC::Connection::send(connectionID, Messages::EventDispatcher::DisplayWasRefreshed(m_displayID, m_currentUpdate, mainThreadWantsUpdate), 0);
    235235        } else if (mainThreadWantsUpdate)
    236             connection->send(Messages::WebProcess::DisplayWasRefreshed(m_displayID, m_currentUpdate), 0);
     236            IPC::Connection::send(connectionID, Messages::WebProcess::DisplayWasRefreshed(m_displayID, m_currentUpdate), 0);
    237237    }
    238238

  • trunk/Source/WebKit/UIProcess/mac/DisplayLink.h

    r277958 r278395  
    2828#if HAVE(CVDISPLAYLINK)
    2929
     30#include "Connection.h"
    3031#include "DisplayLinkObserverID.h"
    3132#include <CoreVideo/CVDisplayLink.h>
    … …  
    8586    CVDisplayLinkRef m_displayLink { nullptr };
    8687    Lock m_observersLock;
    87     HashMap<RefPtr<IPC::Connection>, ConnectionClientInfo> m_observers WTF_GUARDED_BY_LOCK(m_observersLock);
     88    HashMap<IPC::Connection::UniqueID, ConnectionClientInfo> m_observers WTF_GUARDED_BY_LOCK(m_observersLock);
    8889    WebCore::PlatformDisplayID m_displayID;
    8990    WebCore::FramesPerSecond m_displayNominalFramesPerSecond { 0 };
Note: See TracChangeset for help on using the changeset viewer.