Bug 113224 - ASSERT_NOT_REACHED() touched in WebCore::SVGAnimatedStringAnimator::addAnimatedTypes
Summary: ASSERT_NOT_REACHED() touched in WebCore::SVGAnimatedStringAnimator::addAnimat...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords:
Depends on: 119609
Blocks: 116980
  Show dependency treegraph
 
Reported: 2013-03-25 11:01 PDT by Renata Hodovan
Modified: 2013-08-09 08:01 PDT (History)
10 users (show)

See Also:


Attachments
Patch (11.38 KB, patch)
2013-08-08 14:02 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (13.37 KB, patch)
2013-08-08 18:56 PDT, Rob Buis
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Renata Hodovan 2013-03-25 11:01:45 PDT
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
Comment 1 Rob Buis 2013-08-08 14:02:23 PDT
Created attachment 208368 [details]
Patch
Comment 2 Dirk Schulze 2013-08-08 14:10:52 PDT
Comment on attachment 208368 [details]
Patch

r=me.
Comment 3 Rob Buis 2013-08-08 14:30:31 PDT
Committed r153847: <http://trac.webkit.org/changeset/153847>
Comment 4 Simon Fraser (smfr) 2013-08-08 17:39:27 PDT
This caused lots of new test failures:
http://build.webkit.org/results/Apple%20Lion%20Debug%20WK2%20(Tests)/r153848%20(10678)/results.html
Comment 5 WebKit Commit Bot 2013-08-08 17:45:05 PDT
Re-opened since this is blocked by bug 119609
Comment 6 Rob Buis 2013-08-08 18:56:48 PDT
Created attachment 208387 [details]
Patch
Comment 7 Darin Adler 2013-08-08 19:14:15 PDT
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.
Comment 8 Rob Buis 2013-08-09 06:28:38 PDT
Committed r153887: <http://trac.webkit.org/changeset/153887>
Comment 9 Csaba Osztrogonác 2013-08-09 06:30:00 PDT
(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.
Comment 10 Zoltan Arvai 2013-08-09 07:56:04 PDT
Buildfix landed in http://trac.webkit.org/changeset/153892.
Comment 11 Rob Buis 2013-08-09 08:01:24 PDT
(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.