SVGAnimatedListPropertyTearOff::synchronizeWrappersIfNeeded() should …

…do nothing if the property is not animating

https://bugs.webkit.org/show_bug.cgi?id=181316
<rdar://problem/36147545>

Patch by Said Abou-Hallawa <sabouhallawa@apple.com> on 2018-01-05
Reviewed by Simon Fraser.

This is a speculative change to fix a crash which appeared after r226065.
The crash is very intermittent and sometimes very hard to reproduce. The
basic code analysis did not show how this crash can even happen.

* svg/SVGAnimatedTypeAnimator.h:
(WebCore::SVGAnimatedTypeAnimator::resetFromBaseValues): For SVG property
with two values, e.g. <SVGAngleValue, SVGMarkerOrientType>,  we need to
detach the wrappers of the animated property if the animated values are
going to change. This is similar to what we did in resetFromBaseValue().

* svg/properties/SVGAnimatedListPropertyTearOff.h:
(WebCore::SVGAnimatedListPropertyTearOff::synchronizeWrappersIfNeeded):

Canonical link: https://commits.webkit.org/197174@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@226457 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog

@@ -1,3 +1,24 @@
+2018-01-05  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        SVGAnimatedListPropertyTearOff::synchronizeWrappersIfNeeded() should do nothing if the property is not animating
+        https://bugs.webkit.org/show_bug.cgi?id=181316
+        <rdar://problem/36147545>
+
+        Reviewed by Simon Fraser.
+
+        This is a speculative change to fix a crash which appeared after r226065.
+        The crash is very intermittent and sometimes very hard to reproduce. The
+        basic code analysis did not show how this crash can even happen.
+
+        * svg/SVGAnimatedTypeAnimator.h:
+        (WebCore::SVGAnimatedTypeAnimator::resetFromBaseValues): For SVG property
+        with two values, e.g. <SVGAngleValue, SVGMarkerOrientType>,  we need to
+        detach the wrappers of the animated property if the animated values are
+        going to change. This is similar to what we did in resetFromBaseValue().
+
+        * svg/properties/SVGAnimatedListPropertyTearOff.h:
+        (WebCore::SVGAnimatedListPropertyTearOff::synchronizeWrappersIfNeeded):
+
 2018-01-05  Matt Lewis  <jlewis3@apple.com>
 
         Unreviewed, rolling out r226401.

Source/WebCore/svg/SVGAnimatedTypeAnimator.h

@@ -127,10 +127,14 @@ class SVGAnimatedTypeAnimator {
     {
         ASSERT(animatedTypes[0].properties.size() == 2);
         ASSERT(type.type() == m_type);
+        auto* firstProperty = castAnimatedPropertyToActualType<AnimValType1>(animatedTypes[0].properties[0].get());
+        auto* secondProperty =  castAnimatedPropertyToActualType<AnimValType2>(animatedTypes[0].properties[1].get());
+        firstProperty->synchronizeWrappersIfNeeded();
+        secondProperty->synchronizeWrappersIfNeeded();
 
         std::pair<typename AnimValType1::ContentType, typename AnimValType2::ContentType>& animatedTypeValue = (type.*getter)();
-        animatedTypeValue.first = castAnimatedPropertyToActualType<AnimValType1>(animatedTypes[0].properties[0].get())->currentBaseValue();
-        animatedTypeValue.second = castAnimatedPropertyToActualType<AnimValType2>(animatedTypes[0].properties[1].get())->currentBaseValue();
+        animatedTypeValue.first = firstProperty->currentBaseValue();
+        animatedTypeValue.second = secondProperty->currentBaseValue();
 
         executeAction<AnimValType1>(StartAnimationAction, animatedTypes, 0, &animatedTypeValue.first);
         executeAction<AnimValType2>(StartAnimationAction, animatedTypes, 1, &animatedTypeValue.second);

Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h

@@ -143,7 +143,11 @@ class SVGAnimatedListPropertyTearOff : public SVGAnimatedProperty {
 
     void synchronizeWrappersIfNeeded()
     {
-        ASSERT(isAnimating());
+        if (!isAnimating()) {
+            // This should never happen, but we've seen it in the field. Please comment in bug #181316 if you hit this.
+            ASSERT_NOT_REACHED();
+            return;
+        }
 
         // Eventually the wrapper list needs synchronization because any SVGAnimateLengthList::calculateAnimatedValue() call may
         // mutate the length of our values() list, and thus the wrapper() cache needs synchronization, to have the same size.