ThreadGroup's destructor calls Thread::removeFromThreadGroup and tries to remove its weak pointer from Thread.m_threadGroups. As the call is from destructor the instance of ThreadGroup is already released and the removing always fail. As a result, the size of m_thraeadGroups won't decrease.
Created attachment 374292 [details] Patch
Created attachment 374293 [details] Patch
Comment on attachment 374293 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374293&action=review > Source/WTF/ChangeLog:8 > + This patch makes Thread::removeFromThreadGroup remove all released weak pointers. Is it possible to make a unit test for this bug?
Comment on attachment 374293 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374293&action=review Nice catch! Discussed with Mark about whether this works with the current protocol of ThreadGroup and Thread, and we think that the key is that we should remove ThreadGroup unconditionally without using `weakPtr.lock()`. To achieve it simply with O(1), I commented about the HashMap based approach here. Does it seem good for you too? > Source/WTF/wtf/Threading.cpp:259 > return sharedPtr.get() == &threadGroup; > return false; > }); Nice catch! I think the key is that, if we can just remove threadGroup unconditionally from this list, it should work. So, ideally, the code should be like, return &threadGroup == weakPtr.get(); Then it works. But the problem is that std::weak_ptr does not have an original raw pointer. So, can you change the code like, 1. Instead of holding a Vector<std::weak_ptr<ThreadGroup>>, let's hold HashMap<ThreadGroup*, std::weak_ptr<ThreadGroup>> 2. Remove the entry from that hash map in this function 3. In L214, iterate the hash map and collect shared_ptr from the value's weak_ptr Or, another simple way is that holding struct { ThreadGroup* threadGroup; std::weak_ptr<ThreadGroup> weakPtr } in a Vector, but I think HashMap one is better.
Comment on attachment 374293 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374293&action=review >> Source/WTF/ChangeLog:8 >> + This patch makes Thread::removeFromThreadGroup remove all released weak pointers. > > Is it possible to make a unit test for this bug? I think we can test it. Can you add a test to Tools/TestWebKitAPI/Tests/WTF/ThreadGroup.cpp? We can test it like, 1. Create a ThreadGroup 2. Add many Threads 3. Keep Threads live by using condition variable 4. Destroy ThreadGroup 5. Check the count of ThreadGroups of each Thread To get the value of (5), we need a method in Thread like, unsigned Thread::numberOfThreadGroups() { auto locker = holdLock(m_mutex); return m_threadGroups.size(); }
(In reply to Yusuke Suzuki from comment #5) > Comment on attachment 374293 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=374293&action=review > > >> Source/WTF/ChangeLog:8 > >> + This patch makes Thread::removeFromThreadGroup remove all released weak pointers. > > > > Is it possible to make a unit test for this bug? > > I think we can test it. Can you add a test to > Tools/TestWebKitAPI/Tests/WTF/ThreadGroup.cpp? > We can test it like, > > 1. Create a ThreadGroup > 2. Add many Threads > 3. Keep Threads live by using condition variable > 4. Destroy ThreadGroup > 5. Check the count of ThreadGroups of each Thread > > To get the value of (5), we need a method in Thread like, > > unsigned Thread::numberOfThreadGroups() > { > auto locker = holdLock(m_mutex); > return m_threadGroups.size(); > } Thank you for your detailed advice! I'll try to make a test.
Created attachment 375610 [details] Patch
Created attachment 375612 [details] Patch
Comment on attachment 375612 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375612&action=review Some early comments. The code looks good to me. > Tools/TestWebKitAPI/Tests/WTF/ThreadGroup.cpp:156 > + const int NumberOfThreads = 64; Use `unsigned` to avoid gtest build failure. > Tools/TestWebKitAPI/Tests/WTF/ThreadGroup.cpp:162 > + Vector<RefPtr<Thread>> threads; Use `Ref`. > Tools/TestWebKitAPI/Tests/WTF/ThreadGroup.cpp:165 > + for (int i = 0; i < NumberOfThreads; i++) { Use `unsigned`. > Tools/TestWebKitAPI/Tests/WTF/ThreadGroup.cpp:166 > + auto thread = Thread::create("ThreadGroupWorker", [&](void) { `(void)` is not necessary. > Tools/TestWebKitAPI/Tests/WTF/ThreadGroup.cpp:167 > + LockHolder locker(lock); Use `auto locker = holdLock(lock)`. > Tools/TestWebKitAPI/Tests/WTF/ThreadGroup.cpp:168 > + threadRunningCondition.wait(lock, [&](void) { `(void)` is not necessary. > Tools/TestWebKitAPI/Tests/WTF/ThreadGroup.cpp:184 > + for (int i = 0; i < NumberOfThreads; i++) Use `unsigned`.
Created attachment 375898 [details] Patch
Created attachment 375899 [details] Patch
Comment on attachment 375899 [details] Patch r=me
Comment on attachment 375899 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375899&action=review > Source/WTF/wtf/Threading.h:296 > + HashMap<ThreadGroup*, std::weak_ptr<ThreadGroup>> m_threadGroupMap; Future cleanup thought: Can we make HashSet<std::weak_ptr<ThreadGroup>> work for a case like this, rather than requiring use of a map?
Comment on attachment 375899 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375899&action=review >> Source/WTF/wtf/Threading.h:296 >> + HashMap<ThreadGroup*, std::weak_ptr<ThreadGroup>> m_threadGroupMap; > > Future cleanup thought: Can we make HashSet<std::weak_ptr<ThreadGroup>> work for a case like this, rather than requiring use of a map? Yeah, I would like to make this too :D Unfortunately, currently, `std::weak_ptr` does not have a way to get a raw pointer if the held object is dead (I would like to have `std::weak_ptr::get()`...). So we need our own WeakPtr implementation which is thread-safe and having a method (like WeakPtr::getRaw()) returning an original pointer regardless of liveness (thread-safe weak pointer is why we are using std::weak_ptr and std::shared_ptr instead of RefPtr and WeakPtr here).
Comment on attachment 375899 [details] Patch Clearing flags on attachment: 375899 Committed r248586: <https://trac.webkit.org/changeset/248586>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54239165>