Title: [234848] trunk
- Revision
- 234848
- Author
- [email protected]
- Date
- 2018-08-14 08:15:07 -0700 (Tue, 14 Aug 2018)
Log Message
[Web Animations] Crash under AnimationTimeline::cancelOrRemoveDeclarativeAnimation()
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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (234847 => 234848)
--- trunk/LayoutTests/ChangeLog 2018-08-14 14:22:45 UTC (rev 234847)
+++ trunk/LayoutTests/ChangeLog 2018-08-14 15:15:07 UTC (rev 234848)
@@ -1,3 +1,20 @@
+2018-08-14 Antoine Quint <[email protected]>
+
+ [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 <[email protected]>
[LFC][Floating] Adjust vertical position with non-collapsing previous sibling margin.
Added: trunk/LayoutTests/webanimations/css-animation-effect-target-change-and-animation-removal-crash-expected.html ( => )
Added: trunk/LayoutTests/webanimations/css-animation-effect-target-change-and-animation-removal-crash.html
===================================================================
--- trunk/LayoutTests/webanimations/css-animation-effect-target-change-and-animation-removal-crash.html (rev 0)
+++ trunk/LayoutTests/webanimations/css-animation-effect-target-change-and-animation-removal-crash.html 2018-08-14 15:15:07 UTC (rev 234848)
@@ -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>
Modified: trunk/Source/WebCore/ChangeLog (234847 => 234848)
--- trunk/Source/WebCore/ChangeLog 2018-08-14 14:22:45 UTC (rev 234847)
+++ trunk/Source/WebCore/ChangeLog 2018-08-14 15:15:07 UTC (rev 234848)
@@ -1,3 +1,23 @@
+2018-08-14 Antoine Quint <[email protected]>
+
+ [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 <[email protected]>
[LFC][Floating] Adjust vertical position with non-collapsed previous sibling margin.
Modified: trunk/Source/WebCore/animation/AnimationTimeline.cpp (234847 => 234848)
--- trunk/Source/WebCore/animation/AnimationTimeline.cpp 2018-08-14 14:22:45 UTC (rev 234847)
+++ trunk/Source/WebCore/animation/AnimationTimeline.cpp 2018-08-14 15:15:07 UTC (rev 234848)
@@ -272,8 +272,10 @@
// 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::cancelOrRemoveDeclarativeAnimation(RefPtr<DeclarativeAnimation> animation)
{
+ ASSERT(animation);
if (auto* effect = animation->effect()) {
auto phase = effect->phase();
if (phase != AnimationEffectReadOnly::Phase::Idle && phase != AnimationEffectReadOnly::Phase::After) {
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
