Summary: | [Web Animations] JS wrapper may be deleted while animation is yet to dispatch its finish event | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||||
Component: | Animations | Assignee: | 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
Antoine Quint
2019-03-21 16:23:15 PDT
Created attachment 365643 [details]
Patch
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) (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. Committed r243346: <https://trac.webkit.org/changeset/243346> 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 rolled out in https://trac.webkit.org/changeset/243561/webkit Was causing timeouts on bots consistently across 10 queues. Reopening bug. Created attachment 366708 [details]
Patch for landing
Comment on attachment 366708 [details] Patch for landing Clearing flags on attachment: 366708 Committed r243868: <https://trac.webkit.org/changeset/243868> All reviewed patches have been landed. Closing bug. 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. Reverted r243868 for reason: Causing timeouts failures on several queues Committed r243917: <https://trac.webkit.org/changeset/243917> Created attachment 366933 [details]
Patch
Comment on attachment 366933 [details]
Patch
Yes, this seems fine.
Committed r244031: <https://trac.webkit.org/changeset/244031> |