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
Created attachment 208368 [details] Patch
Comment on attachment 208368 [details] Patch r=me.
Committed r153847: <http://trac.webkit.org/changeset/153847>
This caused lots of new test failures: http://build.webkit.org/results/Apple%20Lion%20Debug%20WK2%20(Tests)/r153848%20(10678)/results.html
Re-opened since this is blocked by bug 119609
Created attachment 208387 [details] Patch
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.
Committed r153887: <http://trac.webkit.org/changeset/153887>
(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.
Buildfix landed in http://trac.webkit.org/changeset/153892.
(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.