Diff
Modified: trunk/Source/WebKit/ChangeLog (284694 => 284695)
--- trunk/Source/WebKit/ChangeLog 2021-10-22 18:03:34 UTC (rev 284694)
+++ trunk/Source/WebKit/ChangeLog 2021-10-22 18:09:11 UTC (rev 284695)
@@ -1,3 +1,61 @@
+2021-10-22 Wenson Hsieh <[email protected]>
+
+ RemoteRenderingBackend::CreateImageBuffer should be an async IPC stream message
+ https://bugs.webkit.org/show_bug.cgi?id=231970
+
+ Reviewed by Kimmo Kinnunen.
+
+ This patch reverts the changes in r284476, which worked around a race when adding receive queues for newly
+ created IPC stream destinations and simultaneously dispatching IPC messages to those destinations. Rather than
+ making the IPC message that creates and adds the new image buffer's RemoteDisplayListRecorder synchronous, we
+ instead keep that message async and make adjustments to ensure that incoming out-of-stream IPC messages for
+ RemoteDisplayListRecorder can always be mapped to an appropriate receive queue. See below for more details.
+
+ * GPUProcess/graphics/RemoteDisplayListRecorder.cpp:
+ (WebKit::RemoteDisplayListRecorder::startListeningForIPC):
+
+ Move the main runloop bounce down to `StreamServerConnectionBase::startReceivingMessagesImpl()` instead (to deal
+ with the fact that `addMessageReceiveQueue` currently needs to be invoked on the main runloop). This allows us
+ to call `StreamServerConnection::startReceivingMessages()` from the processing queue while creating a remote
+ image buffer, which (in turn) ensures that incoming out-of-stream messages from the IPC thread will be sent to
+ the correct RemoteDisplayListRecorder destination by the time they're dispatched on the work queue thread.
+
+ * GPUProcess/graphics/RemoteRenderingBackend.cpp:
+ (WebKit::RemoteRenderingBackend::startListeningForIPC):
+ (WebKit::RemoteRenderingBackend::stopListeningForIPC):
+
+ Additionally register a "0-destination" receiver to ensure that all RemoteDisplayListRecorder messages (even
+ without pre-existing destinations) will be enqueued on the same IPC stream connection as this remote rendering
+ backend.
+
+ (WebKit::RemoteRenderingBackend::createImageBuffer):
+ * GPUProcess/graphics/RemoteRenderingBackend.h:
+ * GPUProcess/graphics/RemoteRenderingBackend.messages.in:
+
+ Make `CreateImageBuffer` an async stream message once again.
+
+ * Platform/IPC/StreamConnectionWorkQueue.cpp:
+ (IPC::StreamConnectionWorkQueue::processStreams):
+ * Platform/IPC/StreamConnectionWorkQueue.h:
+
+ Change `m_connections` into a HashCountedSet (from a HashSet), to ensure that the same server connection object
+ can be added to and removed from the work queue multiple times, without removing the connection from the map
+ early.
+
+ * Platform/IPC/StreamServerConnection.cpp:
+ (IPC::StreamServerConnectionBase::startReceivingMessagesImpl):
+ (IPC::StreamServerConnectionBase::stopReceivingMessagesImpl):
+ * Platform/IPC/StreamServerConnection.h:
+ (IPC::StreamServerConnection::startReceivingMessages):
+ (IPC::StreamServerConnection::stopReceivingMessages):
+
+ Add new methods to start and stop receiving all messages for a given ReceiverName, regardless of incoming
+ destination ID. RemoteRenderingBackend now uses this to register a "catch-all" listener for all
+ RemoteDisplayListRecorder messages.
+
+ * WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:
+ (WebKit::RemoteRenderingBackendProxy::createRemoteImageBuffer):
+
2021-10-22 Sihui Liu <[email protected]>
Followup to r284652: ensure file handle is closed in web process
Modified: trunk/Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.cpp (284694 => 284695)
--- trunk/Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.cpp 2021-10-22 18:03:34 UTC (rev 284694)
+++ trunk/Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.cpp 2021-10-22 18:09:11 UTC (rev 284695)
@@ -55,10 +55,7 @@
{
ASSERT(!m_isListeningForIPC);
m_isListeningForIPC = true;
- // FIXME: Can we avoid synchronous dispatch here by adjusting the assertion in `Connection::enqueueMatchingMessagesToMessageReceiveQueue`?
- callOnMainRunLoopAndWait([&] {
- m_renderingBackend->streamConnection().startReceivingMessages(*this, Messages::RemoteDisplayListRecorder::messageReceiverName(), m_imageBufferIdentifier.object().toUInt64());
- });
+ m_renderingBackend->streamConnection().startReceivingMessages(*this, Messages::RemoteDisplayListRecorder::messageReceiverName(), m_imageBufferIdentifier.object().toUInt64());
}
void RemoteDisplayListRecorder::stopListeningForIPC()
Modified: trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp (284694 => 284695)
--- trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp 2021-10-22 18:03:34 UTC (rev 284694)
+++ trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp 2021-10-22 18:09:11 UTC (rev 284695)
@@ -92,6 +92,10 @@
void RemoteRenderingBackend::startListeningForIPC()
{
m_streamConnection->startReceivingMessages(*this, Messages::RemoteRenderingBackend::messageReceiverName(), m_renderingBackendIdentifier.toUInt64());
+ // RemoteDisplayListRecorder messages depend on RemoteRenderingBackend, because RemoteRenderingBackend creates RemoteDisplayListRecorder and
+ // makes a receive queue for it. In order to guarantee correct ordering, ensure that all RemoteDisplayListRecorder messages are processed in
+ // the same sequence as RemoteRenderingBackend messages.
+ m_streamConnection->startReceivingMessages(Messages::RemoteDisplayListRecorder::messageReceiverName());
}
RemoteRenderingBackend::~RemoteRenderingBackend()
@@ -106,6 +110,7 @@
{
ASSERT(RunLoop::isMain());
m_streamConnection->stopReceivingMessages(Messages::RemoteRenderingBackend::messageReceiverName(), m_renderingBackendIdentifier.toUInt64());
+ m_streamConnection->stopReceivingMessages(Messages::RemoteDisplayListRecorder::messageReceiverName());
for (auto& remoteContext : std::exchange(m_remoteDisplayLists, { }))
remoteContext.stopListeningForIPC();
}
@@ -138,12 +143,11 @@
send(Messages::RemoteRenderingBackendProxy::DidFlush(flushIdentifier, renderingResourceIdentifier.object()), m_renderingBackendIdentifier);
}
-void RemoteRenderingBackend::createImageBuffer(const FloatSize& logicalSize, RenderingMode renderingMode, float resolutionScale, const DestinationColorSpace& colorSpace, PixelFormat pixelFormat, RenderingResourceIdentifier imageBufferResourceIdentifier, CompletionHandler<void()>&& completionHandler)
+void RemoteRenderingBackend::createImageBuffer(const FloatSize& logicalSize, RenderingMode renderingMode, float resolutionScale, const DestinationColorSpace& colorSpace, PixelFormat pixelFormat, RenderingResourceIdentifier imageBufferResourceIdentifier)
{
// Immediately turn the RenderingResourceIdentifier (which is error-prone) to a QualifiedRenderingResourceIdentifier,
// and use a helper function to make sure that don't accidentally use the RenderingResourceIdentifier (because the helper function can't see it).
createImageBufferWithQualifiedIdentifier(logicalSize, renderingMode, resolutionScale, colorSpace, pixelFormat, { imageBufferResourceIdentifier, m_gpuConnectionToWebProcess->webProcessIdentifier() });
- completionHandler();
}
void RemoteRenderingBackend::createImageBufferWithQualifiedIdentifier(const FloatSize& logicalSize, RenderingMode renderingMode, float resolutionScale, const DestinationColorSpace& colorSpace, PixelFormat pixelFormat, QualifiedRenderingResourceIdentifier imageBufferResourceIdentifier)
Modified: trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h (284694 => 284695)
--- trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h 2021-10-22 18:03:34 UTC (rev 284694)
+++ trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h 2021-10-22 18:09:11 UTC (rev 284695)
@@ -101,7 +101,7 @@
uint64_t messageSenderDestinationID() const override;
// Messages to be received.
- void createImageBuffer(const WebCore::FloatSize& logicalSize, WebCore::RenderingMode, float resolutionScale, const WebCore::DestinationColorSpace&, WebCore::PixelFormat, WebCore::RenderingResourceIdentifier, CompletionHandler<void()>&&);
+ void createImageBuffer(const WebCore::FloatSize& logicalSize, WebCore::RenderingMode, float resolutionScale, const WebCore::DestinationColorSpace&, WebCore::PixelFormat, WebCore::RenderingResourceIdentifier);
void updateSharedMemoryForGetPixelBuffer(uint32_t byteCount, CompletionHandler<void(const SharedMemory::IPCHandle&)>&&);
void semaphoreForGetPixelBuffer(CompletionHandler<void(const IPC::Semaphore&)>&&);
void updateSharedMemoryAndSemaphoreForGetPixelBuffer(uint32_t byteCount, CompletionHandler<void(const SharedMemory::IPCHandle&, const IPC::Semaphore&)>&&);
Modified: trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.messages.in (284694 => 284695)
--- trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.messages.in 2021-10-22 18:03:34 UTC (rev 284694)
+++ trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.messages.in 2021-10-22 18:09:11 UTC (rev 284695)
@@ -23,7 +23,7 @@
#if ENABLE(GPU_PROCESS)
messages -> RemoteRenderingBackend NotRefCounted Stream {
- CreateImageBuffer(WebCore::FloatSize logicalSize, WebCore::RenderingMode renderingMode, float resolutionScale, WebCore::DestinationColorSpace colorSpace, enum:uint8_t WebCore::PixelFormat pixelFormat, WebCore::RenderingResourceIdentifier renderingResourceIdentifier) -> () Synchronous
+ CreateImageBuffer(WebCore::FloatSize logicalSize, WebCore::RenderingMode renderingMode, float resolutionScale, WebCore::DestinationColorSpace colorSpace, enum:uint8_t WebCore::PixelFormat pixelFormat, WebCore::RenderingResourceIdentifier renderingResourceIdentifier)
UpdateSharedMemoryForGetPixelBuffer(uint32_t byteCount) -> (WebKit::SharedMemory::IPCHandle handle) Synchronous NotStreamEncodableReply
SemaphoreForGetPixelBuffer() -> (IPC::Semaphore semaphore) Synchronous NotStreamEncodableReply
UpdateSharedMemoryAndSemaphoreForGetPixelBuffer(uint32_t byteCount) -> (WebKit::SharedMemory::IPCHandle handle, IPC::Semaphore semaphore) Synchronous NotStreamEncodableReply
Modified: trunk/Source/WebKit/Platform/IPC/StreamConnectionWorkQueue.cpp (284694 => 284695)
--- trunk/Source/WebKit/Platform/IPC/StreamConnectionWorkQueue.cpp 2021-10-22 18:03:34 UTC (rev 284694)
+++ trunk/Source/WebKit/Platform/IPC/StreamConnectionWorkQueue.cpp 2021-10-22 18:09:11 UTC (rev 284695)
@@ -121,11 +121,11 @@
bool hasMoreToProcess = false;
do {
Deque<WTF::Function<void()>> functions;
- HashSet<Ref<StreamServerConnectionBase>> connections;
+ Vector<Ref<StreamServerConnectionBase>> connections;
{
Locker locker { m_lock };
functions.swap(m_functions);
- connections = m_connections;
+ connections = copyToVector(m_connections.values());
}
for (auto& function : functions)
WTFMove(function)();
Modified: trunk/Source/WebKit/Platform/IPC/StreamConnectionWorkQueue.h (284694 => 284695)
--- trunk/Source/WebKit/Platform/IPC/StreamConnectionWorkQueue.h 2021-10-22 18:03:34 UTC (rev 284694)
+++ trunk/Source/WebKit/Platform/IPC/StreamConnectionWorkQueue.h 2021-10-22 18:09:11 UTC (rev 284695)
@@ -30,7 +30,7 @@
#include <atomic>
#include <wtf/Deque.h>
#include <wtf/FunctionDispatcher.h>
-#include <wtf/HashSet.h>
+#include <wtf/HashCountedSet.h>
#include <wtf/Lock.h>
#include <wtf/Threading.h>
@@ -66,7 +66,7 @@
Lock m_lock;
RefPtr<Thread> m_processingThread WTF_GUARDED_BY_LOCK(m_lock);
Deque<Function<void()>> m_functions WTF_GUARDED_BY_LOCK(m_lock);
- HashSet<Ref<StreamServerConnectionBase>> m_connections WTF_GUARDED_BY_LOCK(m_lock);
+ HashCountedSet<Ref<StreamServerConnectionBase>> m_connections WTF_GUARDED_BY_LOCK(m_lock);
};
}
Modified: trunk/Source/WebKit/Platform/IPC/StreamServerConnection.cpp (284694 => 284695)
--- trunk/Source/WebKit/Platform/IPC/StreamServerConnection.cpp 2021-10-22 18:03:34 UTC (rev 284694)
+++ trunk/Source/WebKit/Platform/IPC/StreamServerConnection.cpp 2021-10-22 18:09:11 UTC (rev 284695)
@@ -40,10 +40,27 @@
void StreamServerConnectionBase::startReceivingMessagesImpl(ReceiverName receiverName, uint64_t destinationID)
{
- m_connection->addMessageReceiveQueue(*this, receiverName, destinationID);
+ // FIXME: Can we avoid synchronous dispatch here by adjusting the assertion in `Connection::enqueueMatchingMessagesToMessageReceiveQueue`?
+ callOnMainRunLoopAndWait([&] {
+ m_connection->addMessageReceiveQueue(*this, receiverName, destinationID);
+ });
m_workQueue.addStreamConnection(*this);
}
+void StreamServerConnectionBase::startReceivingMessagesImpl(ReceiverName receiverName)
+{
+ callOnMainRunLoopAndWait([&] {
+ m_connection->addMessageReceiveQueue(*this, receiverName);
+ });
+ m_workQueue.addStreamConnection(*this);
+}
+
+void StreamServerConnectionBase::stopReceivingMessagesImpl(ReceiverName receiverName)
+{
+ m_connection->removeMessageReceiveQueue(receiverName);
+ m_workQueue.removeStreamConnection(*this);
+}
+
void StreamServerConnectionBase::stopReceivingMessagesImpl(ReceiverName receiverName, uint64_t destinationID)
{
m_connection->removeMessageReceiveQueue(receiverName, destinationID);
Modified: trunk/Source/WebKit/Platform/IPC/StreamServerConnection.h (284694 => 284695)
--- trunk/Source/WebKit/Platform/IPC/StreamServerConnection.h 2021-10-22 18:03:34 UTC (rev 284694)
+++ trunk/Source/WebKit/Platform/IPC/StreamServerConnection.h 2021-10-22 18:09:11 UTC (rev 284695)
@@ -62,6 +62,9 @@
void startReceivingMessagesImpl(ReceiverName, uint64_t destinationID);
void stopReceivingMessagesImpl(ReceiverName, uint64_t destinationID);
+ void startReceivingMessagesImpl(ReceiverName);
+ void stopReceivingMessagesImpl(ReceiverName);
+
// MessageReceiveQueue
void enqueueMessage(Connection&, std::unique_ptr<Decoder>&&) final;
@@ -144,6 +147,9 @@
// Stops the message receipt. Note: already received messages might still be delivered.
void stopReceivingMessages(ReceiverName, uint64_t destinationID);
+ inline void startReceivingMessages(ReceiverName);
+ inline void stopReceivingMessages(ReceiverName);
+
// StreamServerConnectionBase overrides.
DispatchResult dispatchStreamMessages(size_t messageLimit) final;
@@ -161,4 +167,14 @@
uint64_t m_currentDestinationID { 0 };
};
+void StreamServerConnection::startReceivingMessages(ReceiverName receiverName)
+{
+ StreamServerConnectionBase::startReceivingMessagesImpl(receiverName);
}
+
+void StreamServerConnection::stopReceivingMessages(ReceiverName receiverName)
+{
+ StreamServerConnectionBase::stopReceivingMessagesImpl(receiverName);
+}
+
+}
Modified: trunk/Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp (284694 => 284695)
--- trunk/Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp 2021-10-22 18:03:34 UTC (rev 284694)
+++ trunk/Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp 2021-10-22 18:09:11 UTC (rev 284695)
@@ -127,9 +127,7 @@
void RemoteRenderingBackendProxy::createRemoteImageBuffer(ImageBuffer& imageBuffer)
{
- // FIXME: This should be an normal (async) stream message. However, doing so may cause subsequent out-of-stream messages sent to the newly created image
- // buffer to be dropped by the IPC connection. For the time being, work around this by using sync IPC (see <https://webkit.org/b/231681> for more details).
- sendSyncToStream(Messages::RemoteRenderingBackend::CreateImageBuffer(imageBuffer.logicalSize(), imageBuffer.renderingMode(), imageBuffer.resolutionScale(), imageBuffer.colorSpace(), imageBuffer.pixelFormat(), imageBuffer.renderingResourceIdentifier()), Messages::RemoteRenderingBackend::CreateImageBuffer::Reply(), 3_s);
+ sendToStream(Messages::RemoteRenderingBackend::CreateImageBuffer(imageBuffer.logicalSize(), imageBuffer.renderingMode(), imageBuffer.resolutionScale(), imageBuffer.colorSpace(), imageBuffer.pixelFormat(), imageBuffer.renderingResourceIdentifier()));
}
RefPtr<ImageBuffer> RemoteRenderingBackendProxy::createImageBuffer(const FloatSize& size, RenderingMode renderingMode, float resolutionScale, const DestinationColorSpace& colorSpace, PixelFormat pixelFormat)
