Bug 201926 – Assertion fires when animating a discrete property with values range and multiple animators

• Component: SVG
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

Created attachment 379054 [details]
test case (assertion fires in debug builds)

Open the attached test case.

In Debug build, the following assertion will fire:

stderr:
_RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
ERROR: File failed to delete. Error message: Operation not permitted
/Volumes/Data/WebKit/OpenSource/Source/WTF/wtf/posix/FileSystemPOSIX.cpp(77) : bool WTF::FileSystemImpl::deleteFile(const WTF::String &)
SHOULD NEVER BE REACHED
/Volumes/Data/WebKit/OpenSource/Source/WebCore/svg/properties/SVGAnimationDiscreteFunction.h(42) : virtual void WebCore::SVGAnimationDiscreteFunction<WTF::String>::setToAtEndOfDurationValue(const WTF::String &) [ValueType = WTF::String]
1   0x2401dea29 WTFCrash
2   0x22800935b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x22bd3e8f3 WebCore::SVGAnimationDiscreteFunction<WTF::String>::setToAtEndOfDurationValue(WTF::String const&)
4   0x22bd3e04c WebCore::SVGAnimatedPropertyAnimator<WebCore::SVGAnimatedPrimitiveProperty<WTF::String>, WebCore::SVGAnimationStringFunction>::setToAtEndOfDurationValue(WTF::String const&)
5   0x22bd27f7a WebCore::SVGAnimateElementBase::calculateToAtEndOfDurationValue(WTF::String const&)
6   0x22bd5d55b WebCore::SVGAnimationElement::startedActiveInterval()
7   0x22c078b38 WebCore::SVGSMILElement::progress(WebCore::SMILTime, WebCore::SVGSMILElement*, bool)
8   0x22c076ff3 WebCore::SMILTimeContainer::updateAnimations(WebCore::SMILTime, bool)
9   0x22c076ab0 WebCore::SMILTimeContainer::begin()
10  0x22bdb6411 WebCore::SVGDocumentExtensions::startAnimations()
11  0x22a5f0802 WebCore::Document::implicitClose()
12  0x22aff30fb WebCore::FrameLoader::checkCallImplicitClose()
13  0x22aff2be4 WebCore::FrameLoader::checkCompleted()
14  0x22aff0f15 WebCore::FrameLoader::finishedParsing()
15  0x22a6081a8 WebCore::Document::finishedParsing()
16  0x22c24e0c8 WebCore::XMLDocumentParser::end()
17  0x22c24e44e WebCore::XMLDocumentParser::finish()
18  0x22af9d57a WebCore::DocumentWriter::end()
19  0x22af9c45f WebCore::DocumentLoader::finishedLoading()
20  0x22af9c09d WebCore::DocumentLoader::notifyFinished(WebCore::CachedResource&)
21  0x22b0e090f WebCore::CachedResource::checkNotify()
22  0x22b0dc7c1 WebCore::CachedResource::finishLoading(WebCore::SharedBuffer*)
23  0x22b0ddaa9 WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer*)
24  0x22b072851 WebCore::SubresourceLoader::didFinishLoading(WebCore::NetworkLoadMetrics const&)
25  0x103597517 WebKit::WebResourceLoader::didFinishResourceLoad(WebCore::NetworkLoadMetrics const&)
26  0x103ae921a void IPC::callMemberFunctionImpl<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>, 0ul>(WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>&&, std::__1::integer_sequence<unsigned long, 0ul>)
27  0x103ae9170 void IPC::callMemberFunction<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>, std::__1::integer_sequence<unsigned long, 0ul> >(std::__1::tuple<WebCore::NetworkLoadMetrics>&&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&))
28  0x103ae86db void IPC::handleMessage<Messages::WebResourceLoader::DidFinishResourceLoad, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&)>(IPC::Decoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&))
29  0x103ae7e8c WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::Decoder&)
30  0x10358aeb6 WebKit::NetworkProcessConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
31  0x102547a59 IPC::Connection::dispatchMessage(IPC::Decoder&)
LEAK: 1 WebPageProxy

[Said Abou-Hallawa]

Created attachment 379060 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 379060 [details]
Patch

Good catch, looks fine as is.
I assume you verified the new layout tests currently hits the assertion without your patch.

[Darin Adler]

Comment on attachment 379060 [details]
Patch

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

> Source/WebCore/ChangeLog:22
> +        The bug is isDiscreteAnimator() will return false if the m_animator is 
> +        null. So the fix is ensure m_animator is created before calling
> +        isDiscreteAnimator(). This should also be done in calculateFromAndByValues().

Conventionally, the fix for something like this would be to have isDiscreteAnimator() call animator() so it is created rather than relying on a side effect of requiring the caller to call animator() first. Maybe there’s a good reason not to do that, but it seems dangerous to rely on side effects and the other of calling functions.

[Nikolas Zimmermann]

(In reply to Darin Adler from comment #3)
> Conventionally, the fix for something like this would be to have
> isDiscreteAnimator() call animator() so it is created rather than relying on
> a side effect of requiring the caller to call animator() first. Maybe
> there’s a good reason not to do that, but it seems dangerous to rely on side
> effects and the other of calling functions.

I wouldn't expect a function called "isDiscreteAnimator" to create the animator.
But I agree that either solution is potentially dangerous, so good you brought up this point. Currently with Saids approach, all call sites (from SVGAnimateElementBase and SVGAnimationElement) are safe, but we should protect us for the future, to avoid introducing bugs.

I will leave this to Said to propose a better approach, not relying on side effects.

[Simon Fraser (smfr)]

Comment on attachment 379060 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        The first animator of a property is considered the result element. The

What is "the result element"?

>> Source/WebCore/ChangeLog:22
>> +        isDiscreteAnimator(). This should also be done in calculateFromAndByValues().
> 
> Conventionally, the fix for something like this would be to have isDiscreteAnimator() call animator() so it is created rather than relying on a side effect of requiring the caller to call animator() first. Maybe there’s a good reason not to do that, but it seems dangerous to rely on side effects and the other of calling functions.

Maybe isDiscreteAnimator() should assert that animator() is not null, or the isDiscreteAnimator() should be on animator itself.

[Said Abou-Hallawa]

Created attachment 379246 [details]
Patch

[Darin Adler]

Comment on attachment 379246 [details]
Patch

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

> Source/WebCore/svg/SVGAnimateElementBase.cpp:84
> +    if (auto* animator = this->animator())
> +        return animator->isDiscrete();
> +
> +    return false;

Another way to write this is:

    auto* animator = this->animator();
    return animator && animator->isDiscrete();

Not sure which style I like better, but I have often chosen to do a null guard with && rather than early exit.

> Source/WebCore/svg/SVGAnimateElementBase.h:48
> +    bool hasInvalidCSSAttributeType() const;

Doesn’t seem like we needed to move this line of code. Could have left it where it was, after the overrides. Maybe was moved as part of intermediate steps?

[Said Abou-Hallawa]

Created attachment 379278 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 379060 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        The first animator of a property is considered the result element. The
> 
> What is "the result element"?

The result element is the first animator if multiple animators animate the same attribute. I filed https://bugs.webkit.org/show_bug.cgi?id=202052 which explains the reason for naming it resultElement and to do the necessary clean up.

>>> Source/WebCore/ChangeLog:22
>>> +        isDiscreteAnimator(). This should also be done in calculateFromAndByValues().
>> 
>> Conventionally, the fix for something like this would be to have isDiscreteAnimator() call animator() so it is created rather than relying on a side effect of requiring the caller to call animator() first. Maybe there’s a good reason not to do that, but it seems dangerous to rely on side effects and the other of calling functions.
> 
> Maybe isDiscreteAnimator() should assert that animator() is not null, or the isDiscreteAnimator() should be on animator itself.

I made isDiscreteAnimator() call animator() instead of calling animatorIfExists(). animator() ensures the m_animator is created.

[Said Abou-Hallawa]

Comment on attachment 379246 [details]
Patch

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

>> Source/WebCore/svg/SVGAnimateElementBase.cpp:84
>> +    return false;
> 
> Another way to write this is:
> 
>     auto* animator = this->animator();
>     return animator && animator->isDiscrete();
> 
> Not sure which style I like better, but I have often chosen to do a null guard with && rather than early exit.

Done.

>> Source/WebCore/svg/SVGAnimateElementBase.h:48
>> +    bool hasInvalidCSSAttributeType() const;
> 
> Doesn’t seem like we needed to move this line of code. Could have left it where it was, after the overrides. Maybe was moved as part of intermediate steps?

Fixed. The diff in the last patch is cleaner.

[WebKit Commit Bot]

Comment on attachment 379278 [details]
Patch

Clearing flags on attachment: 379278

Committed r250175: <https://trac.webkit.org/changeset/250175>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55579525>