<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[244031] trunk</title>
</head>
<body>

<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt;  }
#msg dl a { font-weight: bold}
#msg dl a:link    { color:#fc3; }
#msg dl a:active  { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff  {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/244031">244031</a></dd>
<dt>Author</dt> <dd>graouts@webkit.org</dd>
<dt>Date</dt> <dd>2019-04-08 11:49:04 -0700 (Mon, 08 Apr 2019)</dd>
</dl>

<h3>Log Message</h3>
<pre>[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
<rdar://problem/46614137>

Reviewed by Chris Dumez.

Source/WebCore:

Test: webanimations/js-wrapper-kept-alive.html

We need to teach WebAnimation to keep its JS wrapper alive if it's relevant or could become relevant again by virtue of having a timeline.
We also need to ensure that the new implementation of hasPendingActivity() does not interfere with the ability of pages to enter the page
cache when running animations.

* animation/WebAnimation.cpp:
(WebCore::WebAnimation::canSuspendForDocumentSuspension const):
(WebCore::WebAnimation::stop):
(WebCore::WebAnimation::hasPendingActivity const):
* animation/WebAnimation.h:

LayoutTests:

Add a test that starts a short animation, sets a custom property on it, registers a "finish" event listener on it and deletes
the sole reference to it in the JS world before triggering garbage collection. Prior to this fix, this test would time out
because the JS wrapper would be garbage-collected prior to the animation completing and thus the event listener would not
be called. To complete successfully, this test checks that it receives the event and its target is the same animation object
that was originally created by checking the custom property is still set.

We also make sure that a test, which was found to have regressed with a previous version of this patch, uses the animation
engine that it is expected to be testing.

* legacy-animation-engine/animations/resume-after-page-cache.html:
* webanimations/js-wrapper-kept-alive-expected.txt: Added.
* webanimations/js-wrapper-kept-alive.html: Added.</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsChangeLog">trunk/LayoutTests/ChangeLog</a></li>
<li><a href="#trunkLayoutTestslegacyanimationengineanimationsresumeafterpagecachehtml">trunk/LayoutTests/legacy-animation-engine/animations/resume-after-page-cache.html</a></li>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoreanimationWebAnimationcpp">trunk/Source/WebCore/animation/WebAnimation.cpp</a></li>
<li><a href="#trunkSourceWebCoreanimationWebAnimationh">trunk/Source/WebCore/animation/WebAnimation.h</a></li>
</ul>

<h3>Added Paths</h3>
<ul>
<li><a href="#trunkLayoutTestswebanimationsjswrapperkeptaliveexpectedtxt">trunk/LayoutTests/webanimations/js-wrapper-kept-alive-expected.txt</a></li>
<li><a href="#trunkLayoutTestswebanimationsjswrapperkeptalivehtml">trunk/LayoutTests/webanimations/js-wrapper-kept-alive.html</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkLayoutTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/ChangeLog (244030 => 244031)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/ChangeLog      2019-04-08 18:45:15 UTC (rev 244030)
+++ trunk/LayoutTests/ChangeLog 2019-04-08 18:49:04 UTC (rev 244031)
</span><span class="lines">@@ -1,3 +1,24 @@
</span><ins>+2019-04-08  Antoine Quint  <graouts@apple.com>
+
+        [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
+        <rdar://problem/46614137>
+
+        Reviewed by Chris Dumez.
+
+        Add a test that starts a short animation, sets a custom property on it, registers a "finish" event listener on it and deletes
+        the sole reference to it in the JS world before triggering garbage collection. Prior to this fix, this test would time out
+        because the JS wrapper would be garbage-collected prior to the animation completing and thus the event listener would not
+        be called. To complete successfully, this test checks that it receives the event and its target is the same animation object
+        that was originally created by checking the custom property is still set.
+
+        We also make sure that a test, which was found to have regressed with a previous version of this patch, uses the animation
+        engine that it is expected to be testing.
+
+        * legacy-animation-engine/animations/resume-after-page-cache.html:
+        * webanimations/js-wrapper-kept-alive-expected.txt: Added.
+        * webanimations/js-wrapper-kept-alive.html: Added.
+
</ins><span class="cx"> 2019-04-08  Eric Liang  <ericliang@apple.com>
</span><span class="cx"> 
</span><span class="cx">         AX: <svg> elements with labels and no accessible contents are exposed as empty AXGroups
</span></span></pre></div>
<a id="trunkLayoutTestslegacyanimationengineanimationsresumeafterpagecachehtml"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/legacy-animation-engine/animations/resume-after-page-cache.html (244030 => 244031)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/legacy-animation-engine/animations/resume-after-page-cache.html        2019-04-08 18:45:15 UTC (rev 244030)
+++ trunk/LayoutTests/legacy-animation-engine/animations/resume-after-page-cache.html   2019-04-08 18:49:04 UTC (rev 244031)
</span><span class="lines">@@ -1,3 +1,4 @@
</span><ins>+<!-- webkit-test-runner [ experimental:WebAnimationsCSSIntegrationEnabled=false ] -->
</ins><span class="cx"> <style>
</span><span class="cx"> @-webkit-keyframes bounce {
</span><span class="cx">     from {
</span></span></pre></div>
<a id="trunkLayoutTestswebanimationsjswrapperkeptaliveexpectedtxt"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/webanimations/js-wrapper-kept-alive-expected.txt (0 => 244031)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/webanimations/js-wrapper-kept-alive-expected.txt                               (rev 0)
+++ trunk/LayoutTests/webanimations/js-wrapper-kept-alive-expected.txt  2019-04-08 18:49:04 UTC (rev 244031)
</span><span class="lines">@@ -0,0 +1,10 @@
</span><ins>+This test checks that registering an event listener on an animation whose JS wrapper would otherwise be garbage-collected still fires registered event listeners.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS event.target._isMyAnimation is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
</ins></span></pre></div>
<a id="trunkLayoutTestswebanimationsjswrapperkeptalivehtml"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/webanimations/js-wrapper-kept-alive.html (0 => 244031)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/webanimations/js-wrapper-kept-alive.html                               (rev 0)
+++ trunk/LayoutTests/webanimations/js-wrapper-kept-alive.html  2019-04-08 18:49:04 UTC (rev 244031)
</span><span class="lines">@@ -0,0 +1,33 @@
</span><ins>+<!DOCTYPE html>
+<html>
+<body>
+<div id="target"></div>
+<script src="../resources/js-test-pre.js"></script>
+<script>
+description("This test checks that registering an event listener on an animation whose JS wrapper would otherwise be garbage-collected still fires registered event listeners.");
+
+if (window.internals)
+    jsTestIsAsync = true;
+
+// A longer animation that could not be garbage-collected under any circumstance allows us to finish the test
+// with a reasonable delay without hard-coding a timeout.
+const timeoutAnimation = document.getElementById("target").animate({ marginRight: ["0px", "100px"] }, 1000);
+timeoutAnimation.addEventListener("finish", finishJSTest);
+
+function runTest() {
+    const animation = document.getElementById("target").animate({ marginLeft: ["0px", "100px"] }, 100);
+    animation._isMyAnimation = true;
+    animation.addEventListener("finish", event => {
+        shouldBeTrue("event.target._isMyAnimation");
+        finishJSTest();
+    });
+}
+
+gc();
+runTest();
+gc();
+
+</script>
+<script src="../resources/js-test-post.js"></script>
+</body>
+</html>
</ins><span class="cx">\ No newline at end of file
</span></span></pre></div>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (244030 => 244031)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/ChangeLog   2019-04-08 18:45:15 UTC (rev 244030)
+++ trunk/Source/WebCore/ChangeLog      2019-04-08 18:49:04 UTC (rev 244031)
</span><span class="lines">@@ -1,3 +1,23 @@
</span><ins>+2019-04-08  Antoine Quint  <graouts@apple.com>
+
+        [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
+        <rdar://problem/46614137>
+
+        Reviewed by Chris Dumez.
+
+        Test: webanimations/js-wrapper-kept-alive.html
+
+        We need to teach WebAnimation to keep its JS wrapper alive if it's relevant or could become relevant again by virtue of having a timeline.
+        We also need to ensure that the new implementation of hasPendingActivity() does not interfere with the ability of pages to enter the page
+        cache when running animations.
+
+        * animation/WebAnimation.cpp:
+        (WebCore::WebAnimation::canSuspendForDocumentSuspension const):
+        (WebCore::WebAnimation::stop):
+        (WebCore::WebAnimation::hasPendingActivity const):
+        * animation/WebAnimation.h:
+
</ins><span class="cx"> 2019-04-08  Eric Liang  <ericliang@apple.com>
</span><span class="cx"> 
</span><span class="cx">         AX: <svg> elements with labels and no accessible contents are exposed as empty AXGroups
</span></span></pre></div>
<a id="trunkSourceWebCoreanimationWebAnimationcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/animation/WebAnimation.cpp (244030 => 244031)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/animation/WebAnimation.cpp  2019-04-08 18:45:15 UTC (rev 244030)
+++ trunk/Source/WebCore/animation/WebAnimation.cpp     2019-04-08 18:49:04 UTC (rev 244031)
</span><span class="lines">@@ -1157,15 +1157,25 @@
</span><span class="cx"> 
</span><span class="cx"> bool WebAnimation::canSuspendForDocumentSuspension() const
</span><span class="cx"> {
</span><del>-    return !hasPendingActivity();
</del><ins>+    // Use the base class's implementation of hasPendingActivity() since we wouldn't want the custom implementation
+    // in this class designed to keep JS wrappers alive to interfere with the ability for a page using animations
+    // to enter the page cache.
+    return !ActiveDOMObject::hasPendingActivity();
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> void WebAnimation::stop()
</span><span class="cx"> {
</span><ins>+    ActiveDOMObject::stop();
</ins><span class="cx">     m_isStopped = true;
</span><span class="cx">     removeAllEventListeners();
</span><span class="cx"> }
</span><span class="cx"> 
</span><ins>+bool WebAnimation::hasPendingActivity() const
+{
+    // Keep the JS wrapper alive if the animation is considered relevant or could become relevant again by virtue of having a timeline.
+    return m_timeline || m_isRelevant || ActiveDOMObject::hasPendingActivity();
+}
+
</ins><span class="cx"> void WebAnimation::updateRelevance()
</span><span class="cx"> {
</span><span class="cx">     m_isRelevant = computeRelevance();
</span></span></pre></div>
<a id="trunkSourceWebCoreanimationWebAnimationh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/animation/WebAnimation.h (244030 => 244031)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/animation/WebAnimation.h    2019-04-08 18:45:15 UTC (rev 244030)
+++ trunk/Source/WebCore/animation/WebAnimation.h       2019-04-08 18:49:04 UTC (rev 244031)
</span><span class="lines">@@ -118,6 +118,8 @@
</span><span class="cx">     bool isSuspended() const { return m_isSuspended; }
</span><span class="cx">     virtual void remove();
</span><span class="cx"> 
</span><ins>+    bool hasPendingActivity() const final;
+
</ins><span class="cx">     using RefCounted::ref;
</span><span class="cx">     using RefCounted::deref;
</span><span class="cx"> 
</span></span></pre>
</div>
</div>

</body>
</html>