Bug 141554

Summary: TimerBase::m_heapInsertionOrder calculation is racy
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: PlatformAssignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal CC: darin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed fix andersca: review+

Alexey Proskuryakov
Reported 2015-02-12 22:24:44 PST
TimerBase is used from multiple threads, and insertion order can get incorrect if the threads start overwriting the value.
Attachments
proposed fix (2.63 KB, patch)
2015-02-12 23:29 PST, Alexey Proskuryakov 		andersca: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 2015-02-12 23:29:22 PST
Created attachment 246510 [details] proposed fix Fingers crossed that this won't trigger a static destructor warning anywhere (it did build fine locally for me). I'm not sure if it is legitimate to use uint64_t here, that's not one of the types listed for specializations at <http://en.cppreference.com/w/cpp/atomic/atomic>.
Anders Carlsson
Comment 2 2015-02-13 08:05:59 PST
Comment on attachment 246510 [details] proposed fix Just use uint64_t instead of uint_fast64_t.
Alexey Proskuryakov
Comment 3 2015-02-13 10:30:34 PST
Committed <http://trac.webkit.org/r180058>. Anders also noted that the static object doesn't need to be static, it can be per-thread (perhaps part of ThreadTimers). Looking at the code, it seems like it could use a bit of refactoring for which functions are in TimerBase, and which are in ThreadTimers. I noticed that the counter doesn't actually have a problem with overflow, so I kept it as unsigned. // We need to look at the difference of the insertion orders instead of comparing the two // outright in case of overflow.
Note You need to log in before you can comment on or make changes to this bug.