Bug 196118 – [Web Animations] JS wrapper may be deleted while animation is yet to dispatch its finish event

• Component: Animations
• Status: RESOLVED FIXED

[Antoine Quint]

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));

[Antoine Quint]

<rdar://problem/46614137>

[Antoine Quint]

Created attachment 365643 [details]
Patch

[Ryosuke Niwa]

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)

[Antoine Quint]

(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.

[Antoine Quint]

Committed r243346: <https://trac.webkit.org/changeset/243346>

[Shawn Roberts]

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

[Shawn Roberts]

rolled out in https://trac.webkit.org/changeset/243561/webkit

Was causing timeouts on bots consistently across 10 queues.

[Shawn Roberts]

Reopening bug.

[Antoine Quint]

Created attachment 366708 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 366708 [details]
Patch for landing

Clearing flags on attachment: 366708

Committed r243868: <https://trac.webkit.org/changeset/243868>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Shawn Roberts]

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.

[Shawn Roberts]

Reverted r243868 for reason:

Causing timeouts failures on several queues

Committed r243917: <https://trac.webkit.org/changeset/243917>

[Antoine Quint]

Created attachment 366933 [details]
Patch

[Chris Dumez]

Comment on attachment 366933 [details]
Patch

Yes, this seems fine.

[Antoine Quint]

Committed r244031: <https://trac.webkit.org/changeset/244031>