Bug 196118

Summary: [Web Animations] JS wrapper may be deleted while animation is yet to dispatch its finish event
Product: WebKit Reporter: Antoine Quint <graouts>
Component: AnimationsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, dino, rniwa, sroberts, webkit-bot-watchers-bugzilla, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch cdumez: review+

Description Antoine Quint 2019-03-21 16:23:15 PDT
This simple code may not work if JS garbage collection occurs while the animation is in flight:

document.body.firstElementChild.animate({
    marginLeft: ["0", "100px"]
}, 5000).addEventListener("finish", event => console.log(event.type));
Comment 1 Antoine Quint 2019-03-21 16:24:18 PDT
<rdar://problem/46614137>
Comment 2 Antoine Quint 2019-03-21 16:29:33 PDT
Created attachment 365643 [details]
Patch
Comment 3 Ryosuke Niwa 2019-03-21 16:34:17 PDT
Comment on attachment 365643 [details]
Patch

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

> LayoutTests/webanimations/js-wrapper-kept-alive.html:13
> +    window.myAnimation = document.getElementById("target").animate({ marginLeft: ["0px", "100px"] }, 100);

Let's trigger gc() at the beginning of the test to start with a clean state.

Also, can't we just declare a local variable and call gc() outside runTest()?

> LayoutTests/webanimations/js-wrapper-kept-alive.html:18
> +    window.myAnimation.addEventListener("finish", event => {
> +        shouldBeTrue("event.target._isMyAnimation");
> +        finishJSTest();
> +    });

Instead of making this test timeout, can we use setTimeout to check whether this event listener had fired?
e.g.

window.sawEvent = false;
myAnimation.addEventListener("finish", (event) => { window.sawEvent = true; shouldBeTrue(event.target._isMyAnimation); });
setTimeout(() => shouldBeTrue('sawEvent'), 0)
Comment 4 Antoine Quint 2019-03-21 16:52:51 PDT
(In reply to Ryosuke Niwa from comment #3)
> Comment on attachment 365643 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365643&action=review
> 
> > LayoutTests/webanimations/js-wrapper-kept-alive.html:13
> > +    window.myAnimation = document.getElementById("target").animate({ marginLeft: ["0px", "100px"] }, 100);
> 
> Let's trigger gc() at the beginning of the test to start with a clean state. 

Sure.

> Also, can't we just declare a local variable and call gc() outside runTest()?

Sure.

> > LayoutTests/webanimations/js-wrapper-kept-alive.html:18
> > +    window.myAnimation.addEventListener("finish", event => {
> > +        shouldBeTrue("event.target._isMyAnimation");
> > +        finishJSTest();
> > +    });
> 
> Instead of making this test timeout, can we use setTimeout to check whether
> this event listener had fired?

We can use another longer-running animation as a form of timeout, which allows us not to use some arbitrary value since the animation could legitimately take a long time to start and complete depending on system load.
Comment 5 Antoine Quint 2019-03-21 16:54:04 PDT
Committed r243346: <https://trac.webkit.org/changeset/243346>
Comment 6 Shawn Roberts 2019-03-22 13:57:10 PDT
Since this revision. the following test is timing out consistently. 

legacy-animation-engine/animations/resume-after-page-cache.html

Flakiness Dashboard : 

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=animations%2Fresume-after-page-cache.html

Reproduced with :

run-webkit-tests legacy-animation-engine/animations/resume-after-page-cache.html --iterations 50 --exit-after-n-failures=5 --no-retry

Timed out 100% of the time locally with r243346, passes with r243345

Reproduced with iOS Simulator Release and Debug. Mac WK1 and WK2
Comment 7 Shawn Roberts 2019-03-27 13:32:17 PDT
rolled out in https://trac.webkit.org/changeset/243561/webkit

Was causing timeouts on bots consistently across 10 queues.
Comment 8 Shawn Roberts 2019-03-27 13:41:46 PDT
Reopening bug.
Comment 9 Antoine Quint 2019-04-04 06:35:39 PDT
Created attachment 366708 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2019-04-04 07:16:11 PDT
Comment on attachment 366708 [details]
Patch for landing

Clearing flags on attachment: 366708

Committed r243868: <https://trac.webkit.org/changeset/243868>
Comment 11 WebKit Commit Bot 2019-04-04 07:16:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Shawn Roberts 2019-04-04 15:32:44 PDT
Hi Antoine,

It appears the same timeout is occurring with animations/resume-after-page-cache.html after https://trac.webkit.org/changeset/243868/webkit

Dashboard:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=animations%2Fresume-after-page-cache.html

Reproduced with:

run-webkit-tests animations/resume-after-page-cache.html --iterations 25 -f --force

Test will pass 3 times and failed unexpectedly (test timed out, text diff). 

Test was skipped awhile ago for flaky failures, but the timeout happens after r243868, does not occur with previous revisions.
Comment 13 Shawn Roberts 2019-04-04 16:14:55 PDT
Reverted r243868 for reason:

Causing timeouts failures on several queues

Committed r243917: <https://trac.webkit.org/changeset/243917>
Comment 14 Antoine Quint 2019-04-08 07:08:27 PDT
Created attachment 366933 [details]
Patch
Comment 15 Chris Dumez 2019-04-08 08:47:47 PDT
Comment on attachment 366933 [details]
Patch

Yes, this seems fine.
Comment 16 Antoine Quint 2019-04-08 11:49:12 PDT
Committed r244031: <https://trac.webkit.org/changeset/244031>