There exists a race condition where ConcurrentPtrHashSet::resizeIfNecessary() may not capture an entry added by ConcurrentPtrHashSet::addSlow(). rdar://74637896
Created attachment 423349 [details] proposed patch.
Created attachment 423350 [details] proposed patch.
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());`?
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.
Created attachment 423525 [details] proposed patch.
Created attachment 423526 [details] proposed patch.
Comment on attachment 423526 [details] proposed patch. r=me, make sense.
Thanks for the review. Landed in r274608: <http://trac.webkit.org/r274608>.