Title: [260682] trunk/Source
- Revision
- 260682
- Author
- [email protected]
- Date
- 2020-04-24 16:56:38 -0700 (Fri, 24 Apr 2020)
Log Message
[WTF] allThreads registration is racy with allThreads unregistration
https://bugs.webkit.org/show_bug.cgi?id=210995
<rdar://problem/61609690>
Reviewed by Keith Miller.
Source/WebCore:
* page/cocoa/ResourceUsageThreadCocoa.mm:
(WebCore::ResourceUsageThread::platformCollectCPUData):
Source/WTF:
There is a race between registering a thread to allThreads and unregistering a thread from allThreads.
1. Caller: A new thread is created, but not registering it to allThreads yet.
2. Thread: The thread is running.
3. Thread: The thread finishes its execution before the thread is registered into allThreads.
4. Thread: The thread unregisters itself from allThreads.
5. Caller: Registers the new thread to allThreads after it already finished its execution.
6. The thread is never removed from allThreads.
This patch adds m_didUnregisterFromAllThreads flag to Thread, and add the thread to allThreads only when this flag is false.
Covered by LayoutTests/inspector/cpu-profiler/threads.html.
* wtf/Threading.cpp:
(WTF::Thread::create):
(WTF::Thread::didExit):
* wtf/Threading.h:
(WTF::Thread::Thread):
Modified Paths
Diff
Modified: trunk/Source/WTF/ChangeLog (260681 => 260682)
--- trunk/Source/WTF/ChangeLog 2020-04-24 23:53:34 UTC (rev 260681)
+++ trunk/Source/WTF/ChangeLog 2020-04-24 23:56:38 UTC (rev 260682)
@@ -1,3 +1,30 @@
+2020-04-24 Yusuke Suzuki <[email protected]>
+
+ [WTF] allThreads registration is racy with allThreads unregistration
+ https://bugs.webkit.org/show_bug.cgi?id=210995
+ <rdar://problem/61609690>
+
+ Reviewed by Keith Miller.
+
+ There is a race between registering a thread to allThreads and unregistering a thread from allThreads.
+
+ 1. Caller: A new thread is created, but not registering it to allThreads yet.
+ 2. Thread: The thread is running.
+ 3. Thread: The thread finishes its execution before the thread is registered into allThreads.
+ 4. Thread: The thread unregisters itself from allThreads.
+ 5. Caller: Registers the new thread to allThreads after it already finished its execution.
+ 6. The thread is never removed from allThreads.
+
+ This patch adds m_didUnregisterFromAllThreads flag to Thread, and add the thread to allThreads only when this flag is false.
+
+ Covered by LayoutTests/inspector/cpu-profiler/threads.html.
+
+ * wtf/Threading.cpp:
+ (WTF::Thread::create):
+ (WTF::Thread::didExit):
+ * wtf/Threading.h:
+ (WTF::Thread::Thread):
+
2020-04-24 Alex Christensen <[email protected]>
REGRESSION(260485) Payment requests don't do anything
Modified: trunk/Source/WTF/wtf/Threading.cpp (260681 => 260682)
--- trunk/Source/WTF/wtf/Threading.cpp 2020-04-24 23:53:34 UTC (rev 260681)
+++ trunk/Source/WTF/wtf/Threading.cpp 2020-04-24 23:56:38 UTC (rev 260682)
@@ -196,9 +196,14 @@
#endif
}
+ // We must register threads here since threads registered in allThreads are expected to have complete thread data which can be initialized in launched thread side.
+ // However, it is also possible that the launched thread has finished its execution before it is registered in allThreads here! In this case, the thread has already
+ // called Thread::didExit to unregister itself from allThreads. Registering such a thread will register a stale thread pointer to allThreads, which will not be removed
+ // even after Thread is destroyed. Register a thread only when it has not unregistered itself from allThreads yet.
{
- LockHolder lock(allThreadsMutex());
- allThreads(lock).add(&thread.get());
+ auto locker = holdLock(allThreadsMutex());
+ if (!thread->m_didUnregisterFromAllThreads)
+ allThreads(locker).add(thread.ptr());
}
ASSERT(!thread->stack().isEmpty());
@@ -222,8 +227,9 @@
void Thread::didExit()
{
{
- LockHolder lock(allThreadsMutex());
- allThreads(lock).remove(this);
+ auto locker = holdLock(allThreadsMutex());
+ allThreads(locker).remove(this);
+ m_didUnregisterFromAllThreads = true;
}
if (shouldRemoveThreadFromThreadGroup()) {
Modified: trunk/Source/WTF/wtf/Threading.h (260681 => 260682)
--- trunk/Source/WTF/wtf/Threading.h 2020-04-24 23:53:34 UTC (rev 260681)
+++ trunk/Source/WTF/wtf/Threading.h 2020-04-24 23:56:38 UTC (rev 260682)
@@ -316,6 +316,8 @@
bool m_isCompilationThread: 1;
unsigned m_gcThreadType : 2;
+ bool m_didUnregisterFromAllThreads { false };
+
// Lock & ParkingLot rely on ThreadSpecific. But Thread object can be destroyed even after ThreadSpecific things are destroyed.
// Use WordLock since WordLock does not depend on ThreadSpecific and this "Thread".
WordLock m_mutex;
Modified: trunk/Source/WebCore/ChangeLog (260681 => 260682)
--- trunk/Source/WebCore/ChangeLog 2020-04-24 23:53:34 UTC (rev 260681)
+++ trunk/Source/WebCore/ChangeLog 2020-04-24 23:56:38 UTC (rev 260682)
@@ -1,3 +1,14 @@
+2020-04-24 Yusuke Suzuki <[email protected]>
+
+ [WTF] allThreads registration is racy with allThreads unregistration
+ https://bugs.webkit.org/show_bug.cgi?id=210995
+ <rdar://problem/61609690>
+
+ Reviewed by Keith Miller.
+
+ * page/cocoa/ResourceUsageThreadCocoa.mm:
+ (WebCore::ResourceUsageThread::platformCollectCPUData):
+
2020-04-24 Wenson Hsieh <[email protected]>
Make some more adjustments to TextManipulationController's paragraph boundary heuristic
Modified: trunk/Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm (260681 => 260682)
--- trunk/Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm 2020-04-24 23:53:34 UTC (rev 260681)
+++ trunk/Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm 2020-04-24 23:56:38 UTC (rev 260682)
@@ -156,8 +156,8 @@
HashSet<mach_port_t> knownWebKitThreads;
{
- LockHolder lock(Thread::allThreadsMutex());
- for (auto* thread : Thread::allThreads(lock)) {
+ auto locker = holdLock(Thread::allThreadsMutex());
+ for (auto* thread : Thread::allThreads(locker)) {
mach_port_t machThread = thread->machThread();
if (machThread != MACH_PORT_NULL)
knownWebKitThreads.add(machThread);
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
