- Revision
- 279115
- Author
- [email protected]
- Date
- 2021-06-22 06:25:03 -0700 (Tue, 22 Jun 2021)
Log Message
Functions dispatched to WorkQueue are sometimes destroyed in the calling thread due to block refcounting
https://bugs.webkit.org/show_bug.cgi?id=227160
Patch by Kimmo Kinnunen <[email protected]> on 2021-06-22
Reviewed by Antti Koivisto.
Source/WebKit:
As an example, remove one workaround mutability of dispatched function
and nullptr assignment that was needed for working around
WorkQueue::dispatch bug where the dispatched function was not always
destroyed in the queue.
* Shared/mac/MediaFormatReader/MediaTrackReader.cpp:
(WebKit::MediaTrackReader::finalize):
Source/WTF:
WorkQueue::dispatch Function bodies always execute in the thread of target
queue. However, sometimes the Function destruction would happen in the
dispatched-from thread. This is not what the WorkQueue::dispatch callers
expect.
Implement some of the WorkQueue::dispatch* in terms of dispatch_..._f()
variants which take a function and context pointer instead of a block.
Blocks are reference counted objects without the ability to pass the
ownership of the reference. For dispatch case, it means that caller will
need to hold the reference while dispatch_...() returns. In thread
contention cases the called block might complete and reference be
dropped in the thread of the queue before the dispatched-from thread
would drop its reference to the block. This would cause the dispatched
Function to be destroyed in the dispatched-from thread.
* wtf/cocoa/WorkQueueCocoa.cpp:
(WTF::dispatchWorkItem):
(WTF::WorkQueue::dispatch):
(WTF::WorkQueue::dispatchAfter):
(WTF::WorkQueue::dispatchSync):
Tools:
Test that the Function passed to WorkQueue::dispatch
is always destroyed in the WorkQueue. Test by using the
knowledge that WorkQueues use threads.
Start up many WorkQueues to create more thread contention
to ensure that the dispatched-from thread is sometimes not
run while the thread of the queue finishes processing
the dispatch.
* TestWebKitAPI/Tests/WTF/WorkQueue.cpp:
(TestWebKitAPI::TEST):
Modified Paths
Diff
Modified: trunk/Source/WTF/ChangeLog (279114 => 279115)
--- trunk/Source/WTF/ChangeLog 2021-06-22 13:01:51 UTC (rev 279114)
+++ trunk/Source/WTF/ChangeLog 2021-06-22 13:25:03 UTC (rev 279115)
@@ -1,3 +1,32 @@
+2021-06-22 Kimmo Kinnunen <[email protected]>
+
+ Functions dispatched to WorkQueue are sometimes destroyed in the calling thread due to block refcounting
+ https://bugs.webkit.org/show_bug.cgi?id=227160
+
+ Reviewed by Antti Koivisto.
+
+ WorkQueue::dispatch Function bodies always execute in the thread of target
+ queue. However, sometimes the Function destruction would happen in the
+ dispatched-from thread. This is not what the WorkQueue::dispatch callers
+ expect.
+
+ Implement some of the WorkQueue::dispatch* in terms of dispatch_..._f()
+ variants which take a function and context pointer instead of a block.
+
+ Blocks are reference counted objects without the ability to pass the
+ ownership of the reference. For dispatch case, it means that caller will
+ need to hold the reference while dispatch_...() returns. In thread
+ contention cases the called block might complete and reference be
+ dropped in the thread of the queue before the dispatched-from thread
+ would drop its reference to the block. This would cause the dispatched
+ Function to be destroyed in the dispatched-from thread.
+
+ * wtf/cocoa/WorkQueueCocoa.cpp:
+ (WTF::dispatchWorkItem):
+ (WTF::WorkQueue::dispatch):
+ (WTF::WorkQueue::dispatchAfter):
+ (WTF::WorkQueue::dispatchSync):
+
2021-06-21 Chris Dumez <[email protected]>
Adjust `fetch` port blocking for ports 990, 989
Modified: trunk/Source/WTF/wtf/cocoa/WorkQueueCocoa.cpp (279114 => 279115)
--- trunk/Source/WTF/wtf/cocoa/WorkQueueCocoa.cpp 2021-06-22 13:01:51 UTC (rev 279114)
+++ trunk/Source/WTF/wtf/cocoa/WorkQueueCocoa.cpp 2021-06-22 13:25:03 UTC (rev 279115)
@@ -31,23 +31,37 @@
namespace WTF {
+namespace {
+
+struct DispatchWorkItem {
+ WTF_MAKE_STRUCT_FAST_ALLOCATED;
+ Ref<WorkQueue> m_workQueue;
+ Function<void()> m_function;
+ void operator()() { m_function(); }
+};
+
+}
+
+template<typename T> static void dispatchWorkItem(void* dispatchContext)
+{
+ T* item = reinterpret_cast<T*>(dispatchContext);
+ (*item)();
+ delete item;
+}
+
void WorkQueue::dispatch(Function<void()>&& function)
{
- dispatch_async(m_dispatchQueue.get(), makeBlockPtr([protectedThis = makeRef(*this), function = WTFMove(function)] {
- function();
- }).get());
+ dispatch_async_f(m_dispatchQueue.get(), new DispatchWorkItem { makeRef(*this), WTFMove(function) }, dispatchWorkItem<DispatchWorkItem>);
}
void WorkQueue::dispatchAfter(Seconds duration, Function<void()>&& function)
{
- dispatch_after(dispatch_time(DISPATCH_TIME_NOW, duration.nanosecondsAs<int64_t>()), m_dispatchQueue.get(), makeBlockPtr([protectedThis = makeRef(*this), function = WTFMove(function)] {
- function();
- }).get());
+ dispatch_after_f(dispatch_time(DISPATCH_TIME_NOW, duration.nanosecondsAs<int64_t>()), m_dispatchQueue.get(), new DispatchWorkItem { makeRef(*this), WTFMove(function) }, dispatchWorkItem<DispatchWorkItem>);
}
void WorkQueue::dispatchSync(Function<void()>&& function)
{
- dispatch_sync(m_dispatchQueue.get(), makeBlockPtr(WTFMove(function)).get());
+ dispatch_sync_f(m_dispatchQueue.get(), new Function<void()> { WTFMove(function) }, dispatchWorkItem<Function<void()>>);
}
Ref<WorkQueue> WorkQueue::constructMainWorkQueue()
Modified: trunk/Source/WebKit/ChangeLog (279114 => 279115)
--- trunk/Source/WebKit/ChangeLog 2021-06-22 13:01:51 UTC (rev 279114)
+++ trunk/Source/WebKit/ChangeLog 2021-06-22 13:25:03 UTC (rev 279115)
@@ -1,3 +1,18 @@
+2021-06-22 Kimmo Kinnunen <[email protected]>
+
+ Functions dispatched to WorkQueue are sometimes destroyed in the calling thread due to block refcounting
+ https://bugs.webkit.org/show_bug.cgi?id=227160
+
+ Reviewed by Antti Koivisto.
+
+ As an example, remove one workaround mutability of dispatched function
+ and nullptr assignment that was needed for working around
+ WorkQueue::dispatch bug where the dispatched function was not always
+ destroyed in the queue.
+
+ * Shared/mac/MediaFormatReader/MediaTrackReader.cpp:
+ (WebKit::MediaTrackReader::finalize):
+
2021-06-22 Myles C. Maxfield <[email protected]>
[Cocoa] Force a copy of font data when receiving it from the untrusted web process
Modified: trunk/Source/WebKit/Shared/mac/MediaFormatReader/MediaTrackReader.cpp (279114 => 279115)
--- trunk/Source/WebKit/Shared/mac/MediaFormatReader/MediaTrackReader.cpp 2021-06-22 13:01:51 UTC (rev 279114)
+++ trunk/Source/WebKit/Shared/mac/MediaFormatReader/MediaTrackReader.cpp 2021-06-22 13:25:03 UTC (rev 279115)
@@ -203,9 +203,7 @@
void MediaTrackReader::finalize()
{
Locker locker { m_sampleStorageLock };
- storageQueue().dispatch([sampleStorage = std::exchange(m_sampleStorage, nullptr)]() mutable {
- sampleStorage = nullptr;
- });
+ storageQueue().dispatch([sampleStorage = std::exchange(m_sampleStorage, nullptr)] { });
CoreMediaWrapped<MediaTrackReader>::finalize();
}
Modified: trunk/Tools/ChangeLog (279114 => 279115)
--- trunk/Tools/ChangeLog 2021-06-22 13:01:51 UTC (rev 279114)
+++ trunk/Tools/ChangeLog 2021-06-22 13:25:03 UTC (rev 279115)
@@ -1,5 +1,23 @@
2021-06-22 Kimmo Kinnunen <[email protected]>
+ Functions dispatched to WorkQueue are sometimes destroyed in the calling thread due to block refcounting
+ https://bugs.webkit.org/show_bug.cgi?id=227160
+
+ Reviewed by Antti Koivisto.
+
+ Test that the Function passed to WorkQueue::dispatch
+ is always destroyed in the WorkQueue. Test by using the
+ knowledge that WorkQueues use threads.
+ Start up many WorkQueues to create more thread contention
+ to ensure that the dispatched-from thread is sometimes not
+ run while the thread of the queue finishes processing
+ the dispatch.
+
+ * TestWebKitAPI/Tests/WTF/WorkQueue.cpp:
+ (TestWebKitAPI::TEST):
+
+2021-06-22 Kimmo Kinnunen <[email protected]>
+
Test runner parses the names of value parametrised GTEST tests wrong
https://bugs.webkit.org/show_bug.cgi?id=227207
Modified: trunk/Tools/TestWebKitAPI/Tests/WTF/WorkQueue.cpp (279114 => 279115)
--- trunk/Tools/TestWebKitAPI/Tests/WTF/WorkQueue.cpp 2021-06-22 13:01:51 UTC (rev 279114)
+++ trunk/Tools/TestWebKitAPI/Tests/WTF/WorkQueue.cpp 2021-06-22 13:25:03 UTC (rev 279115)
@@ -30,6 +30,7 @@
#include <wtf/Lock.h>
#include <wtf/Vector.h>
#include <wtf/WorkQueue.h>
+#include <memory>
#include <string>
#include <thread>
@@ -41,7 +42,7 @@
static const char* longTestLabel = "longTest";
static const char* thirdTestLabel = "thirdTest";
static const char* dispatchAfterLabel = "dispatchAfter";
-
+
TEST(WTF_WorkQueue, Simple)
{
Lock m_lock;
@@ -80,7 +81,7 @@
EXPECT_TRUE(calledSimpleTest);
EXPECT_TRUE(calledLongTest);
EXPECT_TRUE(calledThirdTest);
-
+
m_testCompleted.notifyOne();
});
@@ -107,7 +108,7 @@
bool calledSimpleTest = false;
bool calledLongTest = false;
bool calledThirdTest = false;
-
+
auto queue1 = WorkQueue::create("com.apple.WebKit.Test.twoQueues1");
auto queue2 = WorkQueue::create("com.apple.WebKit.Test.twoQueues2");
@@ -115,7 +116,7 @@
EXPECT_EQ(1U, queue2->refCount());
Locker locker { m_lock };
-
+
queue1->dispatch([&](void) {
m_functionCallOrder.append(simpleTestLabel);
calledSimpleTest = true;
@@ -138,7 +139,7 @@
Locker locker { m_lock };
m_functionCallOrder.append(thirdTestLabel);
calledThirdTest = true;
-
+
m_testQueue1Completed.notifyOne();
});
@@ -191,7 +192,7 @@
EXPECT_TRUE(calledSimpleTest);
EXPECT_FALSE(calledDispatchAfterTest);
-
+
m_dispatchAfterTestCompleted.wait(m_lock);
EXPECT_TRUE(calledSimpleTest);
@@ -274,4 +275,40 @@
EXPECT_TRUE(secondSyncTaskTaskRan);
}
+// Tests that the Function passed to WorkQueue::dispatch is destructed on the thread that
+// runs the Function. It is a common pattern to capture a owning reference into a Function
+// and dispatch that to a queue to ensure ordering (or thread affinity) of the object destruction.
+TEST(WTF_WorkQueue, DestroyDispatchedOnDispatchQueue)
+{
+ std::atomic<size_t> counter = 0;
+ class DestructionWorkQueueTester {
+ public:
+ DestructionWorkQueueTester(std::atomic<size_t>& counter)
+ : m_counter(counter)
+ {
+ }
+ ~DestructionWorkQueueTester()
+ {
+ EXPECT_NE(m_createdInThread, Thread::current().uid());
+ m_counter++;
+ }
+ private:
+ uint32_t m_createdInThread = Thread::current().uid();
+ std::atomic<size_t>& m_counter;
+ };
+ constexpr size_t queueCount = 50;
+ constexpr size_t iterationCount = 10000;
+ RefPtr<WorkQueue> queue[queueCount];
+ for (size_t i = 0; i < queueCount; ++i)
+ queue[i] = WorkQueue::create("com.apple.WebKit.Test.destroyDispatchedOnDispatchQueue", WorkQueue::Type::Serial, WorkQueue::QOS::UserInteractive);
+
+ for (size_t i = 0; i < iterationCount; ++i) {
+ for (size_t j = 0; j < queueCount; ++j)
+ queue[j]->dispatch([instance = std::make_unique<DestructionWorkQueueTester>(counter)]() { }); // NOLINT
+ }
+ for (size_t j = 0; j < queueCount; ++j)
+ queue[j]->dispatchSync([] { });
+ EXPECT_EQ(queueCount * iterationCount, counter);
+
+}
} // namespace TestWebKitAPI
