Crash when appending an SVG <use> element dynamically which has animated SVG <path...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 8 Jul 2015 23:56:58 +0000 (23:56 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 8 Jul 2015 23:56:58 +0000 (23:56 +0000)
https://bugs.webkit.org/show_bug.cgi?id=146690
<rdar://problem/20790376>

Patch by Said Abou-Hallawa <sabouhallawa@apple.com> on 2015-07-08
Reviewed by Dean Jackson.

Source/WebCore:

Test: svg/animations/insert-animate-use-path-while-animation.svg

The crashing call stack shows that
SVGAnimatedListPropertyTearOff<SVGPathSegList>::m_animVal is null when
trying to access it in synchronizeWrappersIfNeeded(). This happens because
animationStarted() was not called for this animatedType.

SVGAnimateElementBase::resetAnimatedType() calls
SVGAnimatedPathAnimator::startAnimValAnimation() at the beginning of the
animation. For the target element and all its instances, this function calls
SVGAnimatedPathSegListPropertyTearOff::animationStarted() which calls
SVGAnimatedListPropertyTearOff<SVGPathSegList>::animationStarted(). This
last function allocates the member m_animVal when calling
SVGAnimatedListPropertyTearOff<SVGPathSegList>::animVal().

When adding a new instance of the same animating target element,
SVGAnimateElementBase::resetAnimatedType() just keeps calling
SVGAnimatedPathAnimator::animValDidChange() for all the instances of the
targetElement without ensuring that all of them have started their
animations.

The fix is to make SVGAnimatedPathAnimator::resetAnimValToBaseVal() ensure
that animationStarted() is called for the targetElement and all its instances.

* svg/SVGAnimatedPath.cpp:
(WebCore::SVGAnimatedPathAnimator::startAnimValAnimation): Move resetting
the animation value and starting the animatedTypes code to a new overriding
function which is named resetAnimValToBaseVal().

(WebCore::SVGAnimatedPathAnimator::resetAnimValToBaseVal): Call the overriding
function which calls buildSVGPathByteStreamFromSVGPathSegList() as before
and ensure that all the animatedTypes have started their animations.

* svg/SVGAnimatedPath.h:

LayoutTests:

When adding dynamically a new <use> element which references an animated
SVG path after the animation starts, ensure that WebKit is not crashing.

* svg/animations/insert-animate-use-path-while-animation-expected.txt: Added.
* svg/animations/insert-animate-use-path-while-animation.svg: Added.

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

LayoutTests/ChangeLog
LayoutTests/svg/animations/insert-animate-use-path-while-animation-expected.txt [new file with mode: 0644]
LayoutTests/svg/animations/insert-animate-use-path-while-animation.svg [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/svg/SVGAnimatedPath.cpp
Source/WebCore/svg/SVGAnimatedPath.h

index efed078..c6bb678 100644 (file)
@@ -1,3 +1,17 @@
+2015-07-08  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Crash when appending an SVG <use> element dynamically which has animated SVG <path> element
+        https://bugs.webkit.org/show_bug.cgi?id=146690
+        <rdar://problem/20790376>
+
+        Reviewed by Dean Jackson.
+
+        When adding dynamically a new <use> element which references an animated
+        SVG path after the animation starts, ensure that WebKit is not crashing.
+
+        * svg/animations/insert-animate-use-path-while-animation-expected.txt: Added.
+        * svg/animations/insert-animate-use-path-while-animation.svg: Added.
+
 2015-07-08  David Kilzer  <ddkilzer@apple.com>
 
         http/tests/xmlviewer/dumpAsText/svg.xml contains a typo that breaks the test with libxml2 v2.9.2
diff --git a/LayoutTests/svg/animations/insert-animate-use-path-while-animation-expected.txt b/LayoutTests/svg/animations/insert-animate-use-path-while-animation-expected.txt
new file mode 100644 (file)
index 0000000..863339f
--- /dev/null
@@ -0,0 +1 @@
+Passed
diff --git a/LayoutTests/svg/animations/insert-animate-use-path-while-animation.svg b/LayoutTests/svg/animations/insert-animate-use-path-while-animation.svg
new file mode 100644 (file)
index 0000000..47c800c
--- /dev/null
@@ -0,0 +1,41 @@
+<svg width="600" height="400" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
+  <path id="animatedRect" fill="green">
+    <animate
+      attributeType="XML"
+      attributeName="d"
+      from="M20 60 L20 110 L120 110 L120 60 Z"
+      to="M45 35 L45 135 L95 135 L95 35 Z"
+      dur="1s"
+      repeatCount="indefinite"/>
+  </path>
+  <script><![CDATA[
+    if (window.testRunner) {
+      testRunner.dumpAsText();
+      testRunner.waitUntilDone();
+    }
+
+    // Wait until the only instance of 'animatedRect' starts animation.
+    setTimeout(function () {
+      var svgns = 'http://www.w3.org/2000/svg';
+      var xlinkns = 'http://www.w3.org/1999/xlink';
+
+      for (var i = 1; i <= 4; i++) {
+        var shape = document.createElementNS(svgns, "use");
+        shape.setAttributeNS(xlinkns, 'href', '#animatedRect');
+        shape.setAttributeNS(null, 'transform', 'translate(' + 100 * i + ', 0)');
+        document.documentElement.appendChild(shape);
+      }
+      
+      if (window.testRunner) {
+        // Wait until the next animation cycle starts to make sure
+        // all of the instances of 'animatedRect' are animating.
+        setTimeout(function () {
+          var text = document.createElementNS(svgns, "text");
+          text.appendChild(document.createTextNode("Passed"));
+          document.documentElement.appendChild(text);
+          testRunner.notifyDone();
+        }, 50);
+      }
+    }, 50);
+  ]]></script>
+</svg>
index a5ff4d9..22f6b1a 100644 (file)
@@ -1,3 +1,46 @@
+2015-07-08  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Crash when appending an SVG <use> element dynamically which has animated SVG <path> element
+        https://bugs.webkit.org/show_bug.cgi?id=146690
+        <rdar://problem/20790376>
+
+        Reviewed by Dean Jackson.
+
+        Test: svg/animations/insert-animate-use-path-while-animation.svg
+
+        The crashing call stack shows that
+        SVGAnimatedListPropertyTearOff<SVGPathSegList>::m_animVal is null when
+        trying to access it in synchronizeWrappersIfNeeded(). This happens because
+        animationStarted() was not called for this animatedType.
+        
+        SVGAnimateElementBase::resetAnimatedType() calls
+        SVGAnimatedPathAnimator::startAnimValAnimation() at the beginning of the
+        animation. For the target element and all its instances, this function calls
+        SVGAnimatedPathSegListPropertyTearOff::animationStarted() which calls
+        SVGAnimatedListPropertyTearOff<SVGPathSegList>::animationStarted(). This
+        last function allocates the member m_animVal when calling
+        SVGAnimatedListPropertyTearOff<SVGPathSegList>::animVal(). 
+        
+        When adding a new instance of the same animating target element, 
+        SVGAnimateElementBase::resetAnimatedType() just keeps calling
+        SVGAnimatedPathAnimator::animValDidChange() for all the instances of the
+        targetElement without ensuring that all of them have started their
+        animations.
+        
+        The fix is to make SVGAnimatedPathAnimator::resetAnimValToBaseVal() ensure
+        that animationStarted() is called for the targetElement and all its instances.
+
+        * svg/SVGAnimatedPath.cpp:
+        (WebCore::SVGAnimatedPathAnimator::startAnimValAnimation): Move resetting
+        the animation value and starting the animatedTypes code to a new overriding
+        function which is named resetAnimValToBaseVal().
+        
+        (WebCore::SVGAnimatedPathAnimator::resetAnimValToBaseVal): Call the overriding
+        function which calls buildSVGPathByteStreamFromSVGPathSegList() as before
+        and ensure that all the animatedTypes have started their animations.
+        
+        * svg/SVGAnimatedPath.h:
+
 2015-07-08  Brady Eidson  <beidson@apple.com>
 
         Move PingLoaders to the NetworkingProcess.
index 8097477..3831fc2 100644 (file)
@@ -41,23 +41,10 @@ std::unique_ptr<SVGAnimatedType> SVGAnimatedPathAnimator::constructFromString(co
 std::unique_ptr<SVGAnimatedType> SVGAnimatedPathAnimator::startAnimValAnimation(const SVGElementAnimatedPropertyList& animatedTypes)
 {
     ASSERT(animatedTypes.size() >= 1);
-    SVGAnimatedPathSegListPropertyTearOff* property = castAnimatedPropertyToActualType<SVGAnimatedPathSegListPropertyTearOff>(animatedTypes[0].properties[0].get());
-    const SVGPathSegList& baseValue = property->currentBaseValue();
 
     // Build initial path byte stream.
     auto byteStream = std::make_unique<SVGPathByteStream>();
-    buildSVGPathByteStreamFromSVGPathSegList(baseValue, byteStream.get(), UnalteredParsing);
-
-    Vector<RefPtr<SVGAnimatedPathSegListPropertyTearOff>> result;
-
-    for (auto& type : animatedTypes)
-        result.append(castAnimatedPropertyToActualType<SVGAnimatedPathSegListPropertyTearOff>(type.properties[0].get()));
-
-    SVGElement::InstanceUpdateBlocker blocker(*property->contextElement());
-
-    for (auto& segment : result)
-        segment->animationStarted(byteStream.get(), &baseValue);
-
+    resetAnimValToBaseVal(animatedTypes, byteStream.get());
     return SVGAnimatedType::createPath(WTF::move(byteStream));
 }
 
@@ -66,13 +53,35 @@ void SVGAnimatedPathAnimator::stopAnimValAnimation(const SVGElementAnimatedPrope
     stopAnimValAnimationForType<SVGAnimatedPathSegListPropertyTearOff>(animatedTypes);
 }
 
+void SVGAnimatedPathAnimator::resetAnimValToBaseVal(const SVGElementAnimatedPropertyList& animatedTypes, SVGPathByteStream* byteStream)
+{
+    SVGAnimatedPathSegListPropertyTearOff* property = castAnimatedPropertyToActualType<SVGAnimatedPathSegListPropertyTearOff>(animatedTypes[0].properties[0].get());
+    const SVGPathSegList& baseValue = property->currentBaseValue();
+
+    buildSVGPathByteStreamFromSVGPathSegList(baseValue, byteStream, UnalteredParsing);
+
+    Vector<RefPtr<SVGAnimatedPathSegListPropertyTearOff>> result;
+
+    for (auto& type : animatedTypes) {
+        auto* segment = castAnimatedPropertyToActualType<SVGAnimatedPathSegListPropertyTearOff>(type.properties[0].get());
+        if (segment->isAnimating())
+            continue;
+        result.append(segment);
+    }
+
+    if (!result.isEmpty()) {
+        SVGElement::InstanceUpdateBlocker blocker(*property->contextElement());
+        for (auto& segment : result)
+            segment->animationStarted(byteStream, &baseValue);
+    }
+}
+
 void SVGAnimatedPathAnimator::resetAnimValToBaseVal(const SVGElementAnimatedPropertyList& animatedTypes, SVGAnimatedType* type)
 {
     ASSERT(animatedTypes.size() >= 1);
     ASSERT(type);
     ASSERT(type->type() == m_type);
-    const SVGPathSegList& baseValue = castAnimatedPropertyToActualType<SVGAnimatedPathSegListPropertyTearOff>(animatedTypes[0].properties[0].get())->currentBaseValue();
-    buildSVGPathByteStreamFromSVGPathSegList(baseValue, type->path(), UnalteredParsing);
+    resetAnimValToBaseVal(animatedTypes, type->path());
 }
 
 void SVGAnimatedPathAnimator::animValWillChange(const SVGElementAnimatedPropertyList& animatedTypes)
index 4e11d56..2a64e83 100644 (file)
@@ -40,6 +40,9 @@ public:
     virtual void addAnimatedTypes(SVGAnimatedType*, SVGAnimatedType*) override;
     virtual void calculateAnimatedValue(float percentage, unsigned repeatCount, SVGAnimatedType*, SVGAnimatedType*, SVGAnimatedType*, SVGAnimatedType*) override;
     virtual float calculateDistance(const String& fromString, const String& toString) override;
+
+private:
+    void resetAnimValToBaseVal(const SVGElementAnimatedPropertyList&, SVGPathByteStream*);
 };
 
 } // namespace WebCore