Bug 64544 - Timer scheduling should be based off the monotonic clock
Summary: Timer scheduling should be based off the monotonic clock
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on: 64841
Blocks: 64825
  Show dependency treegraph
 
Reported: 2011-07-14 11:49 PDT by James Robinson
Modified: 2011-07-19 18:26 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2011-07-14 11:49:44 PDT
Timer scheduling should be based off the monotonic clock
Comment 1 James Robinson 2011-07-14 14:04:30 PDT
Created attachment 100860 [details]
Patch
Comment 2 James Robinson 2011-07-14 14:07:41 PDT
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 Darin Adler 2011-07-14 14:11:05 PDT
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?
Comment 4 James Robinson 2011-07-14 14:18:52 PDT
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 James Robinson 2011-07-14 14:19:53 PDT
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 Darin Adler 2011-07-14 14:20:28 PDT
(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 James Robinson 2011-07-14 14:24:11 PDT
(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 WebKit Review Bot 2011-07-14 14:40:01 PDT
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
Comment 9 WebKit Review Bot 2011-07-14 14:40:06 PDT
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
Comment 10 Darin Adler 2011-07-14 15:09:11 PDT
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 WebKit Review Bot 2011-07-14 15:11:24 PDT
Comment on attachment 100860 [details]
Patch

Attachment 100860 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9061380
Comment 12 James Robinson 2011-07-14 21:57:25 PDT
Created attachment 100929 [details]
Patch
Comment 13 James Robinson 2011-07-14 22:03:47 PDT
Created attachment 100930 [details]
Patch
Comment 14 James Robinson 2011-07-14 22:04:29 PDT
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 WebKit Review Bot 2011-07-14 22:30:12 PDT
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
Comment 16 WebKit Review Bot 2011-07-14 22:30:15 PDT
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
Comment 17 James Robinson 2011-07-15 17:59:12 PDT
Created attachment 101080 [details]
ready for review, chromium DEPS have rolled
Comment 18 Darin Adler 2011-07-15 18:10:36 PDT
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?
Comment 19 James Robinson 2011-07-15 18:27:30 PDT
(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.
Comment 20 James Robinson 2011-07-15 19:02:54 PDT
Created attachment 101084 [details]
Patch
Comment 21 WebKit Review Bot 2011-07-18 13:38:56 PDT
Comment on attachment 101084 [details]
Patch

Clearing flags on attachment: 101084

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