[webkit-reviews] review granted: [Bug 196118] [Web Animations] JS wrapper may be deleted while animation is yet to dispatch its finish event : [Attachment 365643] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Antoine Quint <graouts at apple.com>'s
request for review:
Bug 196118: [Web Animations] JS wrapper may be deleted while animation is yet
to dispatch its finish event
https://bugs.webkit.org/show_bug.cgi?id=196118

Attachment 365643: Patch

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




--- Comment #3 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 365643
  --> https://bugs.webkit.org/attachment.cgi?id=365643
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)