Bug 199857 – [WTF] Thread::removeFromThreadGroup leaks weak pointers.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Takashi Komori]

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.

[Takashi Komori]

Created attachment 374292 [details]
Patch

[Takashi Komori]

Created attachment 374293 [details]
Patch

[Brent Fulgham]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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();
}

[Takashi Komori]

(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.

[Takashi Komori]

Created attachment 375610 [details]
Patch

[Takashi Komori]

Created attachment 375612 [details]
Patch

[Yusuke Suzuki]

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`.

[Takashi Komori]

Created attachment 375898 [details]
Patch

[Takashi Komori]

Created attachment 375899 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 375899 [details]
Patch

r=me

[Darin Adler]

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?

[Yusuke Suzuki]

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

[WebKit Commit Bot]

Comment on attachment 375899 [details]
Patch

Clearing flags on attachment: 375899

Committed r248586: <https://trac.webkit.org/changeset/248586>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54239165>