[WTF] Thread::removeFromThreadGroup leaks weak pointers.
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 13 Aug 2019 03:01:11 +0000 (03:01 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 13 Aug 2019 03:01:11 +0000 (03:01 +0000)
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):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@248586 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WTF/ChangeLog
Source/WTF/wtf/Threading.cpp
Source/WTF/wtf/Threading.h
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WTF/ThreadGroup.cpp

index b9da92d..b5fe367 100644 (file)
@@ -1,3 +1,21 @@
+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>
 
         Replace multiparameter overloads of append() in StringBuilder as a first step toward standardizinging on the flexibleAppend() implementation
index 07e7d0f..793c4ec 100644 (file)
@@ -211,10 +211,10 @@ void Thread::didExit()
             Vector<std::shared_ptr<ThreadGroup>> threadGroups;
             {
                 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));
                 }
                 m_isShuttingDown = true;
@@ -240,7 +240,7 @@ ThreadGroupAddResult Thread::addToThreadGroup(const AbstractLocker& threadGroupL
     if (m_isShuttingDown)
         return ThreadGroupAddResult::NotAdded;
     if (threadGroup.m_threads.add(*this).isNewEntry) {
-        m_threadGroups.append(threadGroup.weakFromThis());
+        m_threadGroupMap.add(&threadGroup, threadGroup.weakFromThis());
         return ThreadGroupAddResult::NewlyAdded;
     }
     return ThreadGroupAddResult::AlreadyAdded;
@@ -252,11 +252,13 @@ void Thread::removeFromThreadGroup(const AbstractLocker& threadGroupLocker, Thre
     auto locker = holdLock(m_mutex);
     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();
 }
 
 bool Thread::exchangeIsCompilationThread(bool newValue)
index 6953f6e..c4d00b2 100644 (file)
@@ -36,6 +36,7 @@
 #include <wtf/Expected.h>
 #include <wtf/FastTLS.h>
 #include <wtf/Function.h>
+#include <wtf/HashMap.h>
 #include <wtf/HashSet.h>
 #include <wtf/PlatformRegisters.h>
 #include <wtf/Ref.h>
@@ -95,6 +96,8 @@ public:
     WTF_EXPORT_PRIVATE static HashSet<Thread*>& allThreads(const LockHolder&);
     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
     // of threads. At that time, users should know that holding this ThreadIdentifier does not ensure
@@ -290,7 +293,7 @@ protected:
     // Use WordLock since WordLock does not depend on ThreadSpecific and this "Thread".
     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)
     ThreadIdentifier m_id { 0 };
index bfb6dc3..62ff3ee 100644 (file)
@@ -1,3 +1,14 @@
+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>
 
         AX: Homebrew is not allowed to run any script under sudo.
index 291a3f0..f40eda8 100644 (file)
@@ -142,4 +142,47 @@ TEST(WTF, ThreadGroupAddDuplicateThreads)
     }
 }
 
+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