RESOLVED FIXED 156549
Calling SVGAnimatedPropertyTearOff::animationEnded() will crash if the SVG property is not animating
https://bugs.webkit.org/show_bug.cgi?id=156549
Summary Calling SVGAnimatedPropertyTearOff::animationEnded() will crash if the SVG pr...
Said Abou-Hallawa
Reported 2016-04-13 12:36:00 PDT
There is no repro steps or a test case for this crash but there is this call stack: 0 WebCore 0x0000000186b76e9c void WebCore::SVGAnimatedTypeAnimator::executeAction<WebCore::SVGAnimatedPropertyTearOff<WebCore::SVGLength> >(WebCore::SVGAnimatedTypeAnimator::AnimationAction, WTF::Vector<WebCore::SVGElementAnimatedProperties, 0ul, WTF::CrashOnOverflow, 16ul> const&, unsigned int, WebCore::SVGAnimatedPropertyTearOff<WebCore::SVGLength>::ContentType*) + 132 (SVGAnimatedPropertyTearOff.h:93) 1 WebCore 0x0000000186b76e5c void WebCore::SVGAnimatedTypeAnimator::executeAction<WebCore::SVGAnimatedPropertyTearOff<WebCore::SVGLength> >(WebCore::SVGAnimatedTypeAnimator::AnimationAction, WTF::Vector<WebCore::SVGElementAnimatedProperties, 0ul, WTF::CrashOnOverflow, 16ul> const&, unsigned int, WebCore::SVGAnimatedPropertyTearOff<WebCore::SVGLength>::ContentType*) + 68 (SVGAnimatedTypeAnimator.h:192) 2 WebCore 0x0000000186b8096c WebCore::SVGAnimateElementBase::clearAnimatedType(WebCore::SVGElement*) + 728 (SVGAnimateElementBase.cpp:326) 3 WebCore 0x0000000186c00068 WebCore::SVGSMILElement::setTargetElement(WebCore::SVGElement*) + 120 (SVGSMILElement.cpp:599) 4 WebCore 0x0000000186b85b9c WebCore::SVGAnimationElement::setTargetElement(WebCore::SVGElement*) + 28 (SVGAnimationElement.cpp:685) 5 WebCore 0x0000000186b810f4 WebCore::SVGAnimateElementBase::setTargetElement(WebCore::SVGElement*) + 20 (SVGAnimateElementBase.cpp:420) 6 WebCore 0x0000000186b8ff08 WebCore::SVGDocumentExtensions::clearTargetDependencies(WebCore::SVGElement&) + 216 (SVGElement.h:155) 7 WebCore 0x0000000186b92814 WebCore::SVGElement::removedFrom(WebCore::ContainerNode&) + 92 (SVGElement.cpp:395) 8 WebCore 0x0000000186145e8c void WebCore::Private::addChildNodesToDeletionQueue<WebCore::Node, WebCore::ContainerNode>(WebCore::Node*&, WebCore::Node*&, WebCore::ContainerNode&) + 208 (ContainerNodeAlgorithms.h:233) 9 WebCore 0x0000000185f314f8 WebCore::ContainerNode::removeDetachedChildren() + 132 (ContainerNodeAlgorithms.h:103) 10 WebCore 0x0000000186221874 WebCore::Document::removedLastRef() + 336 (Document.cpp:680) 11 JavaScriptCore 0x0000000185b3abc8 0x00000001856ac000 + 4778952 12 JavaScriptCore 0x00000001859eb230 JSC::IncrementalSweeper::sweepNextBlock() + 104 (IncrementalSweeper.cpp:91) 13 JavaScriptCore 0x00000001856c1dd4 JSC::IncrementalSweeper::doSweep(double) + 40 (IncrementalSweeper.cpp:69) 14 JavaScriptCore 0x00000001856bd550 JSC::HeapTimer::timerDidFire(__CFRunLoopTimer*, void*) + 220 (HeapTimer.cpp:100) 15 CoreFoundation 0x0000000181fe5834 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 28 (CFRunLoop.c:1628) 16 CoreFoundation 0x0000000181fe54d8 __CFRunLoopDoTimer + 884 (CFRunLoop.c:2167) 17 CoreFoundation 0x0000000181fe2bec __CFRunLoopRun + 1520 (CFRunLoop.c:2306) 18 CoreFoundation 0x0000000181f0ce80 CFRunLoopRunSpecific + 384 (CFRunLoop.c:2814) 19 Foundation 0x000000018291ccfc -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 308 (NSRunLoop.m:366) 20 Foundation 0x0000000182972030 -[NSRunLoop(NSRunLoop) run] + 88 (NSRunLoop.m:388) 21 libxpc.dylib 0x0000000181cd0c64 _xpc_objc_main + 660 (main.m:181) 22 libxpc.dylib 0x0000000181cd29dc xpc_main + 200 (init.c:1439) 23 com.apple.WebKit.WebContent 0x00000001000e3924 main + 56 (XPCServiceMain.mm:89) 24 libdyld.dylib 0x0000000181aaa8b8 start + 4 (start_glue.s:78)
Attachments
Patch (1.43 KB, patch)
2016-04-13 12:57 PDT, Said Abou-Hallawa
no flags
Said Abou-Hallawa
Comment 1 2016-04-13 12:47:09 PDT
Before https://trac.webkit.org/changeset/197967, SVGAnimatedPropertyTearOff::animationEnded() could have been called multiple times if the SVGAnimatedPropertyTearOff::animationStarted() is called once. We were calling animVal() which ensures m_animVal is created correctly. We were using m_animVal as the animated property during the animation. Nothing after that sets m_animVal to nullptr. After this change, this is not true. SVGAnimatedPropertyTearOff::animationStarted() sets m_animatedProperty = animVal(). m_animatedProperty is the property to animate and to work with during animation. But SVGAnimatedPropertyTearOff::animationEnded() sets m_animatedProperty = nullptr. So if SVGAnimatedPropertyTearOff::animationEnded() is called after that a crash will happen. This crash has been seen before this change so it is not a regression. But this change exposed the bug significantly. An speculative fix is: In SVGAnimatedTypeAnimator::executeAction() we need to check if (property->isAnimating()) before calling property->animationEnded() if the action is StopAnimationAction.
Said Abou-Hallawa
Comment 2 2016-04-13 12:57:43 PDT
Darin Adler
Comment 3 2016-04-13 13:11:58 PDT
Comment on attachment 276347 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276347&action=review Seems OK; you said you were looking at how to create a test and that does seem critical to understanding if this is a complete and effective fix. > Source/WebCore/svg/SVGAnimatedTypeAnimator.h:205 > - property->animationEnded(); > + if (property->isAnimating()) > + property->animationEnded(); Is there a reason this is preferable over having animationEnded guard itself inside the function?
Jon Lee
Comment 4 2016-04-13 13:26:42 PDT
Said Abou-Hallawa
Comment 5 2016-04-13 17:25:22 PDT
Comment on attachment 276347 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276347&action=review >> Source/WebCore/svg/SVGAnimatedTypeAnimator.h:205 >> + property->animationEnded(); > > Is there a reason this is preferable over having animationEnded guard itself inside the function? I think two reasons: 1. I tried to follow the same pattern we do for StartAnimationAction in the same function: case StartAnimationAction: ASSERT(type); if (!property->isAnimating()) property->animationStarted(type); break; 2. I tried to make the change small. animationEnded() is a template function and it is overriden by SVGAnimatedListPropertyTearOff SVGAnimatedPathSegListPropertyTearOff SVGAnimatedPropertyTearOff So if I have animationEnded guard itself from inside the function, I will have to change three places. And to be consistent, I will change the corresponding animationStarted() in the three template classes as well. So instead of having six changes, I made a single change. Also I confirmed that SVGAnimatedTypeAnimator::executeAction() is the only place which calls animationStarted() and animationEnded().
WebKit Commit Bot
Comment 6 2016-04-15 11:24:22 PDT
Comment on attachment 276347 [details] Patch Clearing flags on attachment: 276347 Committed r199598: <http://trac.webkit.org/changeset/199598>
WebKit Commit Bot
Comment 7 2016-04-15 11:24:27 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.