Bug 156549 – Calling SVGAnimatedPropertyTearOff::animationEnded() will crash if the SVG property is not animating

• Component: SVG
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

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)

[Said Abou-Hallawa]

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]

Created attachment 276347 [details]
Patch

[Darin Adler]

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]

rdar://problem/25709968

[Said Abou-Hallawa]

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 on attachment 276347 [details]
Patch

Clearing flags on attachment: 276347

Committed r199598: <http://trac.webkit.org/changeset/199598>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.