Assertion fires when animating the 'class' attribute of an SVG element

[Said Abou-Hallawa]

Created attachment 368462 [details]
test case

Open the attached test case in a debug build. The following assertion will fire:


0x0000000120fa93ee in ::WTFCrash() at /Volumes/Data/WebKit/OpenSource/Source/WTF/wtf/Assertions.cpp:305
0x000000010bdeb929 in WebCore::SVGAnimatedPrimitiveProperty<WTF::String>::currentValue() const at /Volumes/Data/WebKit/OpenSource/Source/WebCore/svg/properties/SVGAnimatedPrimitiveProperty.h:103
0x000000010d9fe692 in WebCore::SVGElement::className() const at /Volumes/Data/WebKit/OpenSource/Source/WebCore/svg/SVGElement.h:149
0x000000010d9fe572 in WebCore::SVGElement::svgAttributeChanged(WebCore::QualifiedName const&) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/svg/SVGElement.cpp:858
0x000000010db39d75 in WebCore::SVGGraphicsElement::svgAttributeChanged(WebCore::QualifiedName const&) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/svg/SVGGraphicsElement.cpp:150
0x000000010db3aea2 in WebCore::SVGGeometryElement::svgAttributeChanged(WebCore::QualifiedName const&) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/svg/SVGGeometryElement.cpp:116
0x000000010dbf9c38 in WebCore::SVGRectElement::svgAttributeChanged(WebCore::QualifiedName const&) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/svg/SVGRectElement.cpp:87
0x000000010dce07bd in WebCore::SVGAttributeAnimator::applyAnimatedPropertyChange(WebCore::SVGElement*, WebCore::QualifiedName const&) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/svg/properties/SVGAttributeAnimator.cpp:100
0x000000010dce09ab in WebCore::SVGAttributeAnimator::applyAnimatedPropertyChange(WebCore::SVGElement*) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/svg/properties/SVGAttributeAnimator.cpp:117
0x000000010d97d6fb in WebCore::SVGAnimatedPropertyAnimator<WebCore::SVGAnimatedPrimitiveProperty<WTF::String>, WebCore::SVGAnimationStringFunction>::apply(WebCore::SVGElement*) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/svg/properties/SVGAnimatedPropertyAnimator.h:80
0x000000010d968202 in WebCore::SVGAnimateElementBase::applyResultsToTarget() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/svg/SVGAnimateElementBase.cpp:176
0x000000010dcb678d in WebCore::SMILTimeContainer::updateAnimations(WebCore::SMILTime, bool) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/svg/animation/SMILTimeContainer.cpp:321
0x000000010dcb6020 in WebCore::SMILTimeContainer::begin() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/svg/animation/SMILTimeContainer.cpp:138
0x000000010d9f5fa1 in WebCore::SVGDocumentExtensions::startAnimations() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/svg/SVGDocumentExtensions.cpp:97
0x000000010c3a65ca in WebCore::Document::implicitClose() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/dom/Document.cpp:3007

This is a regression of removing the SVG tear off objects. But the attached text did not work correctly before removing them also.

The assertion fires because the animated primitive property do not share the animVal with all its instances.

[Said Abou-Hallawa]

Created attachment 368463 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/50225885>

[Said Abou-Hallawa]

Created attachment 371199 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 371199 [details]
Patch

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

> Source/WebCore/svg/properties/SVGAnimatedPrimitiveProperty.h:155
> -    mutable Optional<PropertyType> m_animVal;
> +    mutable std::shared_ptr<PropertyType> m_animVal;

We don't use shared_Ptr anywhere else in our codebase.
Can we just wrap PropertyType in some RefCounted object? Like SharedPropertyType?
e.g. class SharedPropertyType : public RefCounted<SharedPropertyType> public PropertyType { }
This would avoid an extra malloc for shared_ptr.

[Said Abou-Hallawa]

Created attachment 373269 [details]
Patch

[Darin Adler]

Comment on attachment 373269 [details]
Patch

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

> Source/WebCore/ChangeLog:24
> +        -- Add a special case for the 'class' attribute to invalidate the style
> +           of the target element when its animVal changes.

To me, this doesn’t seem like the right solution. Knowledge about which DOM attributes have effect on style should not be concentrated in the SVGAttributeAnimator class. Instead that knowledge should live in the DOM classes themselves. I’m puzzled at the choice to add this special case here.

[Said Abou-Hallawa]

Created attachment 373365 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 373365 [details]
Patch

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

> Source/WebCore/svg/properties/SVGPrimitiveProperty.h:33
> +class SVGPrimitiveProperty : public SVGProperty {

Maybe SVGSharedPrimitiveProperty?

[Said Abou-Hallawa]

Comment on attachment 373269 [details]
Patch

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

>> Source/WebCore/ChangeLog:24
>> +           of the target element when its animVal changes.
> 
> To me, this doesn’t seem like the right solution. Knowledge about which DOM attributes have effect on style should not be concentrated in the SVGAttributeAnimator class. Instead that knowledge should live in the DOM classes themselves. I’m puzzled at the choice to add this special case here.

I moved the special handling to SVGAnimatedStringAnimator because we need to check whether the attribute's name is equal to 'class' or not. The SVGAnimatedString is not suitable because it does not know about the attribute's name.

[Said Abou-Hallawa]

Created attachment 373371 [details]
Patch

[Said Abou-Hallawa]

Created attachment 373373 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 373373 [details]
Patch

Clearing flags on attachment: 373373

Committed r247085: <https://trac.webkit.org/changeset/247085>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.