Attachment 347072 Details for Bug 188519 – Patch

[patch] Patch

bug-188519-20180814162344.patch (text/plain), 5.82 KB, created by Antoine Quint on 2018-08-14 07:23:45 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Antoine Quint

Created: 2018-08-14 07:23:45 PDT

Size: 5.82 KB

patch

obsolete

>Subversion Revision: 234792
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 03093315abac2a9ec270973a427b83d0b7e77251..8980c9e0233ffd732973e7342ef44222aaff96fc 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        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-12  Sihui Liu  <sihui_liu@apple.com>
> 
>         CrashTracer: com.apple.WebKit.Storage at WebCore::IDBServer::UniqueIDBDatabase::connectionClosedFromClient(WebCore::IDBServer::UniqueIDBDatabaseConnection&)
>diff --git a/Source/WebCore/animation/AnimationTimeline.cpp b/Source/WebCore/animation/AnimationTimeline.cpp
>index 88791a253cb4133dc4c16ecf9b8204e7a099a7c5..342b2835480e09bb7f85d31e1dec927e68885f30 100644
>--- a/Source/WebCore/animation/AnimationTimeline.cpp
>+++ b/Source/WebCore/animation/AnimationTimeline.cpp
>@@ -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) {
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index da6a61d89d7c06335f28d668d3256af74ebade51..88eb8a5a1ece7b5327314b4774e0e0f797273d9c 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        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-12  Michael Catanzaro  <mcatanzaro@igalia.com>
> 
>         Unreviewed GTK test gardening
>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
>index 0000000000000000000000000000000000000000..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
>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
>index 0000000000000000000000000000000000000000..51305aba90460b4ae39fdf14ea5e2c724debb4dc
>--- /dev/null
>+++ b/LayoutTests/webanimations/css-animation-effect-target-change-and-animation-removal-crash.html
>@@ -0,0 +1,26 @@
>+<!DOCTYPE html>
>+<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>

Flags:

eric.carlson: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 188519: 347072