Changeset 234017 in webkit

Timestamp:

Jul 19, 2018 5:06:10 PM (6 years ago)

Author:

graouts@webkit.org

Message:

Flaky crash in AnimationTimeline::cancelOrRemoveDeclarativeAnimation
​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:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/animations/keyframes-dynamic-expected.txt (modified) (1 diff)
• LayoutTests/animations/keyframes-dynamic.html (modified) (2 diffs)
• LayoutTests/imported/mozilla/ChangeLog (modified) (1 diff)
• LayoutTests/imported/mozilla/css-animations/test_element-get-animations-expected.txt (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/animation/AnimationTimeline.cpp (modified) (5 diffs)
• Source/WebCore/animation/KeyframeEffectReadOnly.cpp (modified) (1 diff)
• Source/WebCore/css/StyleResolver.cpp (modified) (1 diff)
• Source/WebCore/css/StyleResolver.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/LayoutTests/animations/keyframes-dynamic-expected.txt

@@ -4 +4 @@
 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
 

trunk/LayoutTests/animations/keyframes-dynamic.html

@@ -28 +28 @@
       ["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],
     ];
 
@@ -65 +63 @@
         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.`));
+            })
+        });
     }
     

trunk/LayoutTests/imported/mozilla/ChangeLog

@@ +1 @@
+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>
 

trunk/LayoutTests/imported/mozilla/css-animations/test_element-get-animations-expected.txt

@@ -8 +8 @@
 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 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/animation/AnimationTimeline.cpp

@@ -40 +40 @@
 #include "RenderView.h"
 #include "StylePropertyShorthand.h"
+#include "StyleResolver.h"
 #include "WebAnimationUtilities.h"
 #include <wtf/text/TextStream.h>
@@ -196 +197 @@
 }
 
+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)
 {
@@ -209 +227 @@
     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.
@@ -218 +234 @@
         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());
         }
@@ -237 +253 @@
                 // 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));

trunk/Source/WebCore/animation/KeyframeEffectReadOnly.cpp

@@ -872 +872 @@
     auto cssAnimation = downcast<CSSAnimation>(animation());
     auto& backingAnimation = cssAnimation->backingAnimation();
-    if (backingAnimation.name().isEmpty())
-        return;
 
     KeyframeList keyframeList(backingAnimation.name());

trunk/Source/WebCore/css/StyleResolver.cpp

@@ -461 +461 @@
 }
 
+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)
 {

trunk/Source/WebCore/css/StyleResolver.h

@@ -162 +162 @@
     void setNewStateWithElement(const Element&);
     std::unique_ptr<RenderStyle> styleForKeyframe(const RenderStyle*, const StyleRuleKeyframe*, KeyframeValue&);
+    bool isAnimationNameValid(const String&);
 
 public: