156549 2016-04-13 12:36:00 -0700 Calling SVGAnimatedPropertyTearOff::animationEnded() will crash if the SVG property is not animating 2016-04-15 11:24:27 -0700 1 1 1 Unclassified WebKit SVG WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 sabouhallawa sabouhallawa commit-queue jonlee webkit-bug-importer zimmermann oldest_to_newest 1183494 0 sabouhallawa 2016-04-13 12:36:00 -0700 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) 1183503 1 sabouhallawa 2016-04-13 12:47:09 -0700 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. 1183509 2 276347 sabouhallawa 2016-04-13 12:57:43 -0700 Created attachment 276347 Patch 1183512 3 276347 darin 2016-04-13 13:11:58 -0700 Comment on attachment 276347 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? 1183515 4 jonlee 2016-04-13 13:26:42 -0700 rdar://problem/25709968 1183594 5 276347 sabouhallawa 2016-04-13 17:25:22 -0700 Comment on attachment 276347 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(). 1184141 6 276347 commit-queue 2016-04-15 11:24:22 -0700 Comment on attachment 276347 Patch Clearing flags on attachment: 276347 Committed r199598: <http://trac.webkit.org/changeset/199598> 1184142 7 commit-queue 2016-04-15 11:24:27 -0700 All reviewed patches have been landed. Closing bug. 276347 2016-04-13 12:57:43 -0700 2016-04-15 11:24:22 -0700 Patch bug-156549-20160413125754.patch text/plain 1468 sabouhallawa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE5OTUxMCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDE2LTA0LTEzICBTYWlkIEFi b3UtSGFsbGF3YSAgPHNhYm91aGFsbGF3YUBhcHBsZS5jb20+CisKKyAgICAgICAgQ2FsbGluZyBT VkdBbmltYXRlZFByb3BlcnR5VGVhck9mZjo6YW5pbWF0aW9uRW5kZWQoKSB3aWxsIGNyYXNoIGlm IHRoZSBTVkcgcHJvcGVydHkgaXMgbm90IGFuaW1hdGluZworICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTU2NTQ5CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQSBzcGVjdWxhdGl2ZSBmaXggZm9yIGEgY3Jhc2gg d2hpY2ggbWF5IGhhcHBlbiB3aGVuIGNhbGxpbmcgYW5pbWF0aW9uRW5kZWQoKQorICAgICAgICBv ZiBhbnkgU1ZHQW5pbWF0ZWRQcm9wZXJ0eSB3aGlsZSBpdCBpcyBub3QgYW5pbWF0aW5nLgorCisg ICAgICAgICogc3ZnL1NWR0FuaW1hdGVkVHlwZUFuaW1hdG9yLmg6CisgICAgICAgIChXZWJDb3Jl OjpTVkdBbmltYXRlZFR5cGVBbmltYXRvcjo6ZXhlY3V0ZUFjdGlvbik6CisKIDIwMTYtMDQtMTMg IERhcmluIEFkbGVyICA8ZGFyaW5AYXBwbGUuY29tPgogCiAgICAgICAgIFJlbW92ZSBVc2VQb2lu dGVyc0V2ZW5Gb3JOb25OdWxsYWJsZU9iamVjdEFyZ3VtZW50cyBmcm9tIERhdGFUcmFuc2ZlcgpJ bmRleDogU291cmNlL1dlYkNvcmUvc3ZnL1NWR0FuaW1hdGVkVHlwZUFuaW1hdG9yLmgKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gU291cmNlL1dlYkNvcmUvc3ZnL1NWR0FuaW1hdGVkVHlwZUFuaW1hdG9yLmgJKHJl dmlzaW9uIDE5OTMyOCkKKysrIFNvdXJjZS9XZWJDb3JlL3N2Zy9TVkdBbmltYXRlZFR5cGVBbmlt YXRvci5oCSh3b3JraW5nIGNvcHkpCkBAIC0yMDEsNyArMjAxLDggQEAgcHJpdmF0ZToKICAgICAg ICAgICAgICAgICBicmVhazsKICAgICAgICAgICAgIGNhc2UgU3RvcEFuaW1hdGlvbkFjdGlvbjoK ICAgICAgICAgICAgICAgICBBU1NFUlQoIXR5cGUpOwotICAgICAgICAgICAgICAgIHByb3BlcnR5 LT5hbmltYXRpb25FbmRlZCgpOworICAgICAgICAgICAgICAgIGlmIChwcm9wZXJ0eS0+aXNBbmlt YXRpbmcoKSkKKyAgICAgICAgICAgICAgICAgICAgcHJvcGVydHktPmFuaW1hdGlvbkVuZGVkKCk7 CiAgICAgICAgICAgICAgICAgYnJlYWs7CiAgICAgICAgICAgICBjYXNlIEFuaW1WYWxXaWxsQ2hh bmdlQWN0aW9uOgogICAgICAgICAgICAgICAgIEFTU0VSVCghdHlwZSk7Cg==