Bug 181316

Summary: SVGAnimatedListPropertyTearOff::synchronizeWrappersIfNeeded() should do nothing if the property is not animating
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: SVGAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, simon.fraser, thorton, webkit-bug-importer, zalan, zimmermann
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2018-01-04 20:36:44 PST
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.
Comment 1 Said Abou-Hallawa 2018-01-04 20:40:03 PST
<rdar://problem/36147545>
Comment 2 Said Abou-Hallawa 2018-01-04 20:40:54 PST
Created attachment 330518 [details]
Patch
Comment 3 Said Abou-Hallawa 2018-01-05 10:27:16 PST
Created attachment 330556 [details]
Patch
Comment 4 Simon Fraser (smfr) 2018-01-05 10:31:00 PST
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.
Comment 5 Said Abou-Hallawa 2018-01-05 11:05:15 PST
Created attachment 330559 [details]
Patch
Comment 6 WebKit Commit Bot 2018-01-05 12:10:48 PST
Comment on attachment 330559 [details]
Patch

Clearing flags on attachment: 330559

Committed r226457: <https://trac.webkit.org/changeset/226457>
Comment 7 WebKit Commit Bot 2018-01-05 12:10:50 PST
All reviewed patches have been landed.  Closing bug.