WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 276347
[details]
Patch
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
rdar://problem/25709968
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.
Top of Page
Format For Printing
XML
Clone This Bug