Bug 141554 - TimerBase::m_heapInsertionOrder calculation is racy
Summary: TimerBase::m_heapInsertionOrder calculation is racy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-02-12 22:24 PST by Alexey Proskuryakov
Modified: 2015-02-13 10:30 PST (History)
1 user (show)

See Also:


Attachments
proposed fix (2.63 KB, patch)
2015-02-12 23:29 PST, Alexey Proskuryakov
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.