Bug 223241 – Fix race condition in ConcurrentPtrHashSet.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

There exists a race condition where ConcurrentPtrHashSet::resizeIfNecessary() may not capture an entry added by ConcurrentPtrHashSet::addSlow().

rdar://74637896

[Mark Lam]

Created attachment 423349 [details]
proposed patch.

[Mark Lam]

Created attachment 423350 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 423350 [details]
proposed patch.

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

r=me, looks good!

> Source/WTF/wtf/ConcurrentPtrHashSet.cpp:157
> +    // addSlow() will always start by exchangeAdd'ing 1 to the current m_table's
> +    // load value before checking if it exceeds its max allowed load. For the
> +    // real m_table, this is not an issue because at most, it will accummulate
> +    // up to N extra adds above max load, where N is the number of GC marker
> +    // threads. However, if m_table may be replaced with m_stubTable for each
> +    // resize operation. As a result, the cummulative error on its load value
> +    // may far exceed N (as specified above). To fix this, we always reset it
> +    // here to prevent an overflow. Note: a load of stubDefaultLoadValue means
> +    // that m_stubTable is full since its size is 0.
> +    //
> +    // In practice, this won't matter because we most likely won't do so many
> +    // resize operations such that this will get to the point of overflowing.
> +    // However, since resizing is not in the fast path, let's just be pedantic
> +    // and reset it for correctness.
> +    m_stubTable.load.store(Table::stubDefaultLoadValue);

Should we reset it after `m_table.store(newTable.get());`?

[Mark Lam]

Comment on attachment 423350 [details]
proposed patch.

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

>> Source/WTF/wtf/ConcurrentPtrHashSet.cpp:157
>> +    m_stubTable.load.store(Table::stubDefaultLoadValue);
> 
> Should we reset it after `m_table.store(newTable.get());`?

Good idea.  I will do that.

[Mark Lam]

Created attachment 423525 [details]
proposed patch.

[Mark Lam]

Created attachment 423526 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 423526 [details]
proposed patch.

r=me, make sense.

[Mark Lam]

Thanks for the review.  Landed in r274608: <http://trac.webkit.org/r274608>.