REGRESSION(294049@main): document.timeline.currentTime never advances
#47359
+57
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
https://bugs.webkit.org/show_bug.cgi?id=295177 rdar://problem/154607696 Reviewed by NOBODY (OOPS!). Prior to 294049@main, when the current time would be requested from `AnimationTimelinesController`, most likely through `DocumentTimeline::currentTime()`, we would either use the cached current time, if available, or compute a new one. In the case where we would compute a new one, we would also enqueue a task (using `EventLoop::queueTask()`) to clear it after the current run loop had completed, ensuring that any code ran within that loop would use the same current time. That changed in 294049@main where we made it so that the document timeline's current time was cached throughout the duration of the animation frame, matching the behavior of Chrome and Firefox. In effect, this meant that that current time was updated every 16ms or so (assuming a display update cadence of 60Hz). However, we neglected to preserve the mechanism to clear that cached current time, instead relying on the fact that animations would cause animations to be updated and for that current time to be cached as the page rendering updated at 60Hz. When we cache the current time, we now clear it after a delay matching the page's preferred rendering update interval. We also add a test which checks that the document timeline's current time is correctly updated after 20ms even with no page rendering updates scheduled. This test would have reliably failed prior to this patch. * LayoutTests/webanimations/document-timeline-current-time-updates-without-running-animations-expected.txt: Added. * LayoutTests/webanimations/document-timeline-current-time-updates-without-running-animations.html: Added. * Source/WebCore/animation/AnimationTimelinesController.cpp: (WebCore::AnimationTimelinesController::AnimationTimelinesController): (WebCore::AnimationTimelinesController::suspendAnimations): (WebCore::AnimationTimelinesController::resumeAnimations): (WebCore::AnimationTimelinesController::cacheCurrentTime): (WebCore::AnimationTimelinesController::clearCachedCurrentTime): * Source/WebCore/animation/AnimationTimelinesController.h:
|
EWS run on current version of this PR (hash 135c739) |
smfr
reviewed
Jun 30, 2025
|
|
||
| // Now wait longer than it should take for a page rendering update to last | ||
| // and check that the current time has changed to a larger value. | ||
| await timeout(20); |
There was a problem hiding this comment.
I think might be flakey on loaded machines. Do you have to wait but explicitly not run Page::updateRendering? There's normally a flurry of those around page load that are hard to avoid, so if you really want to wait for idle, you may need to wait for 4-5 updateRenderings before starting the test.
There was a problem hiding this comment.
This test reliably fails for me prior to this patch. I also expect setTimeout(…, 20) to always run after a WebCore Timer set for 16_ms if both are generally started in the same run loop.
There was a problem hiding this comment.
Hmm, running this test with -f -1 produces some failures across 5000 runs. It's not happening with just -f with WK2.
| // In order to not have a stale cached current time, we schedule a timer to reset it | ||
| // in the time it would take an animation frame to run under normal circumstances. | ||
| ASSERT(m_document->page()); | ||
| m_cachedCurrentTimeClearanceTimer.startOneShot(RefPtr { m_document->page() }->preferredRenderingUpdateInterval()); |
There was a problem hiding this comment.
Would it be better to explicitly clear the cached time in Page::doAfterUpdateRendering()? This seems like it could be flaky.
There was a problem hiding this comment.
No, the cached current time must remain cached until the next page rendering update.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
135c739
135c739