Changeset 284681 in webkit

Timestamp:

Oct 22, 2021, 7:38:42 AM (5 years ago)

Author:

commit-queue@webkit.org

Message:

StreamConnectionWorkQueue::processStreams() has a incorrect protection ref
​https://bugs.webkit.org/show_bug.cgi?id=232070

Patch by Kimmo Kinnunen <​kkinnunen@apple.com> on 2021-10-22
Reviewed by Wenson Hsieh.

• GPUProcess/graphics/RemoteRenderingBackend.cpp:

(WebKit::RemoteRenderingBackend::~RemoteRenderingBackend):
Additionally remove redundant protection ref from RemoteRenderingBackend
cleanup task. Since m_workQueue->stop() is run after dispatching the
task, and since stop() waits until queue has executed the
all the tasks, the m_workQueue ref outlives the protection ref.

• Platform/IPC/StreamConnectionWorkQueue.cpp:

(IPC::StreamConnectionWorkQueue::processStreams):
Remove the redundant protection ref so it does not cause confusion.
The protection ref cannot hold the last ref, as that would mean
that the StreamConnectionWorkQueue thread would run the code to
destroy the work queue itself. There has to be a external ref for
queue->stop() that outlives the protection ref, as the stop() will
wait until the queue thread stops.

(IPC::StreamConnectionWorkQueue::stopAndWaitForCompletion):
(IPC::StreamConnectionWorkQueue::stop): Deleted.

• Platform/IPC/StreamConnectionWorkQueue.h:

Rename stop() to stopAndWaitForCompletion() to signify what
the function does.

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• GPUProcess/graphics/RemoteRenderingBackend.cpp (modified) (1 diff)
• Platform/IPC/StreamConnectionWorkQueue.cpp (modified) (3 diffs)
• Platform/IPC/StreamConnectionWorkQueue.h (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2021-10-22  Kimmo Kinnunen  <kkinnunen@apple.com>
+
+        StreamConnectionWorkQueue::processStreams() has a incorrect protection ref
+        https://bugs.webkit.org/show_bug.cgi?id=232070
+
+        Reviewed by Wenson Hsieh.
+
+        * GPUProcess/graphics/RemoteRenderingBackend.cpp:
+        (WebKit::RemoteRenderingBackend::~RemoteRenderingBackend):
+        Additionally remove redundant protection ref from `RemoteRenderingBackend`
+        cleanup task. Since `m_workQueue->stop()` is run after dispatching the
+        task, and since `stop()` waits until queue has executed the
+        all the tasks, the `m_workQueue` ref outlives the protection ref.
+
+        * Platform/IPC/StreamConnectionWorkQueue.cpp:
+        (IPC::StreamConnectionWorkQueue::processStreams):
+        Remove the redundant protection ref so it does not cause confusion.
+        The protection ref cannot hold the last ref, as that would mean
+        that the `StreamConnectionWorkQueue` thread would run the code to
+        destroy the work queue itself. There has to be a external ref for
+        `queue->stop()` that outlives the protection ref, as the `stop()` will
+        wait until the queue thread stops.
+
+        (IPC::StreamConnectionWorkQueue::stopAndWaitForCompletion):
+        (IPC::StreamConnectionWorkQueue::stop): Deleted.
+        * Platform/IPC/StreamConnectionWorkQueue.h:
+        Rename `stop()` to `stopAndWaitForCompletion()` to signify what
+        the function does.
+
 2021-10-22  Alberto Garcia  <berto@igalia.com>
 

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

@@ -99 +99 @@
     // 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([workQueue = m_workQueue.copyRef(), renderingResourcesRequest = WTFMove(m_renderingResourcesRequest), remoteResourceCache = WTFMove(m_remoteResourceCache)] { });
-    m_workQueue->stop();
+    m_workQueue->dispatch([renderingResourcesRequest = WTFMove(m_renderingResourcesRequest), remoteResourceCache = WTFMove(m_remoteResourceCache)] { });
+    m_workQueue->stopAndWaitForCompletion();
 }
 

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

@@ -36 +36 @@
 StreamConnectionWorkQueue::~StreamConnectionWorkQueue()
 {
-    // `StreamConnectionWorkQueue::stop()` should be called if anything has been dispatched or listened to.
+    // `StreamConnectionWorkQueue::stopAndWaitForCompletion()` should be called if anything has been dispatched or listened to.
     ASSERT(!m_processingThread);
 }
@@ -76 +76 @@
 }
 
-void StreamConnectionWorkQueue::stop()
+void StreamConnectionWorkQueue::stopAndWaitForCompletion()
 {
     m_shouldQuit = true;
@@ -118 +118 @@
 void StreamConnectionWorkQueue::processStreams()
 {
-    Ref protectedThis = *this;
     constexpr size_t defaultMessageLimit = 1000;
     bool hasMoreToProcess = false;

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

@@ -50 +50 @@
 
     void dispatch(WTF::Function<void()>&&) final;
-    void stop();
+    void stopAndWaitForCompletion();
 
     void wakeUp();