Bug 64544 - Timer scheduling should be based off the monotonic clock
: Timer scheduling should be based off the monotonic clock
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 64841
: 64825
  Show dependency treegraph
 
Reported: 2011-07-14 11:49 PST by
Modified: 2011-07-19 18:26 PST (History)


Attachments
Patch (12.28 KB, patch)
2011-07-14 14:04 PST, James Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (5.68 MB, application/zip)
2011-07-14 14:40 PST, WebKit Review Bot
no flags Details
Patch (28.73 KB, patch)
2011-07-14 21:57 PST, James Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Patch (27.75 KB, patch)
2011-07-14 22:03 PST, James Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (153.81 KB, application/zip)
2011-07-14 22:30 PST, WebKit Review Bot
no flags Details
ready for review, chromium DEPS have rolled (27.69 KB, patch)
2011-07-15 17:59 PST, James Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Patch (29.52 KB, patch)
2011-07-15 19:02 PST, James Robinson
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-07-14 11:49:44 PST
Timer scheduling should be based off the monotonic clock
------- Comment #1 From 2011-07-14 14:04:30 PST -------
Created an attachment (id=100860) [details]
Patch
------- Comment #2 From 2011-07-14 14:07:41 PST -------
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
------- Comment #3 From 2011-07-14 14:11:05 PST -------
(From update of attachment 100860 [details])
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?
------- Comment #4 From 2011-07-14 14:18:52 PST -------
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.
------- Comment #5 From 2011-07-14 14:19:53 PST -------
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.
------- Comment #6 From 2011-07-14 14:20:28 PST -------
(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 ...?
------- Comment #7 From 2011-07-14 14:24:11 PST -------
(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.
------- Comment #8 From 2011-07-14 14:40:01 PST -------
(From update of attachment 100860 [details])
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
------- Comment #9 From 2011-07-14 14:40:06 PST -------
Created an attachment (id=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
------- Comment #10 From 2011-07-14 15:09:11 PST -------
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)
------- Comment #11 From 2011-07-14 15:11:24 PST -------
(From update of attachment 100860 [details])
Attachment 100860 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9061380
------- Comment #12 From 2011-07-14 21:57:25 PST -------
Created an attachment (id=100929) [details]
Patch
------- Comment #13 From 2011-07-14 22:03:47 PST -------
Created an attachment (id=100930) [details]
Patch
------- Comment #14 From 2011-07-14 22:04:29 PST -------
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.
------- Comment #15 From 2011-07-14 22:30:12 PST -------
(From update of attachment 100930 [details])
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
------- Comment #16 From 2011-07-14 22:30:15 PST -------
Created an attachment (id=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
------- Comment #17 From 2011-07-15 17:59:12 PST -------
Created an attachment (id=101080) [details]
ready for review, chromium DEPS have rolled
------- Comment #18 From 2011-07-15 18:10:36 PST -------
(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?

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?
------- Comment #19 From 2011-07-15 18:27:30 PST -------
(In reply to comment #18)
> (From update of attachment 101080 [details] [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.
------- Comment #20 From 2011-07-15 19:02:54 PST -------
Created an attachment (id=101084) [details]
Patch
------- Comment #21 From 2011-07-18 13:38:56 PST -------
(From update of attachment 101084 [details])
Clearing flags on attachment: 101084

Committed r91206: <http://trac.webkit.org/changeset/91206>
------- Comment #22 From 2011-07-18 13:39:13 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #23 From 2011-07-19 09:13:15 PST -------
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.
------- Comment #24 From 2011-07-19 10:58:52 PST -------
Sorry, I didn't notice this bot going red. Will investigate.
------- Comment #25 From 2011-07-19 18:26:46 PST -------
(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.