Flaky crash in AnimationTimeline::cancelOrRemoveDeclarativeAnimation
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 20 Jul 2018 00:06:10 +0000 (00:06 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 20 Jul 2018 00:06:10 +0000 (00:06 +0000)
https://bugs.webkit.org/show_bug.cgi?id=187530
<rdar://problem/42095186>

Reviewed by Dean Jackson.

LayoutTests/imported/mozilla:

Mark a WPT progression now that we correctly ignore animation names that have no matching @keyframes rule.

* css-animations/test_element-get-animations-expected.txt:

Source/WebCore:

We would crash in cancelOrRemoveDeclarativeAnimation() because updateCSSAnimationsForElement() would pass
nullptr values due to the return value of cssAnimationsByName.take(nameOfAnimationToRemove). This is because
we would create animations for animation names that may be empty or not match an existing @keyframes rule.
Not only was that wasteful, but it was also non-compliant, and as a result of fixing this we're actually
seeing a progression in the CSS Animations WPT tests.

* animation/AnimationTimeline.cpp:
(WebCore::shouldConsiderAnimation): New function that performs all required steps to see if a provided animation
is valid and has a name that is not "none", not the empty string and matches the name of a @keyframes rule.
(WebCore::AnimationTimeline::updateCSSAnimationsForElement):
* animation/KeyframeEffectReadOnly.cpp:
(WebCore::KeyframeEffectReadOnly::computeCSSAnimationBlendingKeyframes): We no longer need to check whether we have
an empty animation name since we're no longer creating CSSAnimation objects in that circumstance.
* css/StyleResolver.cpp:
(WebCore::StyleResolver::isAnimationNameValid): Add a new method that checks whether the provided animation name
a known @keyframes rule.
* css/StyleResolver.h:

LayoutTests:

Adjust an existing test which assumes an animation might be running when it's not really, so we test the animation is
not running using an alternate method.

* animations/keyframes-dynamic-expected.txt:
* animations/keyframes-dynamic.html:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@234017 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/animations/keyframes-dynamic-expected.txt
LayoutTests/animations/keyframes-dynamic.html
LayoutTests/imported/mozilla/ChangeLog
LayoutTests/imported/mozilla/css-animations/test_element-get-animations-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/animation/AnimationTimeline.cpp
Source/WebCore/animation/KeyframeEffectReadOnly.cpp
Source/WebCore/css/StyleResolver.cpp
Source/WebCore/css/StyleResolver.h

index af34e4e..3366be9 100644 (file)
@@ -1,3 +1,17 @@
+2018-07-19  Antoine Quint  <graouts@apple.com>
+
+        Flaky crash in AnimationTimeline::cancelOrRemoveDeclarativeAnimation
+        https://bugs.webkit.org/show_bug.cgi?id=187530
+        <rdar://problem/42095186>
+
+        Reviewed by Dean Jackson.
+
+        Adjust an existing test which assumes an animation might be running when it's not really, so we test the animation is
+        not running using an alternate method.
+
+        * animations/keyframes-dynamic-expected.txt:
+        * animations/keyframes-dynamic.html:
+
 2018-07-19  Ryan Haddad  <ryanhaddad@apple.com>
 
         Mark storage/indexeddb/modern/opendatabase-after-storage-crash.html as flaky.
index cf6e10c..c11806c 100644 (file)
@@ -3,6 +3,4 @@ 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
 
index 2c718a2..dce9617 100644 (file)
@@ -27,8 +27,6 @@
       ["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() {
 
         style.sheet.removeRule(box3Index);
 
-        runAnimationTest(expectedValues);
+        runAnimationTest(expectedValues, () => {
+            setTimeout(() => {
+                document.getElementById("result").appendChild(document.createTextNode(`${!box3.getAnimations().length ? "PASS" : "FAIL"} - Animation is not running on box3.`));
+            })
+        });
     }
     
     window.addEventListener('DOMContentLoaded', addKeyframes, false);
index 8990aed..9222035 100644 (file)
@@ -1,3 +1,15 @@
+2018-07-19  Antoine Quint  <graouts@apple.com>
+
+        Flaky crash in AnimationTimeline::cancelOrRemoveDeclarativeAnimation
+        https://bugs.webkit.org/show_bug.cgi?id=187530
+        <rdar://problem/42095186>
+
+        Reviewed by Dean Jackson.
+
+        Mark a WPT progression now that we correctly ignore animation names that have no matching @keyframes rule.
+
+        * css-animations/test_element-get-animations-expected.txt:
+
 2018-07-03  Antoine Quint  <graouts@apple.com>
 
         Unreviewed, rebaselining somes Web Animations test expectations.
index abfa296..9497cef 100644 (file)
@@ -7,8 +7,8 @@ PASS getAnimations for both CSS Animations and CSS Transitions at once
 PASS getAnimations for CSS Animations that have finished 
 PASS getAnimations for CSS Animations that have finished but are forwards filling 
 PASS getAnimations for CSS Animations with animation-name: none 
-FAIL getAnimations for CSS Animations with animation-name: missing assert_equals: getAnimations returns an empty sequence for an element with animation-name: missing expected 0 but got 1
-FAIL getAnimations for CSS Animations where the @keyframes rule is added later assert_equals: getAnimations initally only returns Animations for CSS Animations whose animation-name is found expected 1 but got 2
+PASS getAnimations for CSS Animations with animation-name: missing 
+FAIL getAnimations for CSS Animations where the @keyframes rule is added later assert_equals: getAnimations includes Animation when @keyframes rule is added later expected 2 but got 1
 PASS getAnimations for CSS Animations with duplicated animation-name 
 PASS getAnimations for CSS Animations with empty keyframes rule 
 PASS getAnimations for CSS animations in delay phase 
index aec9f6b..88de84d 100644 (file)
@@ -1,3 +1,29 @@
+2018-07-19  Antoine Quint  <graouts@apple.com>
+
+        Flaky crash in AnimationTimeline::cancelOrRemoveDeclarativeAnimation
+        https://bugs.webkit.org/show_bug.cgi?id=187530
+        <rdar://problem/42095186>
+
+        Reviewed by Dean Jackson.
+
+        We would crash in cancelOrRemoveDeclarativeAnimation() because updateCSSAnimationsForElement() would pass
+        nullptr values due to the return value of cssAnimationsByName.take(nameOfAnimationToRemove). This is because
+        we would create animations for animation names that may be empty or not match an existing @keyframes rule.
+        Not only was that wasteful, but it was also non-compliant, and as a result of fixing this we're actually
+        seeing a progression in the CSS Animations WPT tests.
+
+        * animation/AnimationTimeline.cpp:
+        (WebCore::shouldConsiderAnimation): New function that performs all required steps to see if a provided animation
+        is valid and has a name that is not "none", not the empty string and matches the name of a @keyframes rule.
+        (WebCore::AnimationTimeline::updateCSSAnimationsForElement):
+        * animation/KeyframeEffectReadOnly.cpp:
+        (WebCore::KeyframeEffectReadOnly::computeCSSAnimationBlendingKeyframes): We no longer need to check whether we have
+        an empty animation name since we're no longer creating CSSAnimation objects in that circumstance.
+        * css/StyleResolver.cpp:
+        (WebCore::StyleResolver::isAnimationNameValid): Add a new method that checks whether the provided animation name
+        a known @keyframes rule.
+        * css/StyleResolver.h:
+
 2018-07-19  Chris Dumez  <cdumez@apple.com>
 
         Crash under WebCore::DocumentWriter::addData()
index 01860b9..8a0fce9 100644 (file)
@@ -39,6 +39,7 @@
 #include "RenderStyle.h"
 #include "RenderView.h"
 #include "StylePropertyShorthand.h"
+#include "StyleResolver.h"
 #include "WebAnimationUtilities.h"
 #include <wtf/text/TextStream.h>
 #include <wtf/text/WTFString.h>
@@ -195,6 +196,23 @@ void AnimationTimeline::cancelDeclarativeAnimationsForElement(Element& element)
         cssAnimation->cancel();
 }
 
+static bool shouldConsiderAnimation(Element& element, const Animation& animation)
+{
+    if (!animation.isValidAnimation())
+        return false;
+
+    static NeverDestroyed<const String> animationNameNone(MAKE_STATIC_STRING_IMPL("none"));
+
+    auto& name = animation.name();
+    if (name == animationNameNone || name.isEmpty())
+        return false;
+
+    if (auto* styleScope = Style::Scope::forOrdinal(element, animation.nameStyleScopeOrdinal()))
+        return styleScope->resolver().isAnimationNameValid(name);
+
+    return false;
+}
+
 void AnimationTimeline::updateCSSAnimationsForElement(Element& element, const RenderStyle* currentStyle, const RenderStyle& afterChangeStyle)
 {
     // In case this element is newly getting a "display: none" we need to cancel all of its animations and disregard new ones.
@@ -209,15 +227,13 @@ void AnimationTimeline::updateCSSAnimationsForElement(Element& element, const Re
     if (currentStyle && currentStyle->hasAnimations() && afterChangeStyle.hasAnimations() && *(currentStyle->animations()) == *(afterChangeStyle.animations()))
         return;
 
-    static NeverDestroyed<const String> animationNameNone(MAKE_STATIC_STRING_IMPL("none"));
-
     // First, compile the list of animation names that were applied to this element up to this point.
     HashSet<String> namesOfPreviousAnimations;
     if (currentStyle && currentStyle->hasAnimations()) {
         auto* previousAnimations = currentStyle->animations();
         for (size_t i = 0; i < previousAnimations->size(); ++i) {
             auto& previousAnimation = previousAnimations->animation(i);
-            if (previousAnimation.isValidAnimation() && previousAnimation.name() != animationNameNone)
+            if (shouldConsiderAnimation(element, previousAnimation))
                 namesOfPreviousAnimations.add(previousAnimation.name());
         }
     }
@@ -236,7 +252,7 @@ void AnimationTimeline::updateCSSAnimationsForElement(Element& element, const Re
                 // created a CSSAnimation object for it and need to ensure that this CSSAnimation is backed by the current
                 // animation object for this animation name.
                 cssAnimationsByName.get(name)->setBackingAnimation(currentAnimation);
-            } else if (currentAnimation.isValidAnimation() && name != animationNameNone) {
+            } else if (shouldConsiderAnimation(element, currentAnimation)) {
                 // Otherwise we are dealing with a new animation name and must create a CSSAnimation for it.
                 cssAnimationsByName.set(name, CSSAnimation::create(element, currentAnimation, currentStyle, afterChangeStyle));
             }
index ebff9f2..f54bb4c 100644 (file)
@@ -871,8 +871,6 @@ void KeyframeEffectReadOnly::computeCSSAnimationBlendingKeyframes()
 
     auto cssAnimation = downcast<CSSAnimation>(animation());
     auto& backingAnimation = cssAnimation->backingAnimation();
-    if (backingAnimation.name().isEmpty())
-        return;
 
     KeyframeList keyframeList(backingAnimation.name());
     if (auto* styleScope = Style::Scope::forOrdinal(*m_target, backingAnimation.nameStyleScopeOrdinal()))
index 7ce3dbd..2dc5738 100644 (file)
@@ -460,6 +460,11 @@ std::unique_ptr<RenderStyle> StyleResolver::styleForKeyframe(const RenderStyle*
     return state.takeStyle();
 }
 
+bool StyleResolver::isAnimationNameValid(const String& name)
+{
+    return m_keyframesRuleMap.find(AtomicString(name).impl()) != m_keyframesRuleMap.end();
+}
+
 void StyleResolver::keyframeStylesForAnimation(const Element& element, const RenderStyle* elementStyle, KeyframeList& list)
 {
     list.clear();
index ecde64a..4e48c30 100644 (file)
@@ -161,6 +161,7 @@ public:
 
     void setNewStateWithElement(const Element&);
     std::unique_ptr<RenderStyle> styleForKeyframe(const RenderStyle*, const StyleRuleKeyframe*, KeyframeValue&);
+    bool isAnimationNameValid(const String&);
 
 public:
     // These methods will give back the set of rules that matched for a given element (or a pseudo-element).