Bug 186507 - [Web Animations] Make WPT test at timing-model/timelines/document-timelines.html pass reliably
Summary: [Web Animations] Make WPT test at timing-model/timelines/document-timelines.h...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-11 04:41 PDT by Antoine Quint
Modified: 2018-06-30 06:45 PDT (History)
7 users (show)

See Also:


Attachments
Patch (14.03 KB, patch)
2018-06-29 05:28 PDT, Antoine Quint
dino: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews204 for win-future (12.90 MB, application/zip)
2018-06-29 09:27 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2018-06-11 04:41:20 PDT
The WPT test at imported/w3c/web-platform-tests/web-animations/timing-model/timelines/document-timelines.html is failing reliably, flaky or timing out.
Comment 1 Radar WebKit Bug Importer 2018-06-11 04:41:32 PDT
<rdar://problem/41000257>
Comment 2 Antoine Quint 2018-06-29 05:28:30 PDT
Created attachment 343908 [details]
Patch
Comment 3 EWS Watchlist 2018-06-29 09:27:31 PDT
Comment on attachment 343908 [details]
Patch

Attachment 343908 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8383887

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html
Comment 4 EWS Watchlist 2018-06-29 09:27:43 PDT
Created attachment 343919 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 5 Dean Jackson 2018-06-29 11:49:42 PDT
Comment on attachment 343908 [details]
Patch

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

> Source/WebCore/animation/DocumentTimeline.cpp:159
> +        // animations, so we schedule the invalidation task and registere a whenIdle callback on the VM, which will

typo register

> Source/WebCore/animation/DocumentTimeline.cpp:164
> +        RefPtr<DocumentTimeline> self;
> +        m_waitingOnVMIdle = true;
> +        m_document->vm().whenIdle([self, this]() {

You never set or use self.

> Source/WebCore/animation/DocumentTimeline.cpp:215
> +    // We want to make sure we only clear the cached currenr time if we're not currently running

typo current

> Source/WebCore/animation/DocumentTimeline.cpp:219
> +    if (!m_waitingOnVMIdle && !m_invalidationTaskQueue.hasPendingTasks())

Are you sure you'll always have an empty invalidationTaskQueue when the whenIdle callback happens? Or I guess it could fire, but there still be a task to run, which will itself call maybeClearCachedCurrentTime.
Comment 6 Antoine Quint 2018-06-30 06:43:34 PDT
Committed r233394: <https://trac.webkit.org/changeset/233394>
Comment 7 Antoine Quint 2018-06-30 06:45:03 PDT
(In reply to Dean Jackson from comment #5)
> Comment on attachment 343908 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343908&action=review
> 
> > Source/WebCore/animation/DocumentTimeline.cpp:219
> > +    if (!m_waitingOnVMIdle && !m_invalidationTaskQueue.hasPendingTasks())
> 
> Are you sure you'll always have an empty invalidationTaskQueue when the
> whenIdle callback happens? Or I guess it could fire, but there still be a
> task to run, which will itself call maybeClearCachedCurrentTime.

That's right, both conditions need to be true, so we call maybeClearCachedCurrentTime() in both the whenIdle callback and performInvalidationTask().