Bug 23488

Created attachment 26957 [details] Proposed patch.

Created attachment 26964 [details] Proposed patch, v2 Thanks to David Levin who noticed a few coding style issues! Updated patch.

Created attachment 26985 [details] Updated patch Looking at it a bit more, fixed following: - in multiple threads case, the timers can be created before worker thread initializes SharedTimer, and SharedTimer pointer may be set to 0 before all the timers are removed. Add check to avoid calling to SharedTimer when it's 0. (in setSharedTimer, updateSharedTimer) - made ThreadTimers::m_timerHeap an instance of a Vector rather then a pointer to avoid it leaking when ThreadTimers gets destroyed (it does if its thread is not a main one)

Comment on attachment 26985 [details] Updated patch > + class SharedTimer { > + public: > + virtual ~SharedTimer() {} > + virtual void setFiredFunction(void (*)()) = 0; > + // The fire time is relative to the classic POSIX epoch of January 1, 1970, > + // as the result of currentTime() is. > + virtual void setFireTime(double) = 0; > + virtual void stop() = 0; > + }; The formatting was slightly better in the old version, where there was a blank line before the comment. It's a little harder to understand what the comment is grouped with here with everything mushed together. > +Vector<TimerBase*>* ThreadTimers::timerHeap() > +{ > + return &m_timerHeap; > +} I think this should return a reference instead of a pointer. Callers currently have code to handle 0, and I'm not sure why. > +HashSet<const TimerBase*>* ThreadTimers::timersReadyToFire() > +{ > + return m_timersReadyToFire; > +} This too. > - ASSERT(m_index < (timerHeap ? static_cast<int>(timerHeap->size()) : 0)); > + ASSERT(m_index < (timerHeap() ? static_cast<int>(timerHeap()->size()) : 0)); Can timerHeap() be zero? If not, lets remove the unneeded code. > - ASSERT_UNUSED(offset, m_index + offset <= (timerHeap ? static_cast<int>(timerHeap->size()) : 0)); > + ASSERT_UNUSED(offset, m_index + offset <= (timerHeap() ? static_cast<int>(timerHeap()->size()) : 0)); Same question here. > - return m_nextFireTime || (timersReadyToFire && timersReadyToFire->contains(this)); > + return m_nextFireTime || (timersReadyToFire() && timersReadyToFire()->contains(this)); Can timersReadToFire() be zero? If not, lets remove the unneeded code. > +class SharedTimer; Forward declarations should all go at the top of the file, not in the middle. > +// A collection of timers per thread. Kept in ThreadGlobalData. > +class ThreadTimers : Noncopyable { We normally want one class per header file. Can this class go in its own header? Also, can it be private? Do source files using the Timer class need to see this class's definition? Can TimerBase still be the public interface to fireTimersInNestedEventLoop -- after all, it can just call through to the ThreadTimers function. I think the patch would be so much better if you used references instead of pointers that I'm going to say review- and wait for a new patch. But generally, this patch seems great, and I was really close to saying review+.

It's critical to keep the often-used header as small as possible. I'd like to see the new version of this patch keep Timer.h small and put new declarations either in a new .h file or possibly in the .cpp file.

Created attachment 27008 [details] Review comments addressed. Thanks for review! I think this patch addresses all the comments: - added vertical space to SharedTimer.h - moved fireTimersInNestedEventLoop back as static to TimerBase. It redirects to ThreadTimers internally. So other source files do not need to know about ThreadTimers. - ThreadTimers::timerHeap() now returns reference, not a pointer. Removed checks for zero in callers. - ditto for the ThreadTimers::timersReadyToFire, but with a twist: this one had a dual purpose. It was used to point to a stack-allocated set of timers and also serve as reentrancy guard (this is why it could be 0 before and some checks for 0 were necessary). However, this wasn't very good for readability so I've split this into 2 variables - bool m_firingTimers and m_timersReadyToFire that is never 0 (but can be empty). - split ThreadTimers off into a separate .h and .cpp files. Couldn't make it private since it has to be used in ThreadGlobalData.cpp as well as in Timer.cpp. - updated ChangeLog

Comment on attachment 27008 [details] Review comments addressed. > +static MainThreadSharedTimer* mainThreadSharedTimer() > +{ > + static MainThreadSharedTimer* timer = 0; > + if (!timer) > + timer = new MainThreadSharedTimer(); > + return timer; > +} You can get the same behavior without an if statement. static MainThreadSharedTimer* mainThreadSharedTimer() { static MainThreadSharedTimer* timer = new MainThreadSharedTimer; return timer; } > +// Also, SharedTimer can be replaced with 0 before all timers are destructed. Should say "destroyed" here, not "destructed". Looks fine, r=me

Created attachment 27170 [details] Final version Thanks!!! Addressed latest comments (removed 'if' and fixed comment) and resolved against current tree. Can be landed.

Committing to http://svn.webkit.org/repository/webkit/trunk ... C WebCore/platform/ThreadGlobalData.h => WebCore/platform/ThreadTimers.h M WebCore/ChangeLog M WebCore/GNUmakefile.am M WebCore/WebCore.pro M WebCore/WebCore.scons M WebCore/WebCore.vcproj/WebCore.vcproj M WebCore/WebCore.xcodeproj/project.pbxproj M WebCore/WebCoreSources.bkl M WebCore/platform/SharedTimer.h M WebCore/platform/ThreadGlobalData.cpp M WebCore/platform/ThreadGlobalData.h A WebCore/platform/ThreadTimers.cpp M WebCore/platform/Timer.cpp M WebCore/platform/Timer.h Committed r40393 I'm not sure what the C means during the commit. Hopefully not pain, death, or suffering.