[webkit-reviews] review granted: [Bug 201576] Make a ThreadSafeRefCounted object safe to ref & deref inside its destructor : [Attachment 380721] Fixed JSC tests

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 201576: Make a ThreadSafeRefCounted object safe to ref & deref inside its
destructor
https://bugs.webkit.org/show_bug.cgi?id=201576

Attachment 380721: Fixed JSC tests

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




--- Comment #29 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 380721
  --> https://bugs.webkit.org/attachment.cgi?id=380721
Fixed JSC tests

View in context: https://bugs.webkit.org/attachment.cgi?id=380721&action=review

r=me with the DropAllLocks VM shutdown check done in its constructor instead of
its client.  The pre-existing refCount check there was trying to achieve the
same goal, but was unfortunately not effective enough.

>>>> Source/WTF/wtf/ThreadSafeRefCounted.h:89
>>>> +		  m_refCount = 1;
>>> 
>>> I'm not sure how this achieves what the comment says. 
https://bugs.webkit.org/show_bug.cgi?id=201576#c0 makes reference to doing the
same thing as webkit.org/b/195776, which in turn says to use String's
increment/decrement strategy of inc/dec'ing by an s_refCountIncrement of 2. 
With String, setting refCount = 1 would prevent it from every decrementing to 0
again, thereby preventing a double free.  In the case here, m_refCount is being
decremented by 1 each time.  Setting m_refCount = 1 here doesn't prevent it
from being decremented to 0 again.  Did you intend to change
ThreadSafeRefCountedBase to use a s_refCountIncrement of 2?
>> 
>> I see that this has already been discussed in
https://bugs.webkit.org/show_bug.cgi?id=201576#c9, and
https://bugs.webkit.org/show_bug.cgi?id=201576#c17.  I still don't see how this
prevents a double delete.  Consider this scenario:
>> 
>> 1. m_refCount == 1, and derefBase() is called in
ThreadSafeRefCounted::deref(): derefBase() sets m_refCount = 1 and returns true
i.e. deletion is executed.
>> 2. a bug occurs, and we call ThreadSafeRefCounted::deref() again:
derefBase() decs m_refCount to 0, sets it back to 1, and returns true again
i.e. double deletion is executed!
>> 
>> Am I missing something here?
> 
> This patch isn't intending to fix a bug that someone might be calling deref()
without ref(), or ref() on a potentially deleted object. This patch's sole
purpose is to prevent storing the object to a Ref / RefPtr inside object's
destructor not result in double delete. If someone is calling deref() before
calling ref() then it would result in UAF at some point regardless (e.g. such a
bug could keep deref()'ing an object until it gets deleted even if it had other
Ref/RefPtr to it).

OK, I see what I was missing: in the destructor, with m_refCount set to 1,
storing the object in any Ref/RefPtr will necessarily increment m_refCount
above 1, thereby ensuring that destruction of said Ref/RefPtr is guaranteed to
not decrement m_refCount down to 0.  This (the decrement can only happen after
an increment) is what I was missing.

nit: maybe I was just too dense, or perhaps a little further clarification in
the ChangeLog (about how this ensures a non-zero refCount in the destructor
because assignment of this object to a Ref/RefPtr always sees an increment
before a decrement) may help.