[Web Animations] Crash under AnimationTimeline::cancelOrRemoveDeclarativeAnimation()
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 14 Aug 2018 15:15:07 +0000 (15:15 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 14 Aug 2018 15:15:07 +0000 (15:15 +0000)
https://bugs.webkit.org/show_bug.cgi?id=188519
<rdar://problem/43237889>

Reviewed by Eric Carlson.

Source/WebCore:

Test: webanimations/css-animation-effect-target-change-and-animation-removal-crash.html

We would crash because we blindly assumed an animation that was found in the previous style must be in the list of running animations
but in fact it could have been removed already due to the element being removed from the DOM or its effect target changing, etc. So when
we iterate over names of animations that were found in the previous style but not in the new style, we must make a null check to ensure
that there is an animation to remove. Adding an ASSERT() in AnimationTimeline::cancelOrRemoveDeclarativeAnimation() will also clarify the
expectation here.

* animation/AnimationTimeline.cpp:
(WebCore::AnimationTimeline::updateCSSAnimationsForElement):
(WebCore::AnimationTimeline::cancelOrRemoveDeclarativeAnimation):

LayoutTests:

Add a test where we clone the effect to be mutable and set a new target. At this stage the animation is no longer listed in the
m_elementToCSSAnimationByName map on AnimationTimeline. Then we remove the animation and force a style recalc for this element,
"anim" will be in the old style but not in the new style and we used to attempt to get an animation matching that name from
m_elementToCSSAnimationByName but it would be null, which would lead to a crash. Now we check that we indeed have such an animation
before proceeding.

* webanimations/css-animation-effect-target-change-and-animation-removal-crash-expected.html: Added.
* webanimations/css-animation-effect-target-change-and-animation-removal-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/webanimations/css-animation-effect-target-change-and-animation-removal-crash-expected.html [new file with mode: 0644]
LayoutTests/webanimations/css-animation-effect-target-change-and-animation-removal-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/animation/AnimationTimeline.cpp

index e563eb3..ee74960 100644 (file)
@@ -1,3 +1,20 @@
+2018-08-14  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Crash under AnimationTimeline::cancelOrRemoveDeclarativeAnimation()
+        https://bugs.webkit.org/show_bug.cgi?id=188519
+        <rdar://problem/43237889>
+
+        Reviewed by Eric Carlson.
+
+        Add a test where we clone the effect to be mutable and set a new target. At this stage the animation is no longer listed in the
+        m_elementToCSSAnimationByName map on AnimationTimeline. Then we remove the animation and force a style recalc for this element,
+        "anim" will be in the old style but not in the new style and we used to attempt to get an animation matching that name from
+        m_elementToCSSAnimationByName but it would be null, which would lead to a crash. Now we check that we indeed have such an animation
+        before proceeding.
+
+        * webanimations/css-animation-effect-target-change-and-animation-removal-crash-expected.html: Added.
+        * webanimations/css-animation-effect-target-change-and-animation-removal-crash.html: Added.
+
 2018-08-14  Zalan Bujtas  <zalan@apple.com>
 
         [LFC][Floating] Adjust vertical position with non-collapsing previous sibling margin.
diff --git a/LayoutTests/webanimations/css-animation-effect-target-change-and-animation-removal-crash-expected.html b/LayoutTests/webanimations/css-animation-effect-target-change-and-animation-removal-crash-expected.html
new file mode 100644 (file)
index 0000000..e69de29
diff --git a/LayoutTests/webanimations/css-animation-effect-target-change-and-animation-removal-crash.html b/LayoutTests/webanimations/css-animation-effect-target-change-and-animation-removal-crash.html
new file mode 100644 (file)
index 0000000..51305ab
--- /dev/null
@@ -0,0 +1,26 @@
+<!DOCTYPE html><!-- webkit-test-runner [ enableWebAnimationsCSSIntegration=true ] -->
+<body>
+<style>
+
+@keyframes anim {
+    from { margin-left: 0 }
+    to   { margin-left: 100px }
+}
+
+</style>
+<script>
+
+const target = document.body.appendChild(document.createElement("div"));
+target.style.animation = "anim 1s";
+const animation = target.getAnimations()[0];
+
+animation.effect = new KeyframeEffect(animation.effect);
+animation.effect.target = document.createElement("div");
+
+target.style.animation = "none";
+target.getAnimations();
+
+target.remove();
+
+</script>
+</body>
index 3d95895..6eb7da9 100644 (file)
@@ -1,3 +1,23 @@
+2018-08-14  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Crash under AnimationTimeline::cancelOrRemoveDeclarativeAnimation()
+        https://bugs.webkit.org/show_bug.cgi?id=188519
+        <rdar://problem/43237889>
+
+        Reviewed by Eric Carlson.
+
+        Test: webanimations/css-animation-effect-target-change-and-animation-removal-crash.html
+
+        We would crash because we blindly assumed an animation that was found in the previous style must be in the list of running animations
+        but in fact it could have been removed already due to the element being removed from the DOM or its effect target changing, etc. So when
+        we iterate over names of animations that were found in the previous style but not in the new style, we must make a null check to ensure
+        that there is an animation to remove. Adding an ASSERT() in AnimationTimeline::cancelOrRemoveDeclarativeAnimation() will also clarify the
+        expectation here.
+
+        * animation/AnimationTimeline.cpp:
+        (WebCore::AnimationTimeline::updateCSSAnimationsForElement):
+        (WebCore::AnimationTimeline::cancelOrRemoveDeclarativeAnimation):
+
 2018-08-14  Zalan Bujtas  <zalan@apple.com>
 
         [LFC][Floating] Adjust vertical position with non-collapsed previous sibling margin.
index 88791a2..342b283 100644 (file)
@@ -272,8 +272,10 @@ void AnimationTimeline::updateCSSAnimationsForElement(Element& element, const Re
 
     // The animations names left in namesOfPreviousAnimations are now known to no longer apply so we need to
     // remove the CSSAnimation object created for them.
-    for (const auto& nameOfAnimationToRemove : namesOfPreviousAnimations)
-        cancelOrRemoveDeclarativeAnimation(cssAnimationsByName.take(nameOfAnimationToRemove));
+    for (const auto& nameOfAnimationToRemove : namesOfPreviousAnimations) {
+        if (auto animation = cssAnimationsByName.take(nameOfAnimationToRemove))
+            cancelOrRemoveDeclarativeAnimation(animation);
+    }
 
     // Remove the map of CSSAnimations by animation name for this element if it's now empty.
     if (cssAnimationsByName.isEmpty())
@@ -474,6 +476,7 @@ void AnimationTimeline::updateCSSTransitionsForElement(Element& element, const R
 
 void AnimationTimeline::cancelOrRemoveDeclarativeAnimation(RefPtr<DeclarativeAnimation> animation)
 {
+    ASSERT(animation);
     if (auto* effect = animation->effect()) {
         auto phase = effect->phase();
         if (phase != AnimationEffectReadOnly::Phase::Idle && phase != AnimationEffectReadOnly::Phase::After) {