Changeset 260682 in webkit

Timestamp:

Apr 24, 2020 4:56:38 PM (4 years ago)

Author:

ysuzuki@apple.com

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):

Location:

trunk/Source

Files:

• WTF/ChangeLog (modified) (1 diff)
• WTF/wtf/Threading.cpp (modified) (2 diffs)
• WTF/wtf/Threading.h (modified) (1 diff)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/page/cocoa/ResourceUsageThreadCocoa.mm (modified) (1 diff)

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2020-04-24  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [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  <achristensen@webkit.org>
 

trunk/Source/WTF/wtf/Threading.cpp

@@ -197 +197 @@
     }
 
-    {
-        LockHolder lock(allThreadsMutex());
-        allThreads(lock).add(&thread.get());
+    // 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.
+    {
+        auto locker = holdLock(allThreadsMutex());
+        if (!thread->m_didUnregisterFromAllThreads)
+            allThreads(locker).add(thread.ptr());
     }
 
@@ -223 +228 @@
 {
     {
-        LockHolder lock(allThreadsMutex());
-        allThreads(lock).remove(this);
+        auto locker = holdLock(allThreadsMutex());
+        allThreads(locker).remove(this);
+        m_didUnregisterFromAllThreads = true;
     }
 

trunk/Source/WTF/wtf/Threading.h

@@ -317 +317 @@
     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".

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-04-24  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [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  <wenson_hsieh@apple.com>
 

trunk/Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm

@@ -157 +157 @@
     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)