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

[bugzilla-daemon at webkit.org]

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.
https://bugs.webkit.org/show_bug.cgi?id=199857

Attachment 374293: Patch

https://bugs.webkit.org/attachment.cgi?id=374293&action=review




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