Changeset 284768 in webkit

Timestamp:

Oct 24, 2021, 1:57:44 PM (5 years ago)

Author:

Wenson Hsieh

Message:

RemoteRenderingBackend should not send IPC in the middle of destruction
​https://bugs.webkit.org/show_bug.cgi?id=232179

Reviewed by Darin Adler.

Make a couple of minor adjustments to RemoteRenderingBackend (see below for more details). This is necessary in
order to avoid flaky crashes after fixing bug #232113, after which the RemoteRenderingBackend will no longer be
leaked in the GPU process.

• GPUProcess/graphics/RemoteRenderingBackend.cpp:

(WebKit::RemoteRenderingBackend::startListeningForIPC):
(WebKit::RemoteRenderingBackend::stopListeningForIPC):
(WebKit::RemoteRenderingBackend::didCreateImageBufferBackend):
(WebKit::RemoteRenderingBackend::releaseRemoteResourceWithQualifiedIdentifier):
(WebKit::RemoteRenderingBackend::~RemoteRenderingBackend): Deleted.

Move logic to flush remaining incoming IPC messages in the GPU process out of the destructor, and into
stopListeningForIPC() instead. This is because the act of processing certain stream IPC messages (such as
CreateImageBuffer or FlushContext) may cause RemoteRenderingBackend to try and send IPC back to the web process.
However, if RemoteRenderingBackend is in the middle of destruction, it will crash when attempting to do so (when
attempting to call into IPC::MessageSender).

To avoid this, we need to do this work earlier, after we've already stopped listening for further IPC messages.

• GPUProcess/graphics/RemoteRenderingBackend.h:

Turn m_remoteDisplayLists into a regular hash map containing RemoteDisplayListRecorders by their process-
qualified rendering resource identifiers. Since this map may be modified from different threads, we (1) don't
want to be using weak pointers here, and (2) need to ensure that access to this table is guarded behind a lock.
To avoid reference cycles, entries in this table are cleared out when the remote image buffer corresponding to
each RemoteDisplayListRecorder is released in the GPU process.

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• GPUProcess/graphics/RemoteRenderingBackend.cpp (modified) (5 diffs)
• GPUProcess/graphics/RemoteRenderingBackend.h (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2021-10-24  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        RemoteRenderingBackend should not send IPC in the middle of destruction
+        https://bugs.webkit.org/show_bug.cgi?id=232179
+
+        Reviewed by Darin Adler.
+
+        Make a couple of minor adjustments to RemoteRenderingBackend (see below for more details). This is necessary in
+        order to avoid flaky crashes after fixing bug #232113, after which the RemoteRenderingBackend will no longer be
+        leaked in the GPU process.
+
+        * GPUProcess/graphics/RemoteRenderingBackend.cpp:
+        (WebKit::RemoteRenderingBackend::startListeningForIPC):
+        (WebKit::RemoteRenderingBackend::stopListeningForIPC):
+        (WebKit::RemoteRenderingBackend::didCreateImageBufferBackend):
+        (WebKit::RemoteRenderingBackend::releaseRemoteResourceWithQualifiedIdentifier):
+        (WebKit::RemoteRenderingBackend::~RemoteRenderingBackend): Deleted.
+
+        Move logic to flush remaining incoming IPC messages in the GPU process out of the destructor, and into
+        `stopListeningForIPC()` instead. This is because the act of processing certain stream IPC messages (such as
+        CreateImageBuffer or FlushContext) may cause RemoteRenderingBackend to try and send IPC back to the web process.
+        However, if RemoteRenderingBackend is in the middle of destruction, it will crash when attempting to do so (when
+        attempting to call into IPC::MessageSender).
+
+        To avoid this, we need to do this work earlier, after we've already stopped listening for further IPC messages.
+
+        * GPUProcess/graphics/RemoteRenderingBackend.h:
+
+        Turn `m_remoteDisplayLists` into a regular hash map containing RemoteDisplayListRecorders by their process-
+        qualified rendering resource identifiers. Since this map may be modified from different threads, we (1) don't
+        want to be using weak pointers here, and (2) need to ensure that access to this table is guarded behind a lock.
+        To avoid reference cycles, entries in this table are cleared out when the remote image buffer corresponding to
+        each RemoteDisplayListRecorder is released in the GPU process.
+
 2021-10-24  Wenson Hsieh  <wenson_hsieh@apple.com>
 

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

@@ -90 +90 @@
 }
 
+RemoteRenderingBackend::~RemoteRenderingBackend() = default;
+
 void RemoteRenderingBackend::startListeningForIPC()
 {
+    {
+        Locker locker { m_remoteDisplayListsLock };
+        m_canRegisterRemoteDisplayLists = true;
+    }
+
     m_streamConnection->startReceivingMessages(*this, Messages::RemoteRenderingBackend::messageReceiverName(), m_renderingBackendIdentifier.toUInt64());
     // RemoteDisplayListRecorder messages depend on RemoteRenderingBackend, because RemoteRenderingBackend creates RemoteDisplayListRecorder and
@@ -99 +106 @@
 }
 
-RemoteRenderingBackend::~RemoteRenderingBackend()
-{
+void RemoteRenderingBackend::stopListeningForIPC()
+{
+    ASSERT(RunLoop::isMain());
+    m_streamConnection->stopReceivingMessages(Messages::RemoteRenderingBackend::messageReceiverName(), m_renderingBackendIdentifier.toUInt64());
+    m_streamConnection->stopReceivingMessages(Messages::RemoteDisplayListRecorder::messageReceiverName());
+
+    {
+        Locker locker { m_remoteDisplayListsLock };
+        m_canRegisterRemoteDisplayLists = false;
+        for (auto& remoteContext : std::exchange(m_remoteDisplayLists, { }))
+            remoteContext.value->stopListeningForIPC();
+    }
+
     // Make sure we destroy the ResourceCache on the WorkQueue since it gets populated on the WorkQueue.
     // Make sure rendering resource request is released after destroying the cache.
@@ -107 +125 @@
 }
 
-void RemoteRenderingBackend::stopListeningForIPC()
-{
-    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();
-}
-
 void RemoteRenderingBackend::dispatch(Function<void()>&& task)
 {
@@ -133 +142 @@
 void RemoteRenderingBackend::didCreateImageBufferBackend(ImageBufferBackendHandle handle, QualifiedRenderingResourceIdentifier renderingResourceIdentifier, RemoteDisplayListRecorder& remoteDisplayList)
 {
-    m_remoteDisplayLists.add(remoteDisplayList);
+    {
+        Locker locker { m_remoteDisplayListsLock };
+        if (m_canRegisterRemoteDisplayLists)
+            m_remoteDisplayLists.add(renderingResourceIdentifier, remoteDisplayList);
+    }
     MESSAGE_CHECK(renderingResourceIdentifier.processIdentifier() == m_gpuConnectionToWebProcess->webProcessIdentifier(), "Sending didCreateImageBufferBackend() message to the wrong web process.");
     send(Messages::RemoteRenderingBackendProxy::DidCreateImageBufferBackend(WTFMove(handle), renderingResourceIdentifier.object()), m_renderingBackendIdentifier);
@@ -364 +377 @@
     MESSAGE_CHECK(success, "Resource is being released before being cached.");
     updateRenderingResourceRequest();
+
+    Locker locker { m_remoteDisplayListsLock };
+    m_remoteDisplayLists.remove(renderingResourceIdentifier);
 }
 

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

@@ -134 +134 @@
     RefPtr<SharedMemory> m_getPixelBufferSharedMemory;
     ScopedRenderingResourcesRequest m_renderingResourcesRequest;
-    WeakHashSet<RemoteDisplayListRecorder> m_remoteDisplayLists;
+
+    Lock m_remoteDisplayListsLock;
+    bool m_canRegisterRemoteDisplayLists WTF_GUARDED_BY_LOCK(m_remoteDisplayListsLock) { false };
+    HashMap<QualifiedRenderingResourceIdentifier, Ref<RemoteDisplayListRecorder>> m_remoteDisplayLists WTF_GUARDED_BY_LOCK(m_remoteDisplayListsLock);
 };