| Summary: | Calling SVGAnimatedPropertyTearOff::animationEnded() will crash if the SVG property is not animating | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||
| Component: | SVG | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | commit-queue, jonlee, webkit-bug-importer, zimmermann | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Said Abou-Hallawa
2016-04-13 12:36:00 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. Created attachment 276347 [details]
Patch
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? 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(). Comment on attachment 276347 [details] Patch Clearing flags on attachment: 276347 Committed r199598: <http://trac.webkit.org/changeset/199598> All reviewed patches have been landed. Closing bug. |