[webkit-reviews] review denied: [Bug 199857] [WTF] Thread::removeFromThreadGroup leaks weak pointers. : [Attachment 374293] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jul 26 19:37:40 PDT 2019

Yusuke Suzuki <ysuzuki at apple.com> has denied Takashi Komori
<Takashi.Komori at sony.com>'s request for review:
Bug 199857: [WTF] Thread::removeFromThreadGroup leaks weak pointers.

Attachment 374293: Patch


--- Comment #4 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 374293
  --> https://bugs.webkit.org/attachment.cgi?id=374293

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

Or, another simple way is that holding struct { ThreadGroup* threadGroup;
std::weak_ptr<ThreadGroup> weakPtr } in a Vector, but I think HashMap one is

More information about the webkit-reviews mailing list