Bug 181316 - SVGAnimatedListPropertyTearOff::synchronizeWrappersIfNeeded() should do nothing if the property is not animating
Summary: SVGAnimatedListPropertyTearOff::synchronizeWrappersIfNeeded() should do nothi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-04 20:36 PST by Said Abou-Hallawa
Modified: 2018-01-05 12:10 PST (History)
6 users (show)

See Also:


Attachments
Patch (1.68 KB, patch)
2018-01-04 20:40 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (3.51 KB, patch)
2018-01-05 10:27 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (3.88 KB, patch)
2018-01-05 11:05 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.