[SVG] Detach list wrappers before resetting the base value.

https://bugs.webkit.org/show_bug.cgi?id=180912 <rdar://problem/36017970> Reviewed by Simon Fraser. Source/WebCore: Before resetting the animation value (and destroying the assigned SVG object -SVGLengthValue in this case), we need to check if there's an associated tear off wrapper for the said SVG object and make a copy of it. This is currently done in the wrong order through animValDidChange. Test: svg/animations/crash-when-animation-is-running-while-getting-value.html * svg/SVGAnimatedTypeAnimator.h: (WebCore::SVGAnimatedTypeAnimator::resetFromBaseValue): * svg/properties/SVGAnimatedPropertyTearOff.h: * svg/properties/SVGAnimatedStaticPropertyTearOff.h: (WebCore::SVGAnimatedStaticPropertyTearOff::synchronizeWrappersIfNeeded): LayoutTests: * svg/animations/crash-when-animation-is-running-while-getting-value-expected.txt: Added. * svg/animations/crash-when-animation-is-running-while-getting-value.html: Added. Canonical link: https://commits.webkit.org/196826@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@226065 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Dec 18, 2017 · 4751067 · 4751067
1 parent 2162e5a
commit 4751067
Show file tree

Hide file tree

Showing 7 changed files with 63 additions and 1 deletion.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,14 @@
+2017-12-18  Zalan Bujtas  <zalan@apple.com>
+
+        [SVG] Detach list wrappers before resetting the base value.
+        https://bugs.webkit.org/show_bug.cgi?id=180912
+        <rdar://problem/36017970>
+
+        Reviewed by Simon Fraser.
+
+        * svg/animations/crash-when-animation-is-running-while-getting-value-expected.txt: Added.
+        * svg/animations/crash-when-animation-is-running-while-getting-value.html: Added.
+
 2017-12-18  Jer Noble  <jer.noble@apple.com>
 
         Playing media elements which call "pause(); play()" will have the play promise rejected.

diff --git a/LayoutTests/svg/animations/crash-when-animation-is-running-while-getting-value-expected.txt b/LayoutTests/svg/animations/crash-when-animation-is-running-while-getting-value-expected.txt
@@ -0,0 +1,2 @@
+PASS if no crash. 
+
diff --git a/LayoutTests/svg/animations/crash-when-animation-is-running-while-getting-value.html b/LayoutTests/svg/animations/crash-when-animation-is-running-while-getting-value.html
@@ -0,0 +1,16 @@
+PASS if no crash.
+<svg>
+<text x="1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1" id="textElement">
+<set attributeName="x" to="0"/>
+</svg>
+<script>
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+}
+setTimeout(function() {
+    textElement.x.animVal.getItem(0);
+    if (window.testRunner)
+        testRunner.notifyDone();
+}, 0);
+</script>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,23 @@
+2017-12-18  Zalan Bujtas  <zalan@apple.com>
+
+        [SVG] Detach list wrappers before resetting the base value.
+        https://bugs.webkit.org/show_bug.cgi?id=180912
+        <rdar://problem/36017970>
+
+        Reviewed by Simon Fraser.
+
+        Before resetting the animation value (and destroying the assigned SVG object -SVGLengthValue in this case),
+        we need to check if there's an associated tear off wrapper for the said SVG object and make a copy of it.
+        This is currently done in the wrong order through animValDidChange.
+
+        Test: svg/animations/crash-when-animation-is-running-while-getting-value.html
+
+        * svg/SVGAnimatedTypeAnimator.h:
+        (WebCore::SVGAnimatedTypeAnimator::resetFromBaseValue):
+        * svg/properties/SVGAnimatedPropertyTearOff.h:
+        * svg/properties/SVGAnimatedStaticPropertyTearOff.h:
+        (WebCore::SVGAnimatedStaticPropertyTearOff::synchronizeWrappersIfNeeded):
+
 2017-12-18  Brady Eidson  <beidson@apple.com>
 
         REGRESSION: ASSERTION FAILED: !m_importCompleted

diff --git a/Source/WebCore/svg/SVGAnimatedTypeAnimator.h b/Source/WebCore/svg/SVGAnimatedTypeAnimator.h
@@ -78,8 +78,11 @@ class SVGAnimatedTypeAnimator {
     {
         ASSERT(animatedTypes[0].properties.size() == 1);
         ASSERT(type.type() == m_type);
+        auto* property = castAnimatedPropertyToActualType<AnimValType>(animatedTypes[0].properties[0].get());
+        property->synchronizeWrappersIfNeeded();
+
         typename AnimValType::ContentType& animatedTypeValue = (type.*getter)();
-        animatedTypeValue = castAnimatedPropertyToActualType<AnimValType>(animatedTypes[0].properties[0].get())->currentBaseValue();
+        animatedTypeValue = property->currentBaseValue();
 
         executeAction<AnimValType>(StartAnimationAction, animatedTypes, 0, &animatedTypeValue);
     }

diff --git a/Source/WebCore/svg/properties/SVGAnimatedPropertyTearOff.h b/Source/WebCore/svg/properties/SVGAnimatedPropertyTearOff.h
@@ -106,6 +106,11 @@ class SVGAnimatedPropertyTearOff final : public SVGAnimatedProperty {
         ASSERT(isAnimating());
     }
 
+    void synchronizeWrappersIfNeeded()
+    {
+        // no-op
+    }
+
 private:
     SVGAnimatedPropertyTearOff(SVGElement* contextElement, const QualifiedName& attributeName, AnimatedPropertyType animatedPropertyType, PropertyType& property)
         : SVGAnimatedProperty(contextElement, attributeName, animatedPropertyType)

diff --git a/Source/WebCore/svg/properties/SVGAnimatedStaticPropertyTearOff.h b/Source/WebCore/svg/properties/SVGAnimatedStaticPropertyTearOff.h
@@ -93,6 +93,11 @@ class SVGAnimatedStaticPropertyTearOff : public SVGAnimatedProperty {
         ASSERT(isAnimating());
     }
 
+    void synchronizeWrappersIfNeeded()
+    {
+        // no-op
+    }
+
 protected:
     SVGAnimatedStaticPropertyTearOff(SVGElement* contextElement, const QualifiedName& attributeName, AnimatedPropertyType animatedPropertyType, PropertyType& property)
         : SVGAnimatedProperty(contextElement, attributeName, animatedPropertyType)