Assertion fires when animating a discrete property with values range and multiple...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 21 Sep 2019 01:24:45 +0000 (01:24 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 21 Sep 2019 01:24:45 +0000 (01:24 +0000)
https://bugs.webkit.org/show_bug.cgi?id=201926

Patch by Said Abou-Hallawa <sabouhallawa@apple.com> on 2019-09-20
Reviewed by Darin Adler.

Source/WebCore:

The first animator of a property is considered the result element. The
other animators will be just contributers to the first animator. For the
first animator and in SVGSMILElement::progress(), we call resetAnimatedType()
which creates m_animator in SVGAnimateElementBase::animator(). But for
the other animators we do not call resetAnimatedType(). So their m_animator
will stay null until they are used for the first time.

If SVGAnimationElement::startedActiveInterval() calls calculateToAtEndOfDurationValue()
for a discrete property this will have no effect and the call should be
ignored. So SVGAnimateElementBase::calculateToAtEndOfDurationValue()
should bail out early if isDiscreteAnimator() is true.

The bug is isDiscreteAnimator() will return false if the m_animator is
null even if the animated property is discrete, e.g. SVGAnimatedString.
The fix is to make isDiscreteAnimator() ensure m_animator is created.

Unrelated change:
Make most of the protected methods of SVGAnimateElementBase be private.

Test: svg/animations/multiple-discrete-values-animate.svg

* svg/SVGAnimateElementBase.cpp:
(WebCore::SVGAnimateElementBase::calculateFromAndByValues):
(WebCore::SVGAnimateElementBase::calculateToAtEndOfDurationValue):

LayoutTests:

Animate a discrete property, such as SVGAnimatedString. There should be
multiple animators and the range of animation has to be set by the 'values'
attribute.

* svg/animations/multiple-discrete-values-animate-expected.txt: Added.
* svg/animations/multiple-discrete-values-animate.svg: Added.

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

LayoutTests/ChangeLog
LayoutTests/svg/animations/multiple-discrete-values-animate-expected.txt [new file with mode: 0644]
LayoutTests/svg/animations/multiple-discrete-values-animate.svg [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/svg/SVGAnimateElementBase.cpp
Source/WebCore/svg/SVGAnimateElementBase.h

index 89008cd..154e990 100644 (file)
@@ -1,3 +1,17 @@
+2019-09-20  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Assertion fires when animating a discrete property with values range and multiple animators
+        https://bugs.webkit.org/show_bug.cgi?id=201926
+
+        Reviewed by Darin Adler.
+
+        Animate a discrete property, such as SVGAnimatedString. There should be
+        multiple animators and the range of animation has to be set by the 'values'
+        attribute.
+
+        * svg/animations/multiple-discrete-values-animate-expected.txt: Added.
+        * svg/animations/multiple-discrete-values-animate.svg: Added.
+
 2019-09-20  Chris Dumez  <cdumez@apple.com>
 
         REGRESSION (iOS 13): rAF stops firing when navigating away cross-origin and then back
diff --git a/LayoutTests/svg/animations/multiple-discrete-values-animate-expected.txt b/LayoutTests/svg/animations/multiple-discrete-values-animate-expected.txt
new file mode 100644 (file)
index 0000000..d22be3b
--- /dev/null
@@ -0,0 +1 @@
+Passes if we did not debug assert.
diff --git a/LayoutTests/svg/animations/multiple-discrete-values-animate.svg b/LayoutTests/svg/animations/multiple-discrete-values-animate.svg
new file mode 100644 (file)
index 0000000..30abf42
--- /dev/null
@@ -0,0 +1,18 @@
+<svg xmlns="http://www.w3.org/2000/svg">
+    <g>
+        <text>Passes if we did not debug assert.</text>
+        <animate attributeName="class" dur="20ms" values="box; circle" onbegin="animationStarted()"/>
+        <animate attributeName="class" dur="20ms" values="box; circle"/>
+    </g>
+    <script>
+        if (window.testRunner) {
+            testRunner.dumpAsText();
+            testRunner.waitUntilDone();
+        }
+
+        function animationStarted() {
+            if (window.testRunner)
+                testRunner.notifyDone();
+        }
+    </script>
+</svg>
index fa25042..37343a5 100644 (file)
@@ -1,3 +1,35 @@
+2019-09-20  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Assertion fires when animating a discrete property with values range and multiple animators
+        https://bugs.webkit.org/show_bug.cgi?id=201926
+
+        Reviewed by Darin Adler.
+
+        The first animator of a property is considered the result element. The
+        other animators will be just contributers to the first animator. For the
+        first animator and in SVGSMILElement::progress(), we call resetAnimatedType()
+        which creates m_animator in SVGAnimateElementBase::animator(). But for 
+        the other animators we do not call resetAnimatedType(). So their m_animator
+        will stay null until they are used for the first time.
+
+        If SVGAnimationElement::startedActiveInterval() calls calculateToAtEndOfDurationValue()
+        for a discrete property this will have no effect and the call should be
+        ignored. So SVGAnimateElementBase::calculateToAtEndOfDurationValue()
+        should bail out early if isDiscreteAnimator() is true.
+
+        The bug is isDiscreteAnimator() will return false if the m_animator is 
+        null even if the animated property is discrete, e.g. SVGAnimatedString.
+        The fix is to make isDiscreteAnimator() ensure m_animator is created.
+
+        Unrelated change:
+        Make most of the protected methods of SVGAnimateElementBase be private.
+
+        Test: svg/animations/multiple-discrete-values-animate.svg
+
+        * svg/SVGAnimateElementBase.cpp:
+        (WebCore::SVGAnimateElementBase::calculateFromAndByValues):
+        (WebCore::SVGAnimateElementBase::calculateToAtEndOfDurationValue):
+
 2019-09-20  Keith Rollin  <krollin@apple.com>
 
         Remove dead code for a specific macOS and iOS SDK
index a103ad8..6130cd9 100644 (file)
@@ -75,7 +75,11 @@ bool SVGAnimateElementBase::hasInvalidCSSAttributeType() const
 
 bool SVGAnimateElementBase::isDiscreteAnimator() const
 {
-    return hasValidAttributeType() && animatorIfExists() && animatorIfExists()->isDiscrete();
+    if (!hasValidAttributeType())
+        return false;
+
+    auto* animator = this->animator();
+    return animator && animator->isDiscrete();
 }
 
 void SVGAnimateElementBase::setTargetElement(SVGElement* target)
index 50a82c0..dac6c42 100644 (file)
@@ -38,11 +38,13 @@ public:
 protected:
     SVGAnimateElementBase(const QualifiedName&, Document&);
 
+    bool hasValidAttributeType() const override;
+    virtual String animateRangeString(const String& string) const { return string; }
+
+private:
     SVGAttributeAnimator* animator() const;
     SVGAttributeAnimator* animatorIfExists() const { return m_animator.get(); }
 
-    bool hasValidAttributeType() const override;
-
     void setTargetElement(SVGElement*) override;
     void setAttributeName(const QualifiedName&) override;
     void resetAnimation() override;
@@ -57,9 +59,6 @@ protected:
     void clearAnimatedType(SVGElement* targetElement) override;
     Optional<float> calculateDistance(const String& fromString, const String& toString) override;
 
-    virtual String animateRangeString(const String& string) const { return string; }
-
-private:
     bool hasInvalidCSSAttributeType() const;
 
     mutable std::unique_ptr<SVGAttributeAnimator> m_animator;