Title: [284768] trunk/Source/WebKit
- Revision
- 284768
- Author
- [email protected]
- Date
- 2021-10-24 13:57:44 -0700 (Sun, 24 Oct 2021)
Log 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.
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (284767 => 284768)
--- trunk/Source/WebKit/ChangeLog 2021-10-24 20:43:41 UTC (rev 284767)
+++ trunk/Source/WebKit/ChangeLog 2021-10-24 20:57:44 UTC (rev 284768)
@@ -1,5 +1,39 @@
2021-10-24 Wenson Hsieh <[email protected]>
+ 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 <[email protected]>
+
REGRESSION (iOS 15): Safari shows zoom callout even if -webkit-user-select is none
https://bugs.webkit.org/show_bug.cgi?id=231161
rdar://83863266
Modified: trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp (284767 => 284768)
--- trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp 2021-10-24 20:43:41 UTC (rev 284767)
+++ trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp 2021-10-24 20:57:44 UTC (rev 284768)
@@ -89,8 +89,15 @@
send(Messages::RemoteRenderingBackendProxy::DidCreateWakeUpSemaphoreForDisplayListStream(m_workQueue->wakeUpSemaphore()), m_renderingBackendIdentifier);
}
+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
// makes a receive queue for it. In order to guarantee correct ordering, ensure that all RemoteDisplayListRecorder messages are processed in
@@ -98,8 +105,19 @@
m_streamConnection->startReceivingMessages(Messages::RemoteDisplayListRecorder::messageReceiverName());
}
-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.
m_workQueue->dispatch([renderingResourcesRequest = WTFMove(m_renderingResourcesRequest), remoteResourceCache = WTFMove(m_remoteResourceCache)] { });
@@ -106,15 +124,6 @@
m_workQueue->stopAndWaitForCompletion();
}
-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)
{
m_workQueue->dispatch(WTFMove(task));
@@ -132,7 +141,11 @@
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);
}
@@ -363,6 +376,9 @@
auto success = m_remoteResourceCache.releaseRemoteResource(renderingResourceIdentifier, useCount);
MESSAGE_CHECK(success, "Resource is being released before being cached.");
updateRenderingResourceRequest();
+
+ Locker locker { m_remoteDisplayListsLock };
+ m_remoteDisplayLists.remove(renderingResourceIdentifier);
}
void RemoteRenderingBackend::finalizeRenderingUpdate(RenderingUpdateID renderingUpdateID)
Modified: trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h (284767 => 284768)
--- trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h 2021-10-24 20:43:41 UTC (rev 284767)
+++ trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h 2021-10-24 20:57:44 UTC (rev 284768)
@@ -133,7 +133,10 @@
IPC::Semaphore m_getPixelBufferSemaphore;
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);
};
} // namespace WebKit
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
