Title: [231766] trunk
Revision
231766
Author
grao...@webkit.org
Date
2018-05-14 11:19:30 -0700 (Mon, 14 May 2018)

Log Message

[Web Animations] Tests using the new animation engine may crash under WebCore::FrameView::didDestroyRenderTree when using internals methods
https://bugs.webkit.org/show_bug.cgi?id=185612
<rdar://problem/39579344>

Reviewed by Dean Jackson.

Source/WebCore:

Add a new internals.pseudoElement() method to obtain a pseudo element matching a given pseudo-id. This is necessary to be able to move off
internals.pauseTransitionAtTimeOnPseudoElement() and internals.pauseAnimationAtTimeOnPseudoElement() for Web Animations testing.

* testing/Internals.cpp:
(WebCore::Internals::pseudoElement):
* testing/Internals.h:
* testing/Internals.idl:

LayoutTests:

Some tests that were opting into the new animation engine were using internals methods (pauseAnimationAtTimeOnElement, pauseTransitionAtTimeOnElement, etc.)
that enforce the creation of animations in the old animation engine. Meanwhile, the code that toggles the animation engine used based on HTML comments is run
prior to teardown of the previous test and so a test running with the new engine would run with the legacy engine during teardown. These two factors would
cause `ASSERT(!frame().animation().hasAnimations())` to fail under FrameView::didDestroyRenderTree().

We update tests that use these internals method to use the Web Animations API instead and opt into the new animation engine if they didn't already do that.

* animations/animation-hit-test-transform.html:
* animations/keyframes-dynamic-expected.txt:
* animations/keyframes-dynamic.html:
* animations/missing-from-to-expected.txt:
* animations/missing-from-to-transforms-expected.txt:
* animations/missing-from-to-transforms.html:
* animations/missing-from-to.html:
* fast/css-generated-content/pseudo-animation.html:
* transitions/transition-hit-test-transform.html:

Modified Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (231765 => 231766)


--- trunk/LayoutTests/ChangeLog	2018-05-14 18:15:57 UTC (rev 231765)
+++ trunk/LayoutTests/ChangeLog	2018-05-14 18:19:30 UTC (rev 231766)
@@ -1,5 +1,30 @@
 2018-05-14  Antoine Quint  <grao...@apple.com>
 
+        [Web Animations] Tests using the new animation engine may crash under WebCore::FrameView::didDestroyRenderTree when using internals methods
+        https://bugs.webkit.org/show_bug.cgi?id=185612
+        <rdar://problem/39579344>
+
+        Reviewed by Dean Jackson.
+
+        Some tests that were opting into the new animation engine were using internals methods (pauseAnimationAtTimeOnElement, pauseTransitionAtTimeOnElement, etc.)
+        that enforce the creation of animations in the old animation engine. Meanwhile, the code that toggles the animation engine used based on HTML comments is run
+        prior to teardown of the previous test and so a test running with the new engine would run with the legacy engine during teardown. These two factors would
+        cause `ASSERT(!frame().animation().hasAnimations())` to fail under FrameView::didDestroyRenderTree().
+
+        We update tests that use these internals method to use the Web Animations API instead and opt into the new animation engine if they didn't already do that.
+
+        * animations/animation-hit-test-transform.html:
+        * animations/keyframes-dynamic-expected.txt:
+        * animations/keyframes-dynamic.html:
+        * animations/missing-from-to-expected.txt:
+        * animations/missing-from-to-transforms-expected.txt:
+        * animations/missing-from-to-transforms.html:
+        * animations/missing-from-to.html:
+        * fast/css-generated-content/pseudo-animation.html:
+        * transitions/transition-hit-test-transform.html:
+
+2018-05-14  Antoine Quint  <grao...@apple.com>
+
         REGRESSION (r230574): Interrupted hardware transitions don't behave correctly
         https://bugs.webkit.org/show_bug.cgi?id=185299
         <rdar://problem/39630230>

Modified: trunk/LayoutTests/animations/animation-hit-test-transform.html (231765 => 231766)


--- trunk/LayoutTests/animations/animation-hit-test-transform.html	2018-05-14 18:15:57 UTC (rev 231765)
+++ trunk/LayoutTests/animations/animation-hit-test-transform.html	2018-05-14 18:19:30 UTC (rev 231766)
@@ -1,5 +1,4 @@
-<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
-   "http://www.w3.org/TR/html4/loose.dtd">
+<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd"><!-- webkit-test-runner [ enableCSSAnimationsAndCSSTransitionsBackedByWebAnimations=true ] -->
 
 <html lang="en">
 <head>
@@ -56,20 +55,15 @@
      
         function doTest()
         {
-            if (window.testRunner) {
-                var target = document.getElementById("target");
-                if (!internals.pauseAnimationAtTimeOnElement("anim", 2.0, target)) {
-                    document.getElementById('result').innerHTML = "FAIL: Failed to pause animation"
-                    testRunner.notifyDone();
-                    return;
-                }
-        
-                checkResults();
+            var target = document.getElementById("target");
+            if (!pauseAnimationAtTimeOnElement("anim", 2.0, target)) {
+                document.getElementById('result').innerHTML = "FAIL: Failed to pause animation"
                 testRunner.notifyDone();
+                return;
             }
-            else {
-                window.setTimeout("checkResults()", 2000);
-            }
+    
+            checkResults();
+            testRunner.notifyDone();
         }
     
         function startTest()

Modified: trunk/LayoutTests/animations/keyframes-dynamic-expected.txt (231765 => 231766)


--- trunk/LayoutTests/animations/keyframes-dynamic-expected.txt	2018-05-14 18:15:57 UTC (rev 231765)
+++ trunk/LayoutTests/animations/keyframes-dynamic-expected.txt	2018-05-14 18:19:30 UTC (rev 231766)
@@ -1,7 +1,8 @@
 This test performs an animation of the left property. It should stop for 100ms at 100px and 200px We test for those values 50ms after it has stopped at each position. The animations for the three boxes are inserted by _javascript_. The first box's keyframes remain in the stylesheet. The second box's keyframes are removed after its animation starts (but it should animate). The third box's keyframes are removed before its animation starts, and it should not animate.
-PASS - box3 animation was not running
 PASS - "left" property for "box1" element at 0.3s saw something close to: 100
 PASS - "left" property for "box1" element at 0.7s saw something close to: 200
 PASS - "left" property for "box2" element at 0.3s saw something close to: 100
 PASS - "left" property for "box2" element at 0.7s saw something close to: 200
+PASS - "left" property for "box3" element at 0.3s saw something close to: 0
+PASS - "left" property for "box3" element at 0.7s saw something close to: 0
 

Modified: trunk/LayoutTests/animations/keyframes-dynamic.html (231765 => 231766)


--- trunk/LayoutTests/animations/keyframes-dynamic.html	2018-05-14 18:15:57 UTC (rev 231765)
+++ trunk/LayoutTests/animations/keyframes-dynamic.html	2018-05-14 18:19:30 UTC (rev 231766)
@@ -1,5 +1,4 @@
-<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
-   "http://www.w3.org/TR/html4/loose.dtd">
+<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd"><!-- webkit-test-runner [ enableCSSAnimationsAndCSSTransitionsBackedByWebAnimations=true ] -->
 
 <html lang="en">
 <head>
@@ -28,6 +27,8 @@
       ["anim1", 0.7, "box1", "left", 200, 1],
       ["anim2", 0.3, "box2", "left", 100, 1],
       ["anim2", 0.7, "box2", "left", 200, 1],
+      ["anim3", 0.3, "box3", "left", 0, 0],
+      ["anim3", 0.7, "box3", "left", 0, 0],
     ];
 
     function addKeyframes() {
@@ -64,13 +65,6 @@
         style.sheet.removeRule(box3Index);
 
         runAnimationTest(expectedValues);
-
-        if (window.testRunner) {
-            if (internals.pauseAnimationAtTimeOnElement("anim3", 0.1, box3))
-                result += "FAIL - box3 animation was running<br>";
-            else
-                result += "PASS - box3 animation was not running<br>";
-        }
     }
     
     window.addEventListener('DOMContentLoaded', addKeyframes, false);

Modified: trunk/LayoutTests/animations/missing-from-to-expected.txt (231765 => 231766)


--- trunk/LayoutTests/animations/missing-from-to-expected.txt	2018-05-14 18:15:57 UTC (rev 231765)
+++ trunk/LayoutTests/animations/missing-from-to-expected.txt	2018-05-14 18:19:30 UTC (rev 231766)
@@ -1,5 +1,4 @@
 This test performs animations of the left property on five boxes over 2 seconds. Box 1 has all keyframes. Box 2 has a missing "from" keyframe. Box 3 has a missing "to" keyframe. Box 4 has both "from" and "to" keyframes missing, but other keyframes which should trigger the generation of "from" and "to". Box 5 has no keyframes, and should not animate. The test takes 3 snapshots each and expects each result to be within a specified range.
-PASS - box5 animation was not running
 PASS - "left" property for "box1" element at 0.4s saw something close to: 20
 PASS - "left" property for "box1" element at 1s saw something close to: 20
 PASS - "left" property for "box1" element at 1.6s saw something close to: 15
@@ -12,4 +11,7 @@
 PASS - "left" property for "box4" element at 0.4s saw something close to: 20
 PASS - "left" property for "box4" element at 1s saw something close to: 25
 PASS - "left" property for "box4" element at 1.6s saw something close to: 15
+PASS - "left" property for "box5" element at 0.4s saw something close to: 10
+PASS - "left" property for "box5" element at 1s saw something close to: 10
+PASS - "left" property for "box5" element at 1.6s saw something close to: 10
 

Modified: trunk/LayoutTests/animations/missing-from-to-transforms-expected.txt (231765 => 231766)


--- trunk/LayoutTests/animations/missing-from-to-transforms-expected.txt	2018-05-14 18:15:57 UTC (rev 231765)
+++ trunk/LayoutTests/animations/missing-from-to-transforms-expected.txt	2018-05-14 18:19:30 UTC (rev 231766)
@@ -1,5 +1,4 @@
 This test performs animations of the transform property on five boxes over 2 seconds. Box 1 has all keyframes. Box 2 has a missing "from" keyframe. Box 3 has a missing "to" keyframe. Box 4 has both "from" and "to" keyframes missing, but other keyframes which should trigger the generation of "from" and "to". Box 5 has no keyframes, and should not animate. The test takes 3 snapshots each and expects each result to be within a specified range.
-PASS - box5 animation was not running
 PASS - "webkitTransform.4" property for "box1" element at 0.4s saw something close to: 20
 PASS - "webkitTransform.4" property for "box1" element at 1s saw something close to: 20
 PASS - "webkitTransform.4" property for "box1" element at 1.6s saw something close to: 15
@@ -12,4 +11,7 @@
 PASS - "webkitTransform.4" property for "box4" element at 0.4s saw something close to: 20
 PASS - "webkitTransform.4" property for "box4" element at 1s saw something close to: 25
 PASS - "webkitTransform.4" property for "box4" element at 1.6s saw something close to: 15
+PASS - "webkitTransform.4" property for "box5" element at 0.4s saw something close to: 10
+PASS - "webkitTransform.4" property for "box5" element at 1s saw something close to: 10
+PASS - "webkitTransform.4" property for "box5" element at 1.6s saw something close to: 10
 

Modified: trunk/LayoutTests/animations/missing-from-to-transforms.html (231765 => 231766)


--- trunk/LayoutTests/animations/missing-from-to-transforms.html	2018-05-14 18:15:57 UTC (rev 231765)
+++ trunk/LayoutTests/animations/missing-from-to-transforms.html	2018-05-14 18:19:30 UTC (rev 231766)
@@ -71,7 +71,7 @@
   <script src="" type="text/_javascript_" charset="utf-8"></script>
   <script type="text/_javascript_" charset="utf-8">
 
-    const expectedValues = [
+    runAnimationTest([
       // [animation-name, time, element-id, property, expected-value, tolerance]
       ["anim1", 0.4, "box1", "webkitTransform.4", 20, 2],
       ["anim1", 1.0, "box1", "webkitTransform.4", 20, 2],
@@ -84,18 +84,11 @@
       ["anim3", 1.6, "box3", "webkitTransform.4", 15, 2],
       ["anim4", 0.4, "box4", "webkitTransform.4", 20, 2],
       ["anim4", 1.0, "box4", "webkitTransform.4", 25, 2],
-      ["anim4", 1.6, "box4", "webkitTransform.4", 15, 2]
-    ];
-
-    runAnimationTest(expectedValues, function() {
-      if (window.testRunner) {
-          var box5Element = document.getElementById('box5');
-          if (internals.pauseAnimationAtTimeOnElement("anim5", 0.1, box5Element))
-              result += "FAIL - box5 animation was running<br>";
-          else 
-              result += "PASS - box5 animation was not running<br>";
-      }
-    });
+      ["anim4", 1.6, "box4", "webkitTransform.4", 15, 2],
+      ["anim5", 0.4, "box5", "webkitTransform.4", 10, 0],
+      ["anim5", 1.0, "box5", "webkitTransform.4", 10, 0],
+      ["anim5", 1.6, "box5", "webkitTransform.4", 10, 0]
+    ]);
     
   </script>
 </head>

Modified: trunk/LayoutTests/animations/missing-from-to.html (231765 => 231766)


--- trunk/LayoutTests/animations/missing-from-to.html	2018-05-14 18:15:57 UTC (rev 231765)
+++ trunk/LayoutTests/animations/missing-from-to.html	2018-05-14 18:19:30 UTC (rev 231766)
@@ -1,5 +1,4 @@
-<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
-   "http://www.w3.org/TR/html4/loose.dtd">
+<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd"><!-- webkit-test-runner [ enableCSSAnimationsAndCSSTransitionsBackedByWebAnimations=true ] -->
 
 <html lang="en">
 <head>
@@ -71,7 +70,7 @@
   <script src="" type="text/_javascript_" charset="utf-8"></script>
   <script type="text/_javascript_" charset="utf-8">
 
-    const expectedValues = [
+    runAnimationTest([
       // [animation-name, time, element-id, property, expected-value, tolerance]
       ["anim1", 0.4, "box1", "left", 20, 2],
       ["anim1", 1.0, "box1", "left", 20, 2],
@@ -84,18 +83,11 @@
       ["anim3", 1.6, "box3", "left", 15, 2],
       ["anim4", 0.4, "box4", "left", 20, 2],
       ["anim4", 1.0, "box4", "left", 25, 2],
-      ["anim4", 1.6, "box4", "left", 15, 2]
-    ];
-
-    runAnimationTest(expectedValues, function() {
-      if (window.testRunner) {
-          var box5Element = document.getElementById('box5');
-          if (internals.pauseAnimationAtTimeOnElement("anim5", 0.1, box5Element))
-              result += "FAIL - box5 animation was running<br>";
-          else 
-              result += "PASS - box5 animation was not running<br>";
-      }
-    });
+      ["anim4", 1.6, "box4", "left", 15, 2],
+      ["anim5", 0.4, "box5", "left", 10, 0],
+      ["anim5", 1.0, "box5", "left", 10, 0],
+      ["anim5", 1.6, "box5", "left", 10, 0]
+    ]);
     
   </script>
 </head>

Modified: trunk/LayoutTests/fast/css-generated-content/pseudo-animation.html (231765 => 231766)


--- trunk/LayoutTests/fast/css-generated-content/pseudo-animation.html	2018-05-14 18:15:57 UTC (rev 231765)
+++ trunk/LayoutTests/fast/css-generated-content/pseudo-animation.html	2018-05-14 18:19:30 UTC (rev 231766)
@@ -1,4 +1,4 @@
-<!DOCTYPE html>
+<!DOCTYPE html><!-- webkit-test-runner [ enableCSSAnimationsAndCSSTransitionsBackedByWebAnimations=true ] -->
 
 <script src=""
 
@@ -78,6 +78,25 @@
 // FIXME: This test should be modified so subpixel doesn't cause off by one
 // below and it no longer needs shouldBeCloseTo.
 
+function pauseAnimationAtTimeOnPseudoElement(animationName, time, element, pseudoId)
+{
+    const pseudoElement = internals.pseudoElement(element, pseudoId);
+    if (!pseudoElement) {
+        console.log("Failed to find pseudo element");
+        return;
+    }
+
+    const animations = pseudoElement.getAnimations();
+    for (let animation of animations) {
+        if (animation instanceof CSSAnimation && animation.animationName == animationName && animation.effect.getKeyframes().length) {
+            animation.currentTime = time * 1000;
+            animation.pause();
+            return true;
+        }
+    }
+    return false;
+}
+
 function testAnimation(id)
 {
     var div = document.getElementById(id);
@@ -85,11 +104,11 @@
     window.div = div;
     shouldBe('div.offsetWidth', '52');
     if (window.internals) {
-        internals.pauseAnimationAtTimeOnPseudoElement('example', 1.0, div, id);
+        pauseAnimationAtTimeOnPseudoElement('example', 1.0, div, id);
         shouldBeCloseTo('div.offsetWidth', 20, 1);
         computedTop = getPseudoComputedTop(id);
         shouldBeCloseTo('computedTop', 170, 1);
-        internals.pauseAnimationAtTimeOnPseudoElement('example', 2.0, div, id);
+        pauseAnimationAtTimeOnPseudoElement('example', 2.0, div, id);
         shouldBeCloseTo('div.offsetWidth', 12, 1);
         computedTop = getPseudoComputedTop(id);
         shouldBeCloseTo('computedTop', 200, 1);

Modified: trunk/LayoutTests/transitions/transition-hit-test-transform.html (231765 => 231766)


--- trunk/LayoutTests/transitions/transition-hit-test-transform.html	2018-05-14 18:15:57 UTC (rev 231765)
+++ trunk/LayoutTests/transitions/transition-hit-test-transform.html	2018-05-14 18:19:30 UTC (rev 231766)
@@ -1,4 +1,4 @@
-<!DOCTYPE html>
+<!DOCTYPE html><!-- webkit-test-runner [ enableCSSAnimationsAndCSSTransitionsBackedByWebAnimations=true ] -->
 
 <html>
 <head>
@@ -49,7 +49,7 @@
         {
             if (window.testRunner) {
                 var target = document.getElementById('target');
-                if (!internals.pauseTransitionAtTimeOnElement("-webkit-transform", 2.0, target))
+                if (!pauseTransitionAtTimeOnElement("-webkit-transform", 2.0, target))
                     throw("Transition is not running");
         
                 checkResults();

Modified: trunk/Source/WebCore/ChangeLog (231765 => 231766)


--- trunk/Source/WebCore/ChangeLog	2018-05-14 18:15:57 UTC (rev 231765)
+++ trunk/Source/WebCore/ChangeLog	2018-05-14 18:19:30 UTC (rev 231766)
@@ -1,5 +1,21 @@
 2018-05-14  Antoine Quint  <grao...@apple.com>
 
+        [Web Animations] Tests using the new animation engine may crash under WebCore::FrameView::didDestroyRenderTree when using internals methods
+        https://bugs.webkit.org/show_bug.cgi?id=185612
+        <rdar://problem/39579344>
+
+        Reviewed by Dean Jackson.
+
+        Add a new internals.pseudoElement() method to obtain a pseudo element matching a given pseudo-id. This is necessary to be able to move off
+        internals.pauseTransitionAtTimeOnPseudoElement() and internals.pauseAnimationAtTimeOnPseudoElement() for Web Animations testing.
+
+        * testing/Internals.cpp:
+        (WebCore::Internals::pseudoElement):
+        * testing/Internals.h:
+        * testing/Internals.idl:
+
+2018-05-14  Antoine Quint  <grao...@apple.com>
+
         REGRESSION (r230574): Interrupted hardware transitions don't behave correctly
         https://bugs.webkit.org/show_bug.cgi?id=185299
         <rdar://problem/39630230>

Modified: trunk/Source/WebCore/testing/Internals.cpp (231765 => 231766)


--- trunk/Source/WebCore/testing/Internals.cpp	2018-05-14 18:15:57 UTC (rev 231765)
+++ trunk/Source/WebCore/testing/Internals.cpp	2018-05-14 18:19:30 UTC (rev 231766)
@@ -1048,6 +1048,14 @@
     return frame()->animation().pauseTransitionAtTime(*pseudoElement, property, pauseTime);
 }
 
+ExceptionOr<RefPtr<Element>> Internals::pseudoElement(Element& element, const String& pseudoId)
+{
+    if (pseudoId != "before" && pseudoId != "after")
+        return Exception { InvalidAccessError };
+
+    return pseudoId == "before" ? element.beforePseudoElement() : element.afterPseudoElement();
+}
+
 ExceptionOr<String> Internals::elementRenderTreeAsText(Element& element)
 {
     element.document().updateStyleIfNeeded();

Modified: trunk/Source/WebCore/testing/Internals.h (231765 => 231766)


--- trunk/Source/WebCore/testing/Internals.h	2018-05-14 18:15:57 UTC (rev 231765)
+++ trunk/Source/WebCore/testing/Internals.h	2018-05-14 18:19:30 UTC (rev 231766)
@@ -195,6 +195,9 @@
     ExceptionOr<bool> pauseTransitionAtTimeOnElement(const String& propertyName, double pauseTime, Element&);
     ExceptionOr<bool> pauseTransitionAtTimeOnPseudoElement(const String& property, double pauseTime, Element&, const String& pseudoId);
 
+    // For animations testing, we need a way to get at pseudo elements.
+    ExceptionOr<RefPtr<Element>> pseudoElement(Element&, const String&);
+
     Node* treeScopeRootNode(Node&);
     Node* parentTreeScope(Node&);
 

Modified: trunk/Source/WebCore/testing/Internals.idl (231765 => 231766)


--- trunk/Source/WebCore/testing/Internals.idl	2018-05-14 18:15:57 UTC (rev 231765)
+++ trunk/Source/WebCore/testing/Internals.idl	2018-05-14 18:19:30 UTC (rev 231766)
@@ -148,6 +148,9 @@
     [MayThrowException] boolean pauseTransitionAtTimeOnElement(DOMString propertyName, unrestricted double pauseTime, Element element);
     [MayThrowException] boolean pauseTransitionAtTimeOnPseudoElement(DOMString property, unrestricted double pauseTime, Element element, DOMString pseudoId);
 
+    // For animations testing, we need a way to get at pseudo elements.
+    [MayThrowException] Element? pseudoElement(Element element, DOMString pseudoId);
+
     DOMString visiblePlaceholder(Element element);
     void selectColorInColorChooser(HTMLInputElement element, DOMString colorValue);
     [MayThrowException] sequence<DOMString> formControlStateOfPreviousHistoryItem();
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to