[webkit-reviews] review granted: [Bug 23865] Safari can be frozen by rapidly adding timers : [Attachment 38952] Updated patch.

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Dmitry Titov
<dimich at chromium.org>'s request for review:
Bug 23865: Safari can be frozen by rapidly adding timers
https://bugs.webkit.org/show_bug.cgi?id=23865

Attachment 38952: Updated patch.
https://bugs.webkit.org/attachment.cgi?id=38952&action=review

------- Additional Comments from David Levin <levin at chromium.org>
This is basically equivalent to what was there before with the addition of a
maximum amount of time to process the timers.

The removal of m_timersReadyToFire may cause a newly added timer to callback
nearly immediately, but this should only happen for at most a millisecond
(depending on the resolution of currentTime is that low) until currentTime()
advances, so that seems fine.

A few nits below to fix on landing.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +2009-09-02  Dmitry Titov  <dimich at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +

> +	   https://bugs.webkit.org/show_bug.cgi?id=23865
> +
> +	   Prevent UI freeze in case when too many timers are in the timer
queue.
> +	   The fix measures the elapsed time while executing timers. If we have
too many

> +	   timers and it take significant time to fire, quit the loop and
reschedule.
"timers and it take*s*"

> +	   This lets run loop to process user input (close the window for
example).

This lets the run loop process user input (close the window for example).

> +	   (WebCore::TimerBase::setNextFireTime):
> +	   Since timers are now fired one by one, there is no need to keep
track of updated timers.
> +	   * manual-tests/input-starved-by-timers.html: manual test that
attempts to freeze browser by
*M*anual test that...


> diff --git a/WebCore/manual-tests/input-starved-by-timers.html
b/WebCore/manual-tests/input-starved-by-timers.html
> +var targetLatency = 10000; // multiply timers until it takes this much to
fire all their callbacks.
*M*ultiply


> +    // Create more timers, capture the current time so when callbacks are
fired,
Create more timers. Capture the ...


> +function runTest() {
> +  log("Freezing UI...");
> +  setTimeout(function() { timerCallback(new Date().getTime()); }, 0);
> +  setTimeout("multiplyFactor = 0; log('Finishing. Started to drain
timers.');", 10000);

It would be nice to stick with the 4 space indent that you did above.

> diff --git a/WebCore/platform/ThreadTimers.cpp
b/WebCore/platform/ThreadTimers.cpp
>  namespace WebCore {
>  
> +// Fire timers until this time, then quit to let the run loop to process
user input events.

Consider:
  Fire timers for this length of time, and then quit to let the run loop
process user input events.

> +// 100ms is about perceptable delay in UI, have a half of ot as a threshold.


Consider:
  100ms is about a perceptible delay in UI, so use have a half of that as a
threshold.

> +// This is to prevent UI freeze when there are too much timers or machine
performance is low.

s/much/many/

> +    while (!m_timerHeap.isEmpty() && m_timerHeap.first()->m_nextFireTime <=
fireTime) {
...
> +	   // Catch the case where the timer asked timers to fire in a nested
event loop, or we are over time limit.
> +	   if (!m_firingTimers || timeToQuit < currentTime())

Consider making this part of the "while" condition (of course that means the
loop would never execute if timeToQuit > currentTime() at the beginning of the
loop. 

On further consideration, I think this should remain here because that way we
ensure that at least one timer got fired. (I had a hard time seeing how that
could happen but maybe there is a slow/loaded machine...)