Bug 143994

Summary: SVGAnimateElementBase::calculateAnimatedValue() asserts when reinserting an SVG animating element within the same animation limits
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: SVGAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, graouts, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Test case (will crash)
none
Patch
none
Patch
none
transform test case (will crash)
none
Patch
none
Patch none

Description Said Abou-Hallawa 2015-04-21 09:10:20 PDT
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);
Comment 1 Said Abou-Hallawa 2015-04-21 09:50:26 PDT
Created attachment 251236 [details]
Patch
Comment 2 Said Abou-Hallawa 2015-04-21 10:17:19 PDT
Created attachment 251238 [details]
Patch
Comment 3 Said Abou-Hallawa 2015-04-21 11:45:27 PDT
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
Comment 4 Said Abou-Hallawa 2015-04-21 12:09:09 PDT
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
Comment 5 Said Abou-Hallawa 2015-04-21 12:26:58 PDT
Created attachment 251255 [details]
Patch
Comment 6 Said Abou-Hallawa 2015-04-21 12:33:29 PDT
<rdar://problem/18921450>
Comment 7 Simon Fraser (smfr) 2015-04-21 15:05:07 PDT
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.
Comment 8 Said Abou-Hallawa 2015-04-21 15:53:06 PDT
Created attachment 251271 [details]
Patch
Comment 9 Said Abou-Hallawa 2015-04-21 15:58:53 PDT
(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.
Comment 10 WebKit Commit Bot 2015-04-21 17:15:03 PDT
Comment on attachment 251271 [details]
Patch

Clearing flags on attachment: 251271

Committed r183085: <http://trac.webkit.org/changeset/183085>
Comment 11 WebKit Commit Bot 2015-04-21 17:15:06 PDT
All reviewed patches have been landed.  Closing bug.