Bug 84657 – Crash or assertion failure (m_isAnimating) when adding another instance of a SMIL animated element while the animation is running

• Component: SVG
• Status: RESOLVED FIXED

[Tim Horton]

Created attachment 138466 [details]
repro

I found this while working on https://bugs.webkit.org/show_bug.cgi?id=83856, but eventually discovered it happens on ToT too.

See the attached test case. Sometimes I have to refresh it once or twice before seeing the assertion:

ASSERTION FAILED: m_isAnimating
/Volumes/Data/Shared/src/WebKit/OpenSource/Source/WebCore/svg/properties/SVGAnimatedPropertyTearOff.h(92) : void WebCore::SVGAnimatedPropertyTearOff<WebCore::SVGLength>::animValWillChange()
1   0x10a8a8dc7 void WebCore::SVGAnimatedTypeAnimator::executeAction<WebCore::SVGAnimatedPropertyTearOff<WebCore::SVGLength> >(WebCore::SVGAnimatedTypeAnimator::AnimationAction, WTF::Vector<WebCore::SVGAnimatedProperty*, 0ul> const&, unsigned int, WebCore::SVGAnimatedPropertyTearOff<WebCore::SVGLength>::ContentType*)
2   0x10a8a8a69 void WebCore::SVGAnimatedTypeAnimator::animValWillChangeForType<WebCore::SVGAnimatedPropertyTearOff<WebCore::SVGLength> >(WTF::Vector<WebCore::SVGAnimatedProperty*, 0ul> const&)
3   0x10a8a7c38 WebCore::SVGAnimatedLengthAnimator::animValWillChange(WTF::Vector<WebCore::SVGAnimatedProperty*, 0ul> const&)
4   0x10a8c9e20 WebCore::SVGAnimateElement::calculateAnimatedValue(float, unsigned int, WebCore::SVGSMILElement*)
5   0x10a8d3098 WebCore::SVGAnimationElement::updateAnimation(float, unsigned int, WebCore::SVGSMILElement*)
6   0x10a9af09f WebCore::SVGSMILElement::progress(WebCore::SMILTime, WebCore::SVGSMILElement*)
7   0x10a824d95 WebCore::SMILTimeContainer::updateAnimations(WebCore::SMILTime)
8   0x10a8244a7 WebCore::SMILTimeContainer::timerFired(WebCore::Timer<WebCore::SMILTimeContainer>*)
9   0x10a82f133 WebCore::Timer<WebCore::SMILTimeContainer>::fired()

[Tim Horton]

Oh, hey, it crashes instead if it's a release build:

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000028

0   com.apple.WebCore             	0x00007fff9069b864 void WebCore::SVGAnimatedTypeAnimator::executeAction<WebCore::SVGAnimatedPropertyTearOff<WebCore::SVGLength> >(WebCore::SVGAnimatedTypeAnimator::AnimationAction, WTF::Vector<WebCore::SVGAnimatedProperty*, 0ul> const&, unsigned int, WebCore::SVGAnimatedPropertyTearOff<WebCore::SVGLength>::ContentType*) + 244
1   com.apple.WebCore             	0x00007fff906a420d WebCore::SVGAnimateElement::targetElementWillChange(WebCore::SVGElement*, WebCore::SVGElement*) + 141
2   com.apple.WebCore             	0x00007fff9070191b WebCore::SVGSMILElement::resetTargetElement() + 27
3   com.apple.WebCore             	0x00007fff8fda0718 WebCore::SVGDocumentExtensions::removeAllAnimationElementsFromTarget(WebCore::SVGElement*) + 104
4   com.apple.WebCore             	0x00007fff8fdae22d WebCore::SVGElement::removedFromDocument() + 29
5   com.apple.WebCore             	0x00007fff8fdae1cf WebCore::SVGStyledElement::removedFromDocument() + 31
6   com.apple.WebCore             	0x00007fff90129083 void WebCore::Private::addChildNodesToDeletionQueue<WebCore::Node, WebCore::ContainerNode>(WebCore::Node*&, WebCore::Node*&, WebCore::ContainerNode*) + 83
7   com.apple.WebCore             	0x00007fff8fc0aee8 void WebCore::removeAllChildrenInContainer<WebCore::Node, WebCore::ContainerNode>(WebCore::ContainerNode*) + 136
8   com.apple.WebCore             	0x00007fff8fc0ac21 WebCore::Document::removedLastRef() + 449
9   com.apple.WebCore             	0x00007fff904b0daf WebCore::JSNode::destroy(JSC::JSCell*) + 47
10  com.apple.JavaScriptCore      	0x00007fff89b215fe JSC::MarkedBlock::FreeCell* JSC::MarkedBlock::sweepHelper<true>(JSC::MarkedBlock::SweepMode) + 350
11  com.apple.JavaScriptCore      	0x00007fff89ace6cb JSC::Heap::sweep() + 107
12  com.apple.JavaScriptCore      	0x00007fff89acef57 JSC::Heap::collect(JSC::Heap::SweepToggle) + 151

[Radar WebKit Bug Importer]

<rdar://problem/11305318>

[Philip Rogers]

I hit this working on a personal project recently :(. Are you currently working on this Tim?

[Tim Horton]

(In reply to comment #3)
> I hit this working on a personal project recently :(. Are you currently working on this Tim?

Not at the moment.

[Philip Rogers]

Even simpler repro:
<svg>
  <rect id="rect" x="100" y="100" width="100" height="100" fill="green">
    <animate attributeName="x" from="100" to="200" dur="3s" begin="0s" />
  </rect>
  <use xlink:href="#rect" x="200" y="200">
    <animate attributeName="xlink:href" to="#notrect" begin="1s" dur="3s"/>
  </use>
</svg>

[Philip Rogers]

Created attachment 169101 [details]
Allow lazy initialization of SVG XML animated properties.

Finally getting a patch up for this!

I explored tracking the animating properties in the animator so that we could catch this case once instead of checking for it on every call to resetAnimValToBaseVal. Unfortunately the checks required ended up polluting the instance code which would affect performance outside animations. I also explored the naive approach of invalidating the entire tree when we dynamically add instances of animating elements (see comment in SVGElementInstance::invalidateAllInstancesOfElement about this) but, again, that ended up being terrible for performance.

[Tim Horton]

Philip, did you mean to cq+ this?

[Philip Rogers]

(In reply to comment #7)
> Philip, did you mean to cq+ this?

I thought the queue waited on r+ but I may be mistaken. I've removed cq+ just in case.

[Tim Horton]

Comment on attachment 169101 [details]
Allow lazy initialization of SVG XML animated properties.

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

> LayoutTests/svg/animations/use-while-animating-crash.html:23
> +            }, 20); // This needs to be at least 20 to ensure an animation cycle runs.

Do we not have a better way to do this? Events we can watch, or something? I cringe every time I see a non-0 setTimeout in a layout test.

[Philip Rogers]

(In reply to comment #9)
> (From update of attachment 169101 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169101&action=review
> 
> > LayoutTests/svg/animations/use-while-animating-crash.html:23
> > +            }, 20); // This needs to be at least 20 to ensure an animation cycle runs.
> 
> Do we not have a better way to do this? Events we can watch, or something? I cringe every time I see a non-0 setTimeout in a layout test.

Thanks for the review. It's unfortunate but every way to start an animation is guarded by a timer so I don't think we can avoid this.

[WebKit Review Bot]

Comment on attachment 169101 [details]
Allow lazy initialization of SVG XML animated properties.

Clearing flags on attachment: 169101

Committed r131631: <http://trac.webkit.org/changeset/131631>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.