Bug 64544 – Timer scheduling should be based off the monotonic clock

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

Timer scheduling should be based off the monotonic clock

[James Robinson]

Created attachment 100860 [details]
Patch

[James Robinson]

Here's a test page to see the difference in behavior:

http://webstuff.nfshost.com/timers.html

This page logs the current time (according to Date.now()).  Try loading that page, clicking the 'set 3 second timer' button, and then moving the system clock back by 1 minute.  In Safari without this patch, the timer fires 63 seconds after the button is clicked and the timestamp delay according to Date.now() is ~3000ms.  In all other browsers (I tested Firefox and Opera on OS X and IE9 on Win7), the timer fires 3 seconds after the button is clicked and the timestamp delay according to Date.now() is ~63000ms.  The latter behavior is what HTML specifies:

http://www.whatwg.org/specs/web-apps/current-work/multipage/timers.html#timers

[Darin Adler]

Comment on attachment 100860 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100860&action=review

> Source/WebCore/platform/SharedTimer.h:44
> +        // The fire time is based off of WTF::monotonicallyIncreasingTime() and is not comparable to currentTime().

I think we can omit the WTF:: from this comment.

> Source/WebKit/chromium/public/WebKitClient.h:51
> +// FIXME: remove after rolling deps
> +#define WEBKIT_USE_MONOTONIC_CLOCK_FOR_TIMER_SCHEDULING

I’m surprised this is needed. SharedTimer doesn’t expose what its timebase is to code outside the class, so how can it matter?

If it does matter, why are we sure that there are not similar issues for WebKit clients on other platforms? What is the Chromium-specific issue that does not exist elsewhere?

[James Robinson]

The chromium-specific bit is that in chrome setSharedTimerFireTime() is implemented in a file outside the WebKit codebase (it's http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/webkit/glue/webkitclient_impl.cc&exact_package=chromium&q=setSharedTimerFireTime&type=cs&l=501), whereas for other ports it's in a SharedTimer*.cpp in WebCore/platform.  In order to calculate the interval until the next fire, the passed in fireTime has to be compared with the time base (either currentTime() or monotonicallyIncreasingTime()).

It's the exact same code change as for the other ports.

Thanks for the review - I'll fix the comment and then land.

[James Robinson]

Note that this would be a bit easier if setSharedTimerFireTime() specified a delay instead of a scheduled fire time, since the first thing every port does is extract the interval from the scheduled fire time.

[Darin Adler]

(In reply to comment #5)
> Note that this would be a bit easier if setSharedTimerFireTime() specified a delay instead of a scheduled fire time, since the first thing every port does is extract the interval from the scheduled fire time.

And you didn’t change that even though you had to change every implementation, because ...?

[James Robinson]

(In reply to comment #6)
> (In reply to comment #5)
> > Note that this would be a bit easier if setSharedTimerFireTime() specified a delay instead of a scheduled fire time, since the first thing every port does is extract the interval from the scheduled fire time.
> 
> And you didn’t change that even though you had to change every implementation, because ...?

Good point, I'll go ahead and do that.

[WebKit Review Bot]

Comment on attachment 100860 [details]
Patch

Attachment 100860 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9061371

New failing tests:
http/tests/cache/subresource-expiration-1.html
http/tests/history/redirect-js-location-2-seconds.html
animations/play-state-paused.html
http/tests/cache/subresource-expiration-2.html
animations/play-state.html
http/tests/history/redirect-200-refresh-2-seconds.pl
http/tests/history/redirect-js-document-location-2-seconds.html
animations/change-keyframes-name.html
animations/transition-and-animation-3.html
animations/suspend-resume-animation.html
http/tests/history/redirect-js-form-submit-2-seconds.html
animations/animation-start-event-destroy-renderer.html
animations/animation-css-rule-types.html
http/tests/cookies/multiple-cookies.html
http/tests/history/redirect-js-location-href-2-seconds.html
animations/suspend-resume-animation-events.html
animations/change-one-anim.html
animations/animation-direction-normal.html
animations/dynamic-stylesheet-loading.html
http/tests/history/redirect-js-location-assign-2-seconds.html

[WebKit Review Bot]

Created attachment 100865 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Darin Adler]

Windows has an export file too, so this needs to be added there:

6>WebCore.lib(Timer.obj) : error LNK2019: unresolved external symbol "double __cdecl WTF::monotonicallyIncreasingTime(void)" (?monotonicallyIncreasingTime@WTF@@YANXZ) referenced in function "public: void __thiscall WebCore::TimerBase::start(double,double)" (?start@TimerBase@WebCore@@QAEXNN@Z)
6>WebCore.lib(ThreadTimers.obj) : error LNK2001: unresolved external symbol "double __cdecl WTF::monotonicallyIncreasingTime(void)" (?monotonicallyIncreasingTime@WTF@@YANXZ)
6>WebCore.lib(SharedTimerWin.obj) : error LNK2001: unresolved external symbol "double __cdecl WTF::monotonicallyIncreasingTime(void)" (?monotonicallyIncreasingTime@WTF@@YANXZ)

[WebKit Review Bot]

Comment on attachment 100860 [details]
Patch

Attachment 100860 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9061380

[James Robinson]

Created attachment 100929 [details]
Patch

[James Robinson]

Created attachment 100930 [details]
Patch

[James Robinson]

Updated patch will not work quite yet on chromium, since it depends on an update there, so not setting review? yet.

Changes from last patch:
- renamed setSharedTimerFireTime() -> setSharedTimerFireInterval() and changed the parameter from an absolute timestamp to an interval, which simplifies every port's implementation of this function.
- updated WorkerRunLoop's WorkerSharedTimer to reflect new interface
- since WorkerRunLoop uses the same underlying machinery as WebKit2's RunLoop, updated WebKit2's RunLoop and CoreIPC's Connection class to use monotonicallyIncreasingTime() as well.  This is closer to what we do in chromium and provides significantly better behavior when the system clock is adjusted.
- fixed exports for windows.

Expect EWS to fail for chromium.  I'll update the bug when the chromium dependencies have rolled in.

[WebKit Review Bot]

Comment on attachment 100930 [details]
Patch

Attachment 100930 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9096105

New failing tests:
http/tests/appcache/different-origin-manifest.html
animations/animation-drt-api.html
http/tests/appcache/crash-when-navigating-away-then-back.html
http/tests/appcache/fail-on-update-2.html
http/tests/appcache/document-write-html-element.html
canvas/philip/tests/2d.pattern.modify.image1.html
canvas/philip/tests/2d.pattern.animated.gif.html
animations/animation-end-event-destroy-renderer.html
editing/deleting/25322-1.html
http/tests/appcache/destroyed-frame.html
http/tests/appcache/cyrillic-uri.html
canvas/philip/tests/2d.pattern.modify.image2.html
animations/animation-direction.html
animations/animation-drt-api-multiple-keyframes.html
http/tests/appcache/deferred-events-delete-while-raising.html
canvas/philip/tests/2d.drawImage.animated.apng.html
animations/animation-controller-drt-api.html
canvas/philip/tests/2d.drawImage.animated.gif.html
animations/animation-direction-normal.html
animations/animation-add-events-in-handler.html

[WebKit Review Bot]

Created attachment 100932 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[James Robinson]

Created attachment 101080 [details]
ready for review, chromium DEPS have rolled

[Darin Adler]

Comment on attachment 101080 [details]
ready for review, chromium DEPS have rolled

View in context: https://bugs.webkit.org/attachment.cgi?id=101080&action=review

> Source/WebCore/platform/ThreadTimers.cpp:84
> -        m_sharedTimer->setFireTime(m_timerHeap.first()->m_nextFireTime);
> +        m_sharedTimer->setFireInterval(m_timerHeap.first()->m_nextFireTime - monotonicallyIncreasingTime());

Will setFireInterval correctly handle a negative number? Do we need to add code to handle that case?

It seems to me that half of the functions check against zero and another half don't. Could we hoist that check for negative numbers into this function?

Seems lame that monotonicallyIncreasingTime is subtracted here and then added back inside setFireInterval.

> Source/WebCore/platform/wince/SharedTimerWinCE.cpp:95
> +    double intervalMillis = intervalSeconds * 1000.;

Why not use the real name, milliseconds, or the standard abbreviation, ms, rather than the coined term millis?

[James Robinson]

(In reply to comment #18)
> (From update of attachment 101080 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101080&action=review
> 
> > Source/WebCore/platform/ThreadTimers.cpp:84
> > -        m_sharedTimer->setFireTime(m_timerHeap.first()->m_nextFireTime);
> > +        m_sharedTimer->setFireInterval(m_timerHeap.first()->m_nextFireTime - monotonicallyIncreasingTime());
> 
> Will setFireInterval correctly handle a negative number? Do we need to add code to handle that case?
> 
> It seems to me that half of the functions check against zero and another half don't. Could we hoist that check for negative numbers into this function?

I'll hoist the <0 check up to here and remove it from the various ports.

> 
> Seems lame that monotonicallyIncreasingTime is subtracted here and then added back inside setFireInterval.

It's added back in WorkerRunLoop's implementation of setFireInterval(), but not in the MainThreadSharedTimer implementation (which is the more common codepath by a wide margin).

> 
> > Source/WebCore/platform/wince/SharedTimerWinCE.cpp:95
> > +    double intervalMillis = intervalSeconds * 1000.;
> 
> Why not use the real name, milliseconds, or the standard abbreviation, ms, rather than the coined term millis?

Seems that the existing code here uses MS, so I'll use that as well.

Thanks again for the prompt review.  I'll upload another patch for the various EWS bots, just in case I fat finger something in one of the many platform-specific files.

[James Robinson]

Created attachment 101084 [details]
Patch

[WebKit Review Bot]

Comment on attachment 101084 [details]
Patch

Clearing flags on attachment: 101084

Committed r91206: <http://trac.webkit.org/changeset/91206>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jessie Berlin]

It looks like this change broke around 50 tests related to plug-ins on WebKit2:
http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20(WebKit2%20Tests)/builds/13512
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(WebKit2%20Tests)/r91206%20(13513)/results.html

If this isn't fixed soon, we should probably roll out the patch.

[James Robinson]

Sorry, I didn't notice this bot going red. Will investigate.

[James Robinson]

(In reply to comment #23)
> It looks like this change broke around 50 tests related to plug-ins on WebKit2:
> http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20(WebKit2%20Tests)/builds/13512
> http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(WebKit2%20Tests)/r91206%20(13513)/results.html
> 
> If this isn't fixed soon, we should probably roll out the patch.

Patch up here: https://bugs.webkit.org/show_bug.cgi?id=64841 to restore the previous behavior for WK2 runloops (and worker runloops, which use the same underlying mechanism).  This fixes the WebKit2 plugin tests, although it doesn't fix the bug referenced in https://bugs.webkit.org/show_bug.cgi?id=64825.