Timer scheduling should be based off the monotonic clock
Created attachment 100860 [details] Patch
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 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?
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.
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.
(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 ...?
(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 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
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
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 on attachment 100860 [details] Patch Attachment 100860 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9061380
Created attachment 100929 [details] Patch
Created attachment 100930 [details] Patch
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 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
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
Created attachment 101080 [details] ready for review, chromium DEPS have rolled
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?
(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.
Created attachment 101084 [details] Patch
Comment on attachment 101084 [details] Patch Clearing flags on attachment: 101084 Committed r91206: <http://trac.webkit.org/changeset/91206>
All reviewed patches have been landed. Closing bug.
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.
Sorry, I didn't notice this bot going red. Will investigate.
(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.