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+

Description Alexey Proskuryakov 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.
Comment 1 Alexey Proskuryakov 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>.
Comment 2 Anders Carlsson 2015-02-13 08:05:59 PST 
Comment on attachment 246510 [details]
proposed fix

Just use uint64_t instead of uint_fast64_t.
Comment 3 Alexey Proskuryakov 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.