Changeset 244031 in webkit

Timestamp:

Apr 8, 2019, 11:49:04 AM (7 years ago)

Author:

graouts@webkit.org

Message:

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

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/legacy-animation-engine/animations/resume-after-page-cache.html (modified) (1 diff)
• LayoutTests/webanimations/js-wrapper-kept-alive-expected.txt (added)
• LayoutTests/webanimations/js-wrapper-kept-alive.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/animation/WebAnimation.cpp (modified) (1 diff)
• Source/WebCore/animation/WebAnimation.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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.
+
 2019-04-08  Eric Liang  <ericliang@apple.com>
 

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

@@ +1 @@
+<!-- webkit-test-runner [ experimental:WebAnimationsCSSIntegrationEnabled=false ] -->
 <style>
 @-webkit-keyframes bounce {

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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:
+
 2019-04-08  Eric Liang  <ericliang@apple.com>
 

trunk/Source/WebCore/animation/WebAnimation.cpp

@@ -1158 +1158 @@
 bool WebAnimation::canSuspendForDocumentSuspension() const
 {
-    return !hasPendingActivity();
+    // 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();
 }
 
 void WebAnimation::stop()
 {
+    ActiveDOMObject::stop();
     m_isStopped = true;
     removeAllEventListeners();
+}
+
+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();
 }
 

trunk/Source/WebCore/animation/WebAnimation.h

@@ -119 +119 @@
     virtual void remove();
 
+    bool hasPendingActivity() const final;
+
     using RefCounted::ref;
     using RefCounted::deref;