Bug 113224 – ASSERT_NOT_REACHED() touched in WebCore::SVGAnimatedStringAnimator::addAnimatedTypes

• Component: SVG
• Status: RESOLVED FIXED

[Renata Hodovan]

The test what caused the problem is:

<svg xmlns="http://www.w3.org/2000/svg">
	<animate attributeName="fill-rule" from="500,500" by="100, 100"></animate>
</svg>


(It's a minimalized version, what still crashes but doesn't show anything.)


BACKTRACE:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff4c1cd56 in WebCore::SVGAnimatedStringAnimator::addAnimatedTypes (this=0x94bab0) at /home/reni/WebKit-git/Source/WebCore/svg/SVGAnimatedString.cpp:68
68	    ASSERT_NOT_REACHED();
(gdb) 
(gdb) bt   
#0  0x00007ffff4c1cd56 in WebCore::SVGAnimatedStringAnimator::addAnimatedTypes (this=0x94bab0) at /home/reni/WebKit-git/Source/WebCore/svg/SVGAnimatedString.cpp:68
#1  0x00007ffff4c24a25 in WebCore::SVGAnimatedTypeAnimator::calculateFromAndByValues (this=0x94bab0, from=..., to=..., fromString=..., byString=...)
    at /home/reni/WebKit-git/Source/WebCore/svg/SVGAnimatedTypeAnimator.h:75
#2  0x00007ffff4c2682b in WebCore::SVGAnimateElement::calculateFromAndByValues (this=0x94c2b0, fromString=..., byString=...)
    at /home/reni/WebKit-git/Source/WebCore/svg/SVGAnimateElement.cpp:176
#3  0x00007ffff4c2f29b in WebCore::SVGAnimationElement::startedActiveInterval (this=0x94c2b0) at /home/reni/WebKit-git/Source/WebCore/svg/SVGAnimationElement.cpp:587
#4  0x00007ffff4bd4a90 in WebCore::SVGSMILElement::progress (this=0x94c2b0, elapsed=..., resultElement=0x94c2b0, seekToTime=false)
    at /home/reni/WebKit-git/Source/WebCore/svg/animation/SVGSMILElement.cpp:1106
#5  0x00007ffff4bca9d8 in WebCore::SMILTimeContainer::updateAnimations (this=0x948d60, elapsed=..., seekToTime=false)
    at /home/reni/WebKit-git/Source/WebCore/svg/animation/SMILTimeContainer.cpp:296
#6  0x00007ffff4bc9e8d in WebCore::SMILTimeContainer::begin (this=0x948d60) at /home/reni/WebKit-git/Source/WebCore/svg/animation/SMILTimeContainer.cpp:142
#7  0x00007ffff4beef58 in WebCore::SVGDocumentExtensions::startAnimations (this=0x94b070) at /home/reni/WebKit-git/Source/WebCore/svg/SVGDocumentExtensions.cpp:102
#8  0x00007ffff40c5bb5 in WebCore::Document::implicitClose (this=0x9433c0) at /home/reni/WebKit-git/Source/WebCore/dom/Document.cpp:2532
#9  0x00007ffff453ea6d in WebCore::FrameLoader::checkCallImplicitClose (this=0x7012a8) at /home/reni/WebKit-git/Source/WebCore/loader/FrameLoader.cpp:837
#10 0x00007ffff453e801 in WebCore::FrameLoader::checkCompleted (this=0x7012a8) at /home/reni/WebKit-git/Source/WebCore/loader/FrameLoader.cpp:780
#11 0x00007ffff453e566 in WebCore::FrameLoader::finishedParsing (this=0x7012a8) at /home/reni/WebKit-git/Source/WebCore/loader/FrameLoader.cpp:713
#12 0x00007ffff40ccd1f in WebCore::Document::finishedParsing (this=0x9433c0) at /home/reni/WebKit-git/Source/WebCore/dom/Document.cpp:4493
#13 0x00007ffff4a1f34f in WebCore::XMLDocumentParser::end (this=0x7075d0) at /home/reni/WebKit-git/Source/WebCore/xml/parser/XMLDocumentParser.cpp:217
#14 0x00007ffff4a1f38c in WebCore::XMLDocumentParser::finish (this=0x7075d0) at /home/reni/WebKit-git/Source/WebCore/xml/parser/XMLDocumentParser.cpp:229
#15 0x00007ffff45362fe in WebCore::DocumentWriter::end (this=0x678f20) at /home/reni/WebKit-git/Source/WebCore/loader/DocumentWriter.cpp:248
#16 0x00007ffff4523bf4 in WebCore::DocumentLoader::finishedLoading (this=0x678e80, finishTime=0) at /home/reni/WebKit-git/Source/WebCore/loader/DocumentLoader.cpp:402
#17 0x00007ffff4523950 in WebCore::DocumentLoader::notifyFinished (this=0x678e80, resource=0x745240)
    at /home/reni/WebKit-git/Source/WebCore/loader/DocumentLoader.cpp:341
#18 0x00007ffff4503ec2 in WebCore::CachedResource::checkNotify (this=0x745240) at /home/reni/WebKit-git/Source/WebCore/loader/cache/CachedResource.cpp:379
#19 0x00007ffff4503f20 in WebCore::CachedResource::data (this=0x745240, allDataReceived=true)
    at /home/reni/WebKit-git/Source/WebCore/loader/cache/CachedResource.cpp:388

[Rob Buis]

Created attachment 208368 [details]
Patch

[Dirk Schulze]

Comment on attachment 208368 [details]
Patch

r=me.

[Rob Buis]

Committed r153847: <http://trac.webkit.org/changeset/153847>

[Simon Fraser (smfr)]

This caused lots of new test failures:
http://build.webkit.org/results/Apple%20Lion%20Debug%20WK2%20(Tests)/r153848%20(10678)/results.html

[WebKit Commit Bot]

Re-opened since this is blocked by bug 119609

[Rob Buis]

Created attachment 208387 [details]
Patch

[Darin Adler]

Comment on attachment 208387 [details]
Patch

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

> Source/WebCore/svg/SVGAnimateElement.cpp:391
> +    // Spec: http://www.w3.org/TR/SVG/animate.html#AnimationAttributesAndProperties.
> +    switch (m_animatedPropertyType) {
> +    case AnimatedBoolean:
> +    case AnimatedEnumeration:
> +    case AnimatedPreserveAspectRatio:
> +    case AnimatedString:
> +    case AnimatedUnknown:
> +        return false;
> +    default:
> +        return true;
> +    }

It’s nice to not have a default case in such switches, since gcc-family compilers will warn if you forget a type, but include a default turns off that warning. I like the style where have cases for all the enum values, and then an ASSERT_NOT_REACHED outside the switch.

[Rob Buis]

Committed r153887: <http://trac.webkit.org/changeset/153887>

[Csaba Osztrogonác]

(In reply to comment #7)
> (From update of attachment 208387 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208387&action=review
> 
> > Source/WebCore/svg/SVGAnimateElement.cpp:391
> > +    // Spec: http://www.w3.org/TR/SVG/animate.html#AnimationAttributesAndProperties.
> > +    switch (m_animatedPropertyType) {
> > +    case AnimatedBoolean:
> > +    case AnimatedEnumeration:
> > +    case AnimatedPreserveAspectRatio:
> > +    case AnimatedString:
> > +    case AnimatedUnknown:
> > +        return false;
> > +    default:
> > +        return true;
> > +    }
> 
> It’s nice to not have a default case in such switches, since gcc-family compilers will warn if you forget a type, but include a default turns off that warning. I like the style where have cases for all the enum values, and then an ASSERT_NOT_REACHED outside the switch.

It wasn't a good idea to change it to ASSERT_NOT_REACHED(); without default,
because it broke the gcc build with the following warning:
Source/WebCore/svg/SVGAnimateElement.cpp:406:1: error: control reaches end of non-void function [-Werror=return-type]

I think a RELEASE_ASSERT_NOT_REACHED() inside the default label would be better.

[Zoltan Arvai]

Buildfix landed in http://trac.webkit.org/changeset/153892.

[Rob Buis]

(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 208387 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=208387&action=review
> > 
> > > Source/WebCore/svg/SVGAnimateElement.cpp:391
> > > +    // Spec: http://www.w3.org/TR/SVG/animate.html#AnimationAttributesAndProperties.
> > > +    switch (m_animatedPropertyType) {
> > > +    case AnimatedBoolean:
> > > +    case AnimatedEnumeration:
> > > +    case AnimatedPreserveAspectRatio:
> > > +    case AnimatedString:
> > > +    case AnimatedUnknown:
> > > +        return false;
> > > +    default:
> > > +        return true;
> > > +    }
> > 
> > It’s nice to not have a default case in such switches, since gcc-family compilers will warn if you forget a type, but include a default turns off that warning. I like the style where have cases for all the enum values, and then an ASSERT_NOT_REACHED outside the switch.
> 
> It wasn't a good idea to change it to ASSERT_NOT_REACHED(); without default,
> because it broke the gcc build with the following warning:
> Source/WebCore/svg/SVGAnimateElement.cpp:406:1: error: control reaches end of non-void function [-Werror=return-type]
> 
> I think a RELEASE_ASSERT_NOT_REACHED() inside the default label would be better.

Sorry about that, I clearly need a coffee :) Thanks for the fix.