Bug 84657

Summary: Crash or assertion failure (m_isAnimating) when adding another instance of a SMIL animated element while the animation is running
Product: WebKit Reporter: Tim Horton <thorton>
Component: SVGAssignee: Philip Rogers <pdr>
Status: RESOLVED FIXED    
Severity: Normal CC: fmalita, pdr, schenney, tasak, webkit-bug-importer, webkit.review.bot, zimmermann
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 99678    
Attachments:
Description Flags
repro
none
Allow lazy initialization of SVG XML animated properties. none

Tim Horton
Reported 2012-04-23 17:16:04 PDT
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()
Attachments
repro (1.01 KB, image/svg+xml)
2012-04-23 17:16 PDT, Tim Horton 		no flags
Details
Allow lazy initialization of SVG XML animated properties. (6.30 KB, patch)
2012-10-17 00:05 PDT, Philip Rogers 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Tim Horton
Comment 1 2012-04-23 17:33:04 PDT
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
Comment 2 2012-04-23 17:36:25 PDT
<rdar://problem/11305318>
Philip Rogers
Comment 3 2012-05-28 16:41:46 PDT
I hit this working on a personal project recently :(. Are you currently working on this Tim?
Tim Horton
Comment 4 2012-05-28 16:42:51 PDT
(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
Comment 5 2012-09-23 21:42:36 PDT
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
Comment 6 2012-10-17 00:05:12 PDT
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
Comment 7 2012-10-17 00:10:37 PDT
Philip, did you mean to cq+ this?
Philip Rogers
Comment 8 2012-10-17 00:20:43 PDT
(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 9 2012-10-17 12:06:08 PDT
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
Comment 10 2012-10-17 12:49:46 PDT
(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 11 2012-10-17 13:08:01 PDT
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
Comment 12 2012-10-17 13:08:05 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.