Summary: | Fix race condition in ConcurrentPtrHashSet. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, ews-watchlist, webkit-bug-importer, ysuzuki | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=222947 | ||||||||||||
Attachments: |
|
Description
Mark Lam
2021-03-16 00:36:37 PDT
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>. |