Bug 181316 – SVGAnimatedListPropertyTearOff::synchronizeWrappersIfNeeded() should do nothing if the property is not animating

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Said Abou-Hallawa]

If SVGAnimatedListPropertyTearOff is not animating this means its m_animatedProperty is null. In this case SVGAnimatedListPropertyTearOff::synchronizeWrappersIfNeeded() should do nothing. Otherwise a crash will happen.

SVGAnimatedListPropertyTearOff::synchronizeWrappersIfNeeded() can now be called from SVGAnimatedTypeAnimator::resetFromBaseValue() and a very intermittent crash with the following call stack was recorded:

WebCore::SVGListProperty<WebCore::SVGTransformListValues>::values()
WebCore::SVGAnimatedListPropertyTearOff<WebCore::SVGTransformListValues>::synchronizeWrappersIfNeeded()
WebCore::SVGAnimatedTypeAnimator::resetFromBaseValue<WebCore::SVGAnimatedTransformListPropertyTearOff>()
WebCore::SVGAnimatedTransformListAnimator::resetAnimValToBaseVal()
WebCore::SVGAnimateElementBase::resetAnimatedType()

The crash was happening because SVGAnimatedListPropertyTearOff::m_animatedProperty is null.

[Said Abou-Hallawa]

<rdar://problem/36147545>

[Said Abou-Hallawa]

Created attachment 330518 [details]
Patch

[Said Abou-Hallawa]

Created attachment 330556 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 330556 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330556&action=review

> Source/WebCore/ChangeLog:9
> +        This is a speculative change to fix a crash which appeared after r226065.

This should say why there is no testcase.

> Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h:147
>          ASSERT(isAnimating());
> +        if (!isAnimating())

We normally avoid patterns like this. If you think the assert and the if() are both needed, then add a comment saying "this should never happen, but we've seen it in the field. Please comment in bug ### i you hit this" or something.

[Said Abou-Hallawa]

Created attachment 330559 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 330559 [details]
Patch

Clearing flags on attachment: 330559

Committed r226457: <https://trac.webkit.org/changeset/226457>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.