Changeset 284695 in webkit

Timestamp:

Oct 22, 2021, 11:09:11 AM (5 years ago)

Author:

Wenson Hsieh

Message:

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):

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• GPUProcess/graphics/RemoteDisplayListRecorder.cpp (modified) (1 diff)
• GPUProcess/graphics/RemoteRenderingBackend.cpp (modified) (3 diffs)
• GPUProcess/graphics/RemoteRenderingBackend.h (modified) (1 diff)
• GPUProcess/graphics/RemoteRenderingBackend.messages.in (modified) (1 diff)
• Platform/IPC/StreamConnectionWorkQueue.cpp (modified) (1 diff)
• Platform/IPC/StreamConnectionWorkQueue.h (modified) (2 diffs)
• Platform/IPC/StreamServerConnection.cpp (modified) (1 diff)
• Platform/IPC/StreamServerConnection.h (modified) (3 diffs)
• WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2021-10-22  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        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  <sihui_liu@apple.com>
 

trunk/Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.cpp

@@ -56 +56 @@
     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());
 }
 

trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp

@@ -93 +93 @@
 {
     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());
 }
 
@@ -107 +111 @@
     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();
@@ -139 +144 @@
 }
 
-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();
 }
 

trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h

@@ -102 +102 @@
 
     // 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&)>&&);

trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.messages.in

@@ -24 +24 @@
 
 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

trunk/Source/WebKit/Platform/IPC/StreamConnectionWorkQueue.cpp

@@ -122 +122 @@
     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)

trunk/Source/WebKit/Platform/IPC/StreamConnectionWorkQueue.h

@@ -31 +31 @@
 #include <wtf/Deque.h>
 #include <wtf/FunctionDispatcher.h>
-#include <wtf/HashSet.h>
+#include <wtf/HashCountedSet.h>
 #include <wtf/Lock.h>
 #include <wtf/Threading.h>
@@ -67 +67 @@
     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);
 };
 

trunk/Source/WebKit/Platform/IPC/StreamServerConnection.cpp

@@ -41 +41 @@
 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);
 }
 

trunk/Source/WebKit/Platform/IPC/StreamServerConnection.h

@@ -62 +62 @@
     void startReceivingMessagesImpl(ReceiverName, uint64_t destinationID);
     void stopReceivingMessagesImpl(ReceiverName, uint64_t destinationID);
+
+    void startReceivingMessagesImpl(ReceiverName);
+    void stopReceivingMessagesImpl(ReceiverName);
 
     // MessageReceiveQueue
@@ -145 +148 @@
     void stopReceivingMessages(ReceiverName, uint64_t destinationID);
 
+    inline void startReceivingMessages(ReceiverName);
+    inline void stopReceivingMessages(ReceiverName);
+
     // StreamServerConnectionBase overrides.
     DispatchResult dispatchStreamMessages(size_t messageLimit) final;
@@ -162 +168 @@
 };
 
+void StreamServerConnection::startReceivingMessages(ReceiverName receiverName)
+{
+    StreamServerConnectionBase::startReceivingMessagesImpl(receiverName);
 }
+
+void StreamServerConnection::stopReceivingMessages(ReceiverName receiverName)
+{
+    StreamServerConnectionBase::stopReceivingMessagesImpl(receiverName);
+}
+
+}

trunk/Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp

@@ -128 +128 @@
 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()));
 }