From dd053e6b2b06d89b05c93bd77b45ba6aae1249b1 Mon Sep 17 00:00:00 2001 From: "graouts@webkit.org" Date: Mon, 14 May 2018 18:19:30 +0000 Subject: [PATCH] [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 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: git-svn-id: https://svn.webkit.org/repository/webkit/trunk@231766 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- LayoutTests/ChangeLog | 25 ++++++++++++++++++++++ .../animations/animation-hit-test-transform.html | 22 +++++++------------ .../animations/keyframes-dynamic-expected.txt | 3 ++- LayoutTests/animations/keyframes-dynamic.html | 12 +++-------- .../animations/missing-from-to-expected.txt | 4 +++- .../missing-from-to-transforms-expected.txt | 4 +++- .../animations/missing-from-to-transforms.html | 19 ++++++---------- LayoutTests/animations/missing-from-to.html | 22 ++++++------------- .../css-generated-content/pseudo-animation.html | 25 +++++++++++++++++++--- .../transitions/transition-hit-test-transform.html | 4 ++-- Source/WebCore/ChangeLog | 16 ++++++++++++++ Source/WebCore/testing/Internals.cpp | 8 +++++++ Source/WebCore/testing/Internals.h | 3 +++ Source/WebCore/testing/Internals.idl | 3 +++ 14 files changed, 111 insertions(+), 59 deletions(-) diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog index 9f0eec1..f70692b 100644 --- a/LayoutTests/ChangeLog +++ b/LayoutTests/ChangeLog @@ -1,5 +1,30 @@ 2018-05-14 Antoine Quint + [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 + + + 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 + REGRESSION (r230574): Interrupted hardware transitions don't behave correctly https://bugs.webkit.org/show_bug.cgi?id=185299 diff --git a/LayoutTests/animations/animation-hit-test-transform.html b/LayoutTests/animations/animation-hit-test-transform.html index 49f50e7..7807bc7 100644 --- a/LayoutTests/animations/animation-hit-test-transform.html +++ b/LayoutTests/animations/animation-hit-test-transform.html @@ -1,5 +1,4 @@ - + @@ -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() diff --git a/LayoutTests/animations/keyframes-dynamic-expected.txt b/LayoutTests/animations/keyframes-dynamic-expected.txt index d209f18..cf6e10c 100644 --- a/LayoutTests/animations/keyframes-dynamic-expected.txt +++ b/LayoutTests/animations/keyframes-dynamic-expected.txt @@ -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 diff --git a/LayoutTests/animations/keyframes-dynamic.html b/LayoutTests/animations/keyframes-dynamic.html index 9d78f3d..cdd40c1 100644 --- a/LayoutTests/animations/keyframes-dynamic.html +++ b/LayoutTests/animations/keyframes-dynamic.html @@ -1,5 +1,4 @@ - + @@ -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
"; - else - result += "PASS - box3 animation was not running
"; - } } window.addEventListener('DOMContentLoaded', addKeyframes, false); diff --git a/LayoutTests/animations/missing-from-to-expected.txt b/LayoutTests/animations/missing-from-to-expected.txt index 4ddf27d..5b3ff41 100644 --- a/LayoutTests/animations/missing-from-to-expected.txt +++ b/LayoutTests/animations/missing-from-to-expected.txt @@ -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 "box3" element at 1.6s saw something close to: 15 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 diff --git a/LayoutTests/animations/missing-from-to-transforms-expected.txt b/LayoutTests/animations/missing-from-to-transforms-expected.txt index a40bc1d..e5da3b2 100644 --- a/LayoutTests/animations/missing-from-to-transforms-expected.txt +++ b/LayoutTests/animations/missing-from-to-transforms-expected.txt @@ -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 "box3" element at 1.6s saw something clo 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 diff --git a/LayoutTests/animations/missing-from-to-transforms.html b/LayoutTests/animations/missing-from-to-transforms.html index debeaa3..e86fef6 100644 --- a/LayoutTests/animations/missing-from-to-transforms.html +++ b/LayoutTests/animations/missing-from-to-transforms.html @@ -71,7 +71,7 @@ diff --git a/LayoutTests/animations/missing-from-to.html b/LayoutTests/animations/missing-from-to.html index 851707f..28c1f6c 100644 --- a/LayoutTests/animations/missing-from-to.html +++ b/LayoutTests/animations/missing-from-to.html @@ -1,5 +1,4 @@ - + @@ -71,7 +70,7 @@ diff --git a/LayoutTests/fast/css-generated-content/pseudo-animation.html b/LayoutTests/fast/css-generated-content/pseudo-animation.html index 3a0c6c4..b05132c 100644 --- a/LayoutTests/fast/css-generated-content/pseudo-animation.html +++ b/LayoutTests/fast/css-generated-content/pseudo-animation.html @@ -1,4 +1,4 @@ - + @@ -78,6 +78,25 @@ function getPseudoComputedTop(id) // 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 @@ function testAnimation(id) 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); diff --git a/LayoutTests/transitions/transition-hit-test-transform.html b/LayoutTests/transitions/transition-hit-test-transform.html index 1c0a035..8874816 100644 --- a/LayoutTests/transitions/transition-hit-test-transform.html +++ b/LayoutTests/transitions/transition-hit-test-transform.html @@ -1,4 +1,4 @@ - + @@ -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(); diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog index 98e520b..01d58a3 100644 --- a/Source/WebCore/ChangeLog +++ b/Source/WebCore/ChangeLog @@ -1,5 +1,21 @@ 2018-05-14 Antoine Quint + [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 + + + 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 + REGRESSION (r230574): Interrupted hardware transitions don't behave correctly https://bugs.webkit.org/show_bug.cgi?id=185299 diff --git a/Source/WebCore/testing/Internals.cpp b/Source/WebCore/testing/Internals.cpp index 81a4009..09aa0a9 100644 --- a/Source/WebCore/testing/Internals.cpp +++ b/Source/WebCore/testing/Internals.cpp @@ -1048,6 +1048,14 @@ ExceptionOr Internals::pauseTransitionAtTimeOnPseudoElement(const String& return frame()->animation().pauseTransitionAtTime(*pseudoElement, property, pauseTime); } +ExceptionOr> Internals::pseudoElement(Element& element, const String& pseudoId) +{ + if (pseudoId != "before" && pseudoId != "after") + return Exception { InvalidAccessError }; + + return pseudoId == "before" ? element.beforePseudoElement() : element.afterPseudoElement(); +} + ExceptionOr Internals::elementRenderTreeAsText(Element& element) { element.document().updateStyleIfNeeded(); diff --git a/Source/WebCore/testing/Internals.h b/Source/WebCore/testing/Internals.h index 1b83bcb..0823637 100644 --- a/Source/WebCore/testing/Internals.h +++ b/Source/WebCore/testing/Internals.h @@ -195,6 +195,9 @@ public: ExceptionOr pauseTransitionAtTimeOnElement(const String& propertyName, double pauseTime, Element&); ExceptionOr pauseTransitionAtTimeOnPseudoElement(const String& property, double pauseTime, Element&, const String& pseudoId); + // For animations testing, we need a way to get at pseudo elements. + ExceptionOr> pseudoElement(Element&, const String&); + Node* treeScopeRootNode(Node&); Node* parentTreeScope(Node&); diff --git a/Source/WebCore/testing/Internals.idl b/Source/WebCore/testing/Internals.idl index 8d9c267..cb508ca 100644 --- a/Source/WebCore/testing/Internals.idl +++ b/Source/WebCore/testing/Internals.idl @@ -148,6 +148,9 @@ enum EventThrottlingBehavior { [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 formControlStateOfPreviousHistoryItem(); -- 1.8.3.1