- Revision
- 278395
- Author
- cdu...@apple.com
- Date
- 2021-06-02 22:09:02 -0700 (Wed, 02 Jun 2021)
Log 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:
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (278394 => 278395)
--- trunk/Source/WebKit/ChangeLog 2021-06-03 04:59:05 UTC (rev 278394)
+++ trunk/Source/WebKit/ChangeLog 2021-06-03 05:09:02 UTC (rev 278395)
@@ -1,3 +1,39 @@
+2021-06-02 Chris Dumez <cdu...@apple.com>
+
+ 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:
+
2021-06-02 Wenson Hsieh <wenson_hs...@apple.com>
[iOS] Show data detector context menu on long press inside image overlays
Modified: trunk/Source/WebKit/Platform/IPC/Connection.cpp (278394 => 278395)
--- trunk/Source/WebKit/Platform/IPC/Connection.cpp 2021-06-03 04:59:05 UTC (rev 278394)
+++ trunk/Source/WebKit/Platform/IPC/Connection.cpp 2021-06-03 05:09:02 UTC (rev 278395)
@@ -57,6 +57,8 @@
std::atomic<unsigned> UnboundedSynchronousIPCScope::unboundedSynchronousIPCCount = 0;
+Lock Connection::s_connectionMapLock;
+
struct Connection::WaitForMessageState {
WaitForMessageState(MessageName messageName, uint64_t destinationID, OptionSet<WaitForOption> waitForOptions)
: messageName(messageName)
@@ -268,7 +270,7 @@
return adoptRef(*new Connection(identifier, false, client));
}
-static HashMap<IPC::Connection::UniqueID, Connection*>& allConnections()
+HashMap<IPC::Connection::UniqueID, Connection*>& Connection::connectionMap()
{
static NeverDestroyed<HashMap<IPC::Connection::UniqueID, Connection*>> map;
return map;
@@ -291,8 +293,12 @@
, m_connectionQueue(WorkQueue::create("com.apple.IPC.ReceiveQueue"))
{
ASSERT(RunLoop::isMain());
- allConnections().add(m_uniqueID, this);
+ {
+ Locker locker { s_connectionMapLock };
+ connectionMap().add(m_uniqueID, this);
+ }
+
platformInitialize(identifier);
}
@@ -301,15 +307,21 @@
ASSERT(RunLoop::isMain());
ASSERT(!isValid());
- allConnections().remove(m_uniqueID);
+ {
+ Locker locker { s_connectionMapLock };
+ connectionMap().remove(m_uniqueID);
+ }
clearAsyncReplyHandlers(*this);
}
-Connection* Connection::connection(UniqueID uniqueID)
+// WTF_IGNORES_THREAD_SAFETY_ANALYSIS because this function accesses connectionMap() without locking.
+// It is safe because this function is only called on the main thread and Connection objects are only
+// constructed / destroyed on the main thread.
+Connection* Connection::connection(UniqueID uniqueID) WTF_IGNORES_THREAD_SAFETY_ANALYSIS
{
ASSERT(RunLoop::isMain());
- return allConnections().get(uniqueID);
+ return connectionMap().get(uniqueID);
}
void Connection::setOnlySendMessagesAsDispatchWhenWaitingForSyncReplyWhenProcessingSuchAMessage(bool flag)
Modified: trunk/Source/WebKit/Platform/IPC/Connection.h (278394 => 278395)
--- trunk/Source/WebKit/Platform/IPC/Connection.h 2021-06-03 04:59:05 UTC (rev 278394)
+++ trunk/Source/WebKit/Platform/IPC/Connection.h 2021-06-03 05:09:02 UTC (rev 278395)
@@ -243,6 +243,8 @@
void postConnectionDidCloseOnConnectionWorkQueue();
template<typename T, typename C> uint64_t sendWithAsyncReply(T&& message, C&& completionHandler, uint64_t destinationID = 0, OptionSet<SendOption> = { }); // Thread-safe.
template<typename T> bool send(T&& message, uint64_t destinationID, OptionSet<SendOption> sendOptions = { }); // Thread-safe.
+ template<typename T> static bool send(UniqueID, T&& message, uint64_t destinationID, OptionSet<SendOption> sendOptions = { }); // Thread-safe.
+
// Sync senders should check the SendSyncResult for true/false in case they need to know if the result was really received.
// Sync senders should hold on to the SendSyncResult in case they reference the contents of the reply via DataRefererence / ArrayReference.
using SendSyncResult = std::unique_ptr<Decoder>;
@@ -325,6 +327,8 @@
void platformInvalidate();
bool isIncomingMessagesThrottlingEnabled() const { return !!m_incomingMessagesThrottler; }
+
+ static HashMap<IPC::Connection::UniqueID, Connection*>& connectionMap() WTF_REQUIRES_LOCK(s_connectionMapLock);
std::unique_ptr<Decoder> waitForMessage(MessageName, uint64_t destinationID, Timeout, OptionSet<WaitForOption>);
@@ -382,6 +386,7 @@
unsigned m_throttlingLevel { 0 };
};
+ static Lock s_connectionMapLock;
Client& m_client;
UniqueID m_uniqueID;
bool m_isServer;
@@ -515,6 +520,16 @@
return sendMessage(WTFMove(encoder), sendOptions);
}
+template<typename T>
+bool Connection::send(UniqueID connectionID, T&& message, uint64_t destinationID, OptionSet<SendOption> sendOptions)
+{
+ Locker locker { s_connectionMapLock };
+ auto* connection = connectionMap().get(connectionID);
+ if (!connection)
+ return false;
+ return connection->send(WTFMove(message), destinationID, sendOptions);
+}
+
uint64_t nextAsyncReplyHandlerID();
void addAsyncReplyHandler(Connection&, uint64_t, CompletionHandler<void(Decoder*)>&&);
CompletionHandler<void(Decoder*)> takeAsyncReplyHandler(Connection&, uint64_t);
Modified: trunk/Source/WebKit/UIProcess/mac/DisplayLink.cpp (278394 => 278395)
--- trunk/Source/WebKit/UIProcess/mac/DisplayLink.cpp 2021-06-03 04:59:05 UTC (rev 278394)
+++ trunk/Source/WebKit/UIProcess/mac/DisplayLink.cpp 2021-06-03 05:09:02 UTC (rev 278395)
@@ -90,7 +90,7 @@
{
Locker locker { m_observersLock };
- m_observers.ensure(&connection, [] {
+ m_observers.ensure(connection.uniqueID(), [] {
return ConnectionClientInfo { };
}).iterator->value.observers.append({ observerID, preferredFramesPerSecond });
}
@@ -111,7 +111,7 @@
Locker locker { m_observersLock };
- auto it = m_observers.find(&connection);
+ auto it = m_observers.find(connection.uniqueID());
if (it == m_observers.end())
return;
@@ -136,7 +136,7 @@
ASSERT(RunLoop::isMain());
Locker locker { m_observersLock };
- m_observers.remove(&connection);
+ m_observers.remove(connection.uniqueID());
// We do not stop the display link right away when |m_observers| becomes empty. Instead, we
// let the display link fire up to |maxFireCountWithoutObservers| times without observers to avoid
@@ -145,7 +145,7 @@
void DisplayLink::removeInfoForConnectionIfPossible(IPC::Connection& connection)
{
- auto it = m_observers.find(&connection);
+ auto it = m_observers.find(connection.uniqueID());
if (it == m_observers.end())
return;
@@ -158,7 +158,7 @@
{
Locker locker { m_observersLock };
- auto& connectionInfo = m_observers.ensure(&connection, [] {
+ auto& connectionInfo = m_observers.ensure(connection.uniqueID(), [] {
return ConnectionClientInfo { };
}).iterator->value;
@@ -169,7 +169,7 @@
{
Locker locker { m_observersLock };
- auto it = m_observers.find(&connection);
+ auto it = m_observers.find(connection.uniqueID());
if (it == m_observers.end())
return;
@@ -185,7 +185,7 @@
Locker locker { m_observersLock };
- auto it = m_observers.find(&connection);
+ auto it = m_observers.find(connection.uniqueID());
if (it == m_observers.end())
return;
@@ -218,7 +218,7 @@
};
bool anyConnectionHadObservers = false;
- for (auto& [connection, connectionInfo] : m_observers) {
+ for (auto& [connectionID, connectionInfo] : m_observers) {
if (connectionInfo.observers.isEmpty())
continue;
@@ -231,9 +231,9 @@
<< " observers, on background queue " << shouldSendIPCOnBackgroundQueue << " maxFramesPerSecond " << observersMaxFramesPerSecond << " full speed clients " << connectionInfo.fullSpeedUpdatesClientCount << " relevant " << mainThreadWantsUpdate);
if (connectionInfo.fullSpeedUpdatesClientCount) {
- connection->send(Messages::EventDispatcher::DisplayWasRefreshed(m_displayID, m_currentUpdate, mainThreadWantsUpdate), 0);
+ IPC::Connection::send(connectionID, Messages::EventDispatcher::DisplayWasRefreshed(m_displayID, m_currentUpdate, mainThreadWantsUpdate), 0);
} else if (mainThreadWantsUpdate)
- connection->send(Messages::WebProcess::DisplayWasRefreshed(m_displayID, m_currentUpdate), 0);
+ IPC::Connection::send(connectionID, Messages::WebProcess::DisplayWasRefreshed(m_displayID, m_currentUpdate), 0);
}
m_currentUpdate = m_currentUpdate.nextUpdate();
Modified: trunk/Source/WebKit/UIProcess/mac/DisplayLink.h (278394 => 278395)
--- trunk/Source/WebKit/UIProcess/mac/DisplayLink.h 2021-06-03 04:59:05 UTC (rev 278394)
+++ trunk/Source/WebKit/UIProcess/mac/DisplayLink.h 2021-06-03 05:09:02 UTC (rev 278395)
@@ -27,6 +27,7 @@
#if HAVE(CVDISPLAYLINK)
+#include "Connection.h"
#include "DisplayLinkObserverID.h"
#include <CoreVideo/CVDisplayLink.h>
#include <WebCore/AnimationFrameRate.h>
@@ -84,7 +85,7 @@
CVDisplayLinkRef m_displayLink { nullptr };
Lock m_observersLock;
- HashMap<RefPtr<IPC::Connection>, ConnectionClientInfo> m_observers WTF_GUARDED_BY_LOCK(m_observersLock);
+ HashMap<IPC::Connection::UniqueID, ConnectionClientInfo> m_observers WTF_GUARDED_BY_LOCK(m_observersLock);
WebCore::PlatformDisplayID m_displayID;
WebCore::FramesPerSecond m_displayNominalFramesPerSecond { 0 };
WebCore::DisplayUpdate m_currentUpdate;
