Changeset 248586 in webkit

Timestamp:

Aug 12, 2019 8:01:11 PM (5 years ago)

Author:

commit-queue@webkit.org

Message:

[WTF] Thread::removeFromThreadGroup leaks weak pointers.
​https://bugs.webkit.org/show_bug.cgi?id=199857

Patch by Takashi Komori <​Takashi.Komori@sony.com> on 2019-08-12
Reviewed by Yusuke Suzuki.

Source/WTF:

Fix leaking of ThreadGroup's weak pointers.

Tests: WTF.ThreadGroupRemove API tests

• wtf/Threading.cpp:

(WTF::Thread::didExit):
(WTF::Thread::addToThreadGroup):
(WTF::Thread::removeFromThreadGroup):
(WTF::Thread::numberOfThreadGroups):

• wtf/Threading.h:

Tools:

• TestWebKitAPI/Tests/WTF/ThreadGroup.cpp:

(TestWebKitAPI::countThreadGroups):
(TestWebKitAPI::TEST):

Location:

trunk

Files:

• Source/WTF/ChangeLog (modified) (1 diff)
• Source/WTF/wtf/Threading.cpp (modified) (3 diffs)
• Source/WTF/wtf/Threading.h (modified) (3 diffs)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WTF/ThreadGroup.cpp (modified) (1 diff)

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2019-08-12  Takashi Komori  <Takashi.Komori@sony.com>
+
+        [WTF] Thread::removeFromThreadGroup leaks weak pointers.
+        https://bugs.webkit.org/show_bug.cgi?id=199857
+
+        Reviewed by Yusuke Suzuki.
+
+        Fix leaking of ThreadGroup's weak pointers.
+
+        Tests: WTF.ThreadGroupRemove API tests
+
+        * wtf/Threading.cpp:
+        (WTF::Thread::didExit):
+        (WTF::Thread::addToThreadGroup):
+        (WTF::Thread::removeFromThreadGroup):
+        (WTF::Thread::numberOfThreadGroups):
+        * wtf/Threading.h:
+
 2019-08-12  Sam Weinig  <weinig@apple.com>
 

trunk/Source/WTF/wtf/Threading.cpp

@@ -212 +212 @@
             {
                 auto locker = holdLock(m_mutex);
-                for (auto& threadGroup : m_threadGroups) {
+                for (auto& threadGroupPointerPair : m_threadGroupMap) {
                     // If ThreadGroup is just being destroyed,
                     // we do not need to perform unregistering.
-                    if (auto retained = threadGroup.lock())
+                    if (auto retained = threadGroupPointerPair.value.lock())
                         threadGroups.append(WTFMove(retained));
                 }
@@ -241 +241 @@
         return ThreadGroupAddResult::NotAdded;
     if (threadGroup.m_threads.add(*this).isNewEntry) {
-        m_threadGroups.append(threadGroup.weakFromThis());
+        m_threadGroupMap.add(&threadGroup, threadGroup.weakFromThis());
         return ThreadGroupAddResult::NewlyAdded;
     }
@@ -253 +253 @@
     if (m_isShuttingDown)
         return;
-    m_threadGroups.removeFirstMatching([&] (auto weakPtr) {
-        if (auto sharedPtr = weakPtr.lock())
-            return sharedPtr.get() == &threadGroup;
-        return false;
-    });
+    m_threadGroupMap.remove(&threadGroup);
+}
+
+unsigned Thread::numberOfThreadGroups()
+{
+    auto locker = holdLock(m_mutex);
+    return m_threadGroupMap.size();
 }
 

trunk/Source/WTF/wtf/Threading.h

@@ -37 +37 @@
 #include <wtf/FastTLS.h>
 #include <wtf/Function.h>
+#include <wtf/HashMap.h>
 #include <wtf/HashSet.h>
 #include <wtf/PlatformRegisters.h>
@@ -96 +97 @@
     WTF_EXPORT_PRIVATE static Lock& allThreadsMutex();
 
+    WTF_EXPORT_PRIVATE unsigned numberOfThreadGroups();
+
 #if OS(WINDOWS)
     // Returns ThreadIdentifier directly. It is useful if the user only cares about identity
@@ -291 +294 @@
     WordLock m_mutex;
     StackBounds m_stack { StackBounds::emptyBounds() };
-    Vector<std::weak_ptr<ThreadGroup>> m_threadGroups;
+    HashMap<ThreadGroup*, std::weak_ptr<ThreadGroup>> m_threadGroupMap;
     PlatformThreadHandle m_handle;
 #if OS(WINDOWS)

trunk/Tools/ChangeLog

@@ +1 @@
+2019-08-12  Takashi Komori  <Takashi.Komori@sony.com>
+
+        [WTF] Thread::removeFromThreadGroup leaks weak pointers.
+        https://bugs.webkit.org/show_bug.cgi?id=199857
+
+        Reviewed by Yusuke Suzuki.
+
+        * TestWebKitAPI/Tests/WTF/ThreadGroup.cpp:
+        (TestWebKitAPI::countThreadGroups):
+        (TestWebKitAPI::TEST):
+
 2019-08-12  Alexey Shvayka  <shvaikalesh@gmail.com>
 

trunk/Tools/TestWebKitAPI/Tests/WTF/ThreadGroup.cpp

@@ -143 +143 @@
 }
 
+static unsigned countThreadGroups(Vector<Ref<Thread>>& threads)
+{
+    unsigned count = 0;
+    for (auto& thread : threads)
+        count += thread->numberOfThreadGroups();
+
+    return count;
+}
+
+TEST(WTF, ThreadGroupRemove)
+{
+    const unsigned NumberOfThreads = 64;
+
+    Lock lock;
+    Condition threadRunningCondition;
+    bool threadRunning = true;
+
+    Vector<Ref<Thread>> threads;
+
+    auto threadGroup = ThreadGroup::create();
+    for (unsigned i = 0; i < NumberOfThreads; i++) {
+        auto thread = Thread::create("ThreadGroupWorker", [&]() {
+            auto locker = holdLock(lock);
+            threadRunningCondition.wait(lock, [&]() {
+                return !threadRunning;
+            });
+        });
+        threadGroup->add(thread.get());
+        threads.append(WTFMove(thread));
+    }
+
+    EXPECT_EQ(NumberOfThreads, countThreadGroups(threads));
+
+    threadGroup = nullptr;
+
+    EXPECT_EQ(0U, countThreadGroups(threads));
+
+    threadRunning = false;
+    threadRunningCondition.notifyAll();
+    for (unsigned i = 0; i < NumberOfThreads; i++)
+        threads[i]->waitForCompletion();
+}
+
 } // namespace TestWebKitAPI