Bug 201576 – Make it safe to store a ThreadSafeRefCounted object in Ref & RefPtr safe inside its destructor

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

[Ryosuke Niwa]

Just as we made it safe to ref & deref a Node inside ~Node in the bug 195776,
we should make it safe to ref & deref an object which uses ThreadSafeRefCounted inside its destructor.

[Ryosuke Niwa]

Created attachment 378281 [details]
Patch

[EWS Watchlist]

Comment on attachment 378281 [details]
Patch

Attachment 378281 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13009210

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 378285 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[EWS Watchlist]

Comment on attachment 378281 [details]
Patch

Attachment 378281 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13009229

New failing tests:
wasm.yaml/wasm/references/anyref_globals.js.wasm-collect-continuously
stress/arith-trunc-on-various-types.js.default
stress/disable-gigacage-strings.js.disable-gigacage
stress/regress-170253.js.default
stress/get-array-length-concurrently-change-mode.js.ftl-eager
stress/op-negate-inline-cache.js.bytecode-cache
wasm.yaml/wasm/references/anyref_table.js.wasm-no-cjit-yes-tls-context
stress/to-number-throws-correct-exception.js.bytecode-cache
wasm.yaml/wasm/js-api/Instance.imports.exports.unicode.js.wasm-collect-continuously
stress/super-property-access-exceptions.js.default
microbenchmarks/super-get-by-id-with-this-polymorphic.js.default
wasm.yaml/wasm/spec-tests/float_exprs.wast.js.wasm-no-cjit-yes-tls-context
wasm.yaml/wasm/spec-tests/f32_cmp.wast.js.wasm-no-cjit-yes-tls-context
wasm.yaml/wasm/spec-tests/f64_cmp.wast.js.wasm-no-cjit-yes-tls-context
stress/arith-floor-on-various-types.js.default
stress/spread-optimized-properly.js.ftl-eager
wasm.yaml/wasm/references/anyref_globals.js.wasm-slow-memory
stress/map-with-nan.js.default
stress/test-out-of-memory.js.dfg-eager-no-cjit-validate
wasm.yaml/wasm/js-api/wasm-to-wasm-bad-signature.js.wasm-no-air
stress/call-apply-exponential-bytecode-size.js.ftl-eager-no-cjit
stress/disable-gigacage-typed-arrays.js.disable-gigacage
stress/arith-negate-on-various-types.js.default
stress/disable-gigacage-arrays.js.disable-gigacage
wasm.yaml/wasm/spec-tests/f32.wast.js.wasm-no-cjit-yes-tls-context
wasm.yaml/wasm/references/anyref_globals.js.wasm-no-cjit-yes-tls-context
stress/test-out-of-memory.js.ftl-eager-no-cjit
stress/class-subclassing-function.js.ftl-eager
stress/arith-add-on-double-array-with-holes.js.ftl-eager
wasm.yaml/wasm/spec-tests/f64.wast.js.wasm-no-tls-context
microbenchmarks/eval-cached.js.ftl-eager
wasm.yaml/wasm/references/anyref_table.js.wasm-eager-jettison
stress/to-number-throws-correct-exception.js.ftl-eager
wasm.yaml/wasm/spec-tests/float_misc.wast.js.wasm-no-cjit-yes-tls-context
stress/fold-multi-get-by-offset-to-get-by-offset-without-folding-the-structure-check.js.ftl-eager
wasm.yaml/wasm/js-api/wasm-to-wasm-bad-signature.js.wasm-eager-jettison
stress/to-number-throws-correct-exception.js.default
stress/arith-ceil-on-various-types.js.default
stress/tagged-template-object-collect.js.dfg-eager-no-cjit-validate
stress/test-out-of-memory.js.ftl-eager
stress/ftl-gettypedarrayoffset-wasteful.js.ftl-eager-no-cjit
wasm.yaml/wasm/references/anyref_table.js.wasm-no-tls-context
microbenchmarks/super-get-by-val-with-this-monomorphic.js.bytecode-cache
apiTests

[Ryosuke Niwa]

Comment on attachment 378281 [details]
Patch

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

> Source/WTF/wtf/ThreadSafeRefCounted.h:86
> +        unsigned tempRefCount = m_refCount - 1;
> +        if (!tempRefCount) {

Oh oops, this is obviously not gonna work. Not sure what the heck I was thinking.

[Ryosuke Niwa]

Created attachment 378310 [details]
Patch

[Ryosuke Niwa]

Hm... ThreadSafeRefCounted::deref() is pretty big. We should probably consider putting it into a separate function with NEVER_INLINE at some point.

[EWS Watchlist]

Comment on attachment 378310 [details]
Patch

Attachment 378310 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13012033

New failing tests:
apiTests

[Darin Adler]

Comment on attachment 378310 [details]
Patch

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

> Source/WTF/wtf/ThreadSafeRefCounted.h:79
> +        if (!--m_refCount) {
> +            m_refCount = 1;

This doesn’t seem safe. There’s a window where m_refCount is 0 before it gets changed back to 1.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #9)
> Comment on attachment 378310 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378310&action=review
> 
> > Source/WTF/wtf/ThreadSafeRefCounted.h:79
> > +        if (!--m_refCount) {
> > +            m_refCount = 1;
> 
> This doesn’t seem safe. There’s a window where m_refCount is 0 before it
> gets changed back to 1.

Indeed, it's not most thread-safe code. Here's my reasoning: if m_refCount had reached 0 and subsequently incremented by 1 by another thread, then that thread previously didn't have any Ref / RefPtr reference to this object yet created new one. That's already a bug on its own.

While it would be nice to prevent double delete in such a scenario as well, we're kind of hosed in such a situation because such a thread might as well start ref'ing this object long after it had already been deleted.

I figured we can address a mitigation for such a case in a different bug since this patch's primary purpose is to prevent ref/deref happening inside a destructor in the same thread from deleting the object twice, not about some other thread ref/deref'ing an object without validating that the object is still live.

[Darin Adler]

Comment on attachment 378310 [details]
Patch

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

>>> Source/WTF/wtf/ThreadSafeRefCounted.h:79
>>> +            m_refCount = 1;
>> 
>> This doesn’t seem safe. There’s a window where m_refCount is 0 before it gets changed back to 1.
> 
> Indeed, it's not most thread-safe code. Here's my reasoning: if m_refCount had reached 0 and subsequently incremented by 1 by another thread, then that thread previously didn't have any Ref / RefPtr reference to this object yet created new one. That's already a bug on its own.
> 
> While it would be nice to prevent double delete in such a scenario as well, we're kind of hosed in such a situation because such a thread might as well start ref'ing this object long after it had already been deleted.
> 
> I figured we can address a mitigation for such a case in a different bug since this patch's primary purpose is to prevent ref/deref happening inside a destructor in the same thread from deleting the object twice, not about some other thread ref/deref'ing an object without validating that the object is still live.

I think that if there was a comment I would understand this better.

To be clear, you are saying that it’s still forbidden to ref/deref inside the destructor? But that we make sure it doesn’t lead to double delete so the implications of the programming mistake are not as horrible.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #11)
> Comment on attachment 378310 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378310&action=review
> 
> >>> Source/WTF/wtf/ThreadSafeRefCounted.h:79
> >>> +            m_refCount = 1;
> >> 
> >> This doesn’t seem safe. There’s a window where m_refCount is 0 before it gets changed back to 1.
> > 
> > Indeed, it's not most thread-safe code. Here's my reasoning: if m_refCount had reached 0 and subsequently incremented by 1 by another thread, then that thread previously didn't have any Ref / RefPtr reference to this object yet created new one. That's already a bug on its own.
> > 
> > While it would be nice to prevent double delete in such a scenario as well, we're kind of hosed in such a situation because such a thread might as well start ref'ing this object long after it had already been deleted.
> > 
> > I figured we can address a mitigation for such a case in a different bug since this patch's primary purpose is to prevent ref/deref happening inside a destructor in the same thread from deleting the object twice, not about some other thread ref/deref'ing an object without validating that the object is still live.
> 
> I think that if there was a comment I would understand this better.

Yeah, I guess we should add a comment to that end.

> To be clear, you are saying that it’s still forbidden to ref/deref inside
> the destructor? But that we make sure it doesn’t lead to double delete so
> the implications of the programming mistake are not as horrible.

Yes. The bug 195776 (same work for Node) was motivated by an actual security bug which ended up storing a Node in Ref/RefPtr inside a subclass' destructor.

[Geoffrey Garen]

Comment on attachment 378310 [details]
Patch

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

>>>>> Source/WTF/wtf/ThreadSafeRefCounted.h:79
>>>>> +            m_refCount = 1;
>>>> 
>>>> This doesn’t seem safe. There’s a window where m_refCount is 0 before it gets changed back to 1.
>>> 
>>> Indeed, it's not most thread-safe code. Here's my reasoning: if m_refCount had reached 0 and subsequently incremented by 1 by another thread, then that thread previously didn't have any Ref / RefPtr reference to this object yet created new one. That's already a bug on its own.
>>> 
>>> While it would be nice to prevent double delete in such a scenario as well, we're kind of hosed in such a situation because such a thread might as well start ref'ing this object long after it had already been deleted.
>>> 
>>> I figured we can address a mitigation for such a case in a different bug since this patch's primary purpose is to prevent ref/deref happening inside a destructor in the same thread from deleting the object twice, not about some other thread ref/deref'ing an object without validating that the object is still live.
>> 
>> I think that if there was a comment I would understand this better.
>> 
>> To be clear, you are saying that it’s still forbidden to ref/deref inside the destructor? But that we make sure it doesn’t lead to double delete so the implications of the programming mistake are not as horrible.
> 
> Yeah, I guess we should add a comment to that end.

Maybe just return early if refcount is zero?

auto refCount = m_refCount;

// Protect against double destruction inside a destructor.
if (!refCount) {
    ASSERT(...)
    return;
}

m_refCount = refCount - 1;

return refCount == 1;

Also, why do this only in ThreadSafeRefCounted and not RefCounted?

[Darin Adler]

It’s already done in RefCounted apparently.

[Ryosuke Niwa]

Created attachment 380078 [details]
Updated for ToT

[Ryosuke Niwa]

Ping reviewers.

[Geoffrey Garen]

Comment on attachment 380078 [details]
Updated for ToT

r=me

It's clever to --m_refCount and then set back to 1. I think that's the right solution even though it doesn't fix a concurrent race. I'll write down my reasoning here, in case we ever reconsider:

(1) Unconditional atomic increment/decrement is free on modern ARM hardware (as measured recently by me on MallocBench, PLT, Speedometer2, and JetStream2); but conditional increment/decrement is not free. It's nice for performance, and therefore our ability to use ThreadSafeRefCounted more, if our approach uses unconditional decrement here.

(2) Our goal, as I understand it, is really to defend against re-entrant destruction caused by creating a new RefPtr during destruction. That goal seems a lot more important than protecting against racy raw calls to ref() / deref() because it directly impacts our ability to use RefPtr in more places.

[Ryosuke Niwa]

Thanks for the review, and yes, that summaries the reasoning.

I'll add to (2) that in those cases where ref() / deref() is called concurrently, such a thread is already doing UAF by the virtue of the fact this thread has already entered the destructor at that point. The fact m_refCount momentarily reached 0 would mean that the offending thread did not have any Ref or RefPtr to this object yet created new Ref / RefPtr while this thread was proceeding with the deletion. In that way, there isn't really a way to prevent the concurrent case because the offending thread's ref() could have happened long after the object had been deleted by this thread regardless of whether we use conditional decrement or not.

[Ryosuke Niwa]

Comment on attachment 380078 [details]
Updated for ToT

Clearing flags on attachment: 380078

Committed r250762: <https://trac.webkit.org/changeset/250762>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56001847>

[Ryosuke Niwa]

Oops, this broke JSC tests:

perl ./Tools/Scripts/run-javascriptcore-tests --cloop --no-build --no-jsc-stress --no-fail-fast --json-output=jsc_cloop.json --debug

...

ASSERTION FAILED: !m_deletionHasBegun
/Volumes/Data/slave/highsierra-cloop-debug/build/WebKitBuild/Debug/usr/local/include/wtf/ThreadSafeRefCounted.h(58) : void WTF::ThreadSafeRefCountedBase::ref() const
1   0x10b3b0679 WTFCrash
2   0x10b3b0699 WTFCrashWithSecurityImplication
3   0x10b3c539b WTF::ThreadSafeRefCountedBase::ref() const
4   0x10bae3f51 void WTF::refIfNotNull<JSC::VM>(JSC::VM*)
5   0x10bae3f14 WTF::RefPtr<JSC::VM, WTF::DumbPtrTraits<JSC::VM> >::RefPtr(JSC::VM*)
6   0x10bae3ead WTF::RefPtr<JSC::VM, WTF::DumbPtrTraits<JSC::VM> >::RefPtr(JSC::VM*)
7   0x10bf02649 JSC::JSLock::DropAllLocks::DropAllLocks(JSC::VM*)
8   0x10bf0278d JSC::JSLock::DropAllLocks::DropAllLocks(JSC::VM*)
9   0x10bf028ad JSC::JSLock::DropAllLocks::DropAllLocks(JSC::VM&)
10  0x10b98df7e JSC::Heap::releaseDelayedReleasedObjects()
11  0x10b98dbb6 JSC::Heap::lastChanceToFinalize()
12  0x10c08ef6c JSC::VM::~VM()
13  0x10c092205 JSC::VM::~VM()
14  0x10b6b4e8a WTF::ThreadSafeRefCounted<JSC::VM, (WTF::DestructionThread)0>::deref() const::'lambda'()::operator()() const
15  0x10b69478d WTF::ThreadSafeRefCounted<JSC::VM, (WTF::DestructionThread)0>::deref() const
16  0x10bae3e81 void WTF::derefIfNotNull<JSC::VM>(JSC::VM*)
17  0x10bacb817 WTF::RefPtr<JSC::VM, WTF::DumbPtrTraits<JSC::VM> >::operator=(std::nullptr_t)
18  0x10bf0168a JSC::JSLockHolder::~JSLockHolder()
19  0x10bf01705 JSC::JSLockHolder::~JSLockHolder()
20  0x10b69473b JSContextGroupRelease
21  0x10b606e74 -[JSVirtualMachine dealloc]
22  0x7fff53f0e042 (anonymous namespace)::AutoreleasePoolPage::pop(void*)
23  0x10b2f0d14 testToString()
24  0x10b2ef630 testObjectiveCAPI
25  0x10b2d4ff8 main
26  0x7fff54b2f015 start
27  0x1

This because of the following code:

JSLock::DropAllLocks::DropAllLocks(VM* vm)
    : m_droppedLockCount(0)
    // If the VM is in the middle of being destroyed then we don't want to resurrect it
    // by allowing DropAllLocks to ref it. By this point the JSLock has already been 
    // released anyways, so it doesn't matter that DropAllLocks is a no-op.
    , m_vm(vm->refCount() ? vm : nullptr)
{

[WebKit Commit Bot]

Re-opened since this is blocked by bug 202609

[Ryosuke Niwa]

Created attachment 380721 [details]
Fixed JSC tests

[Ryosuke Niwa]

Comment on attachment 380721 [details]
Fixed JSC tests

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

> Source/JavaScriptCore/runtime/JSLock.cpp:-282
> -    , m_vm(vm->refCount() ? vm : nullptr)

Note that this code was added by https://trac.webkit.org/changeset/165074/webkit.

[Mark Lam]

Comment on attachment 380721 [details]
Fixed JSC tests

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

> Source/WTF/wtf/ThreadSafeRefCounted.h:89
> +        if (UNLIKELY(!--m_refCount)) {
> +            // Setting m_refCount to 1 here prevents double delete within the destructor but not from another thread
> +            // since such a thread could have ref'ed this object long after it had been deleted. See webkit.org/b/201576.
> +            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?

[Mark Lam]

Comment on attachment 380721 [details]
Fixed JSC tests

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

> Source/JavaScriptCore/heap/Heap.cpp:512
> +                // If the VM is in the middle of being destroyed then we don't want to resurrect it
> +                // by allowing DropAllLocks to ref it. By this point the JSLock has already been 
> +                // released anyways, so it doesn't matter that DropAllLocks is a no-op.
> +
>                  // We need to drop locks before calling out to arbitrary code.
> -                JSLock::DropAllLocks dropAllLocks(m_vm);
> +                JSLock::DropAllLocks dropAllLocks(m_isShuttingDown ? nullptr : &m_vm);

nit: It seems hokey to gave the client of DropAllLocks check if the VM, by proxy of the Heap, is already shutting down in order to do the right thing here.  Can we just encapsulate this in DropAllLocks itself?  See below ...

>> Source/JavaScriptCore/runtime/JSLock.cpp:-282
>> -    , m_vm(vm->refCount() ? vm : nullptr)
> 
> Note that this code was added by https://trac.webkit.org/changeset/165074/webkit.

I suggest doing the shutdown check here instead like so:
    , m_vm(vm->heap.isShuttingDown() ? nullptr : vm)

>> 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?

[Ryosuke Niwa]

(In reply to Mark Lam from comment #27)
> Comment on attachment 380721 [details]
> Fixed JSC tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380721&action=review
>
> >> 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).

[Mark Lam]

Comment on attachment 380721 [details]
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.

[Mark Lam]

Regarding ChangeLog suggestion, alternatively, change the bug title to “Make a ThreadSafeRefCounted object safe to assign to a Reg/RefPtr inside its destructor”.  The current title’s reference to “ref & deref” did not strongly suggest to me that the 2 operations must be paired and done in that order.  I misread it as it’s safe to ref & deref arbitrarily in the destructor and still not get double frees.

[Geoffrey Garen]

Comment on attachment 380721 [details]
Fixed JSC tests

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

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

Maybe just change "prevents double delete" to "prevents RefPtr creating during destruction from triggering recursive delete".

[Ryosuke Niwa]

Committed r251034: <https://trac.webkit.org/changeset/251034>