Bug 143994 – SVGAnimateElementBase::calculateAnimatedValue() asserts when reinserting an SVG animating element within the same animation limits

• Component: SVG
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

Created attachment 251234 [details]
Test case (will crash)

Open the attached file.

Result: WebKit crashes with the following call stack:

WebCore::SVGAnimateElementBase::calculateAnimatedValue
WebCore::SVGAnimationElement::updateAnimation
WebCore::SVGSMILElement::progress
WebCore::SMILTimeContainer::updateAnimations
WebCore::SMILTimeContainer::timerFired

Notes: The crash happens when removing an animating SVG element and inserting it back while animating within the same animation limits. The reason for the crash is when removing an animating element from the SVG document, we call SVGAnimateElementBase::resetAnimatedPropertyType() which sets SVGAnimateElementBase::m_fromType and SVGAnimateElementBase::m_toType to nullptr. When the element is inserted back to the SVG document, SVGAnimationElement::updateAnimation() is called to get the new animated value. Before doing that we check if the animation limits have changed or not. But since in this case, the limits are not changed, we do not call SVGAnimateElementBase::calculateFromAndToValues() which is supposed to call SVGAnimatedTypeAnimator::calculateFromAndToValues(). And this later function is supposed to set valid values to SVGAnimateElementBase::m_fromType and SVGAnimateElementBase::m_toType. But since this does not happen, we end up calling SSVGAnimateElementBase::calculateAnimatedValue() which asserts

ASSERT(m_fromType);
ASSERT(m_fromType->type() == m_animatedPropertyType);
ASSERT(m_toType);

[Said Abou-Hallawa]

Created attachment 251236 [details]
Patch

[Said Abou-Hallawa]

Created attachment 251238 [details]
Patch

[Said Abou-Hallawa]

This is the crash trace we get when opening the attached test case. The assert in SVGAnimateElementBase::calculateAnimatedValue() should happen before this crash in debug build.

SVGAnimatedLengthAnimator::calculateAnimatedValue(float, unsigned int, SVGAnimatedType*, WebCore::SVGAnimatedType*, WebCore::SVGAnimatedType*, WebCore::SVGAnimatedType*) + 60
WebCore::SVGAnimationElement::updateAnimation(float, unsigned int, WebCore::SVGSMILElement*) + 747
WebCore::SVGSMILElement::progress(WebCore::SMILTime, WebCore::SVGSMILElement*, bool) + 607
WebCore::SMILTimeContainer::updateAnimations(WebCore::SMILTime, bool) + 426

[Said Abou-Hallawa]

Created attachment 251250 [details]
transform test case (will crash)

This is another test case with a different crash trace. However it happens because of the same reason mentioned above. In the debug build SVGAnimateElementBase::calculateAnimatedValue() asserts before this crash:

WebCore::SVGAnimatedTransformListAnimator::calculateAnimatedValue(float, unsigned int, WebCore::SVGAnimatedType*, WebCore::SVGAnimatedType*, WebCore::SVGAnimatedType*, WebCore::SVGAnimatedType*) + 34
WebCore::SVGAnimationElement::updateAnimation(float, unsigned int, WebCore::SVGSMILElement*) + 747
WebCore::SVGSMILElement::progress(WebCore::SMILTime, WebCore::SVGSMILElement*, bool) + 607
WebCore::SMILTimeContainer::updateAnimations(WebCore::SMILTime, bool) + 509

[Said Abou-Hallawa]

Created attachment 251255 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/18921450>

[Simon Fraser (smfr)]

Comment on attachment 251255 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251255&action=review

> LayoutTests/svg/animations/crash-reinsert-animate-length-same-limits.svg:19
> +      }, 200);

Try to find a way to reduce or eliminate this timeout.

> LayoutTests/svg/animations/crash-reinsert-animate-transform-same-limits.svg:19
> +      }, 200);

Try to find a way to reduce or eliminate this timeout.

[Said Abou-Hallawa]

Created attachment 251271 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #7)
> Comment on attachment 251255 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251255&action=review
> 
> > LayoutTests/svg/animations/crash-reinsert-animate-length-same-limits.svg:19
> > +      }, 200);
> 
> Try to find a way to reduce or eliminate this timeout.
> 
> > LayoutTests/svg/animations/crash-reinsert-animate-transform-same-limits.svg:19
> > +      }, 200);
> 
> Try to find a way to reduce or eliminate this timeout.

The 200ms is now replaced by 0. The setTimeout(...,0); has to stay to ensure that the animation timer will fire at least once after reinserting the animating element and before the test ends.  I also confirmed that WebKit crashes with the modified tests.

[WebKit Commit Bot]

Comment on attachment 251271 [details]
Patch

Clearing flags on attachment: 251271

Committed r183085: <http://trac.webkit.org/changeset/183085>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.