Bug 21004

Summary: SVG animation example asserts
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, dino, jeffschiller, koivisto, mitz, zimmermann
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
URL: https://bugs.webkit.org/attachment.cgi?id=21568&action=view

Description Simon Fraser (smfr) 2008-09-22 13:28:53 PDT
The SVG animation example at https://bugs.webkit.org/attachment.cgi?id=21568&action=view asserts in debug builds, here:

#0  0x037f0768 in WTF::HashTableConstIterator<WebCore::SVGSVGElement*, WebCore::SVGSVGElement*, WTF::IdentityExtractor<WebCore::SVGSVGElement*>, WTF::PtrHash<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*> >::checkValidity (this=0xbfffd808) at HashTable.h:183
#1  0x037f154e in WTF::HashTableConstIterator<WebCore::SVGSVGElement*, WebCore::SVGSVGElement*, WTF::IdentityExtractor<WebCore::SVGSVGElement*>, WTF::PtrHash<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*> >::operator++ (this=0xbfffd808) at HashTable.h:158
#2  0x037f15c5 in WTF::HashTableIterator<WebCore::SVGSVGElement*, WebCore::SVGSVGElement*, WTF::IdentityExtractor<WebCore::SVGSVGElement*>, WTF::PtrHash<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*> >::operator++ (this=0xbfffd808) at HashTable.h:234
#3  0x037f15db in WTF::HashTableIteratorAdapter<WTF::HashTable<WebCore::SVGSVGElement*, WebCore::SVGSVGElement*, WTF::IdentityExtractor<WebCore::SVGSVGElement*>, WTF::PtrHash<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*> >, WebCore::SVGSVGElement*>::operator++ (this=0xbfffd808) at HashTable.h:1104
#4  0x037ee788 in WebCore::SVGDocumentExtensions::startAnimations (this=0x1aba00e0) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/svg/SVGDocumentExtensions.cpp:71
#5  0x033f39ee in WebCore::Document::implicitClose (this=0x6901a00) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/dom/Document.cpp:1643
#6  0x0349180e in WebCore::FrameLoader::checkCallImplicitClose (this=0x686be24) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/loader/FrameLoader.cpp:1350
#7  0x0349dfc0 in WebCore::FrameLoader::checkCompleted (this=0x686be24) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/loader/FrameLoader.cpp:1302
#8  0x034a0a3b in WebCore::FrameLoader::finishedParsing (this=0x686be24) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/loader/FrameLoader.cpp:1252
#9  0x033f046c in WebCore::Document::finishedParsing (this=0x6901a00) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/dom/Document.cpp:3824
#10 0x038dad83 in WebCore::XMLTokenizer::end (this=0x1ab8dff0) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/dom/XMLTokenizer.cpp:226
#11 0x038dadab in WebCore::XMLTokenizer::finish (this=0x1ab8dff0) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/dom/XMLTokenizer.cpp:234
#12 0x033ea338 in WebCore::Document::finishParsing (this=0x6901a00) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/dom/Document.cpp:1728
#13 0x0349e1af in WebCore::FrameLoader::endIfNotLoadingMainResource (this=0x686be24) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/loader/FrameLoader.cpp:1075
#14 0x0349e1e5 in WebCore::FrameLoader::end (this=0x686be24) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/loader/FrameLoader.cpp:1059
#15 0x0341be6c in WebCore::DocumentLoader::finishedLoading (this=0x68e8200) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/loader/DocumentLoader.cpp:343
#16 0x03499116 in WebCore::FrameLoader::finishedLoading (this=0x686be24) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/loader/FrameLoader.cpp:2961
#17 0x036a5231 in WebCore::MainResourceLoader::didFinishLoading (this=0x68ec800) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/loader/MainResourceLoader.cpp:320
#18 0x037c1a98 in WebCore::ResourceLoader::didFinishLoading (this=0x68ec800) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/loader/ResourceLoader.cpp:398
#19 0x037bf076 in -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] (self=0x1ab1c740, _cmd=0x911985c4, con=0xe67f30) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/platform/network/mac/ResourceHandleMac.mm:530
#20 0x9026e3f7 in -[NSURLConnection(NSURLConnectionReallyInternal) sendDidFinishLoading] ()
#21 0x9026e363 in _NSURLConnectionDidFinishLoading ()
#22 0x9565fcef in sendDidFinishLoadingCallback ()
...
Comment 1 Simon Fraser (smfr) 2008-09-22 13:45:37 PDT
The HasSet is being modified during enumeration:

#0  0x037ee823 in WebCore::SVGDocumentExtensions::removeTimeContainer (this=0x1abc73c0, element=0x1abe34d0) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/svg/SVGDocumentExtensions.cpp:62
#1  0x0385b1ef in WebCore::SVGSVGElement::~SVGSVGElement (this=0x1abe34d0) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/svg/SVGSVGElement.cpp:82
#2  0x033049f8 in WebCore::ContainerNode::removeAllChildren (this=0x1abe49a0) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/dom/ContainerNode.cpp:113
#3  0x0330810c in WebCore::ContainerNode::~ContainerNode (this=0x1abe49a0) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/dom/ContainerNode.cpp:119
#4  0x0343ef11 in WebCore::Element::~Element (this=0x1abe49a0) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/dom/Element.cpp:126
#5  0x0389c139 in WebCore::StyledElement::~StyledElement (this=0x1abe49a0) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/dom/StyledElement.cpp:125
#6  0x037f71d4 in WebCore::SVGElement::~SVGElement (this=0x1abe49a0) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/svg/SVGElement.cpp:59
#7  0x0385f47b in WebCore::SVGStyledElement::~SVGStyledElement (this=0x1abe49a0) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/svg/SVGStyledElement.cpp:57
#8  0x03861401 in WebCore::SVGStyledLocatableElement::~SVGStyledLocatableElement (this=0x1abe49a0, __vtt_parm=0x45abd88) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/svg/SVGStyledLocatableElement.cpp:43
#9  0x03861d96 in WebCore::SVGStyledTransformableElement::~SVGStyledTransformableElement (this=0x1abe49a0, __vtt_parm=0x45abd84) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/svg/SVGStyledTransformableElement.cpp:49
#10 0x038046c9 in WebCore::SVGGElement::~SVGGElement (this=0x1abe49a0) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/svg/SVGGElement.cpp:42
#11 0x0322caab in WebCore::TreeShared<WebCore::Node>::removedLastRef (this=0x1abe49a0) at TreeShared.h:99
#12 0x038665ff in WebCore::TreeShared<WebCore::Node>::deref (this=0x1abe49a0) at TreeShared.h:69
#13 0x03877bb6 in WTF::RefPtr<WebCore::SVGElement>::operator= (this=0x1abe1ba0, optr=0x1abab600) at RefPtr.h:118
#14 0x03876f2d in WebCore::SVGUseElement::buildPendingResource (this=0x1abe1a40) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/svg/SVGUseElement.cpp:310
#15 0x038740a5 in WebCore::SVGUseElement::svgAttributeChanged (this=0x1abe1a40, attrName=@0x1abbe94c) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/svg/SVGUseElement.cpp:139
#16 0x037f6944 in WebCore::SVGElement::attributeChanged (this=0x1abe1a40, attr=0x1abbe940, preserveDecls=false) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/svg/SVGElement.cpp:266
#17 0x036b6cda in WebCore::NamedAttrMap::addAttribute (this=0x1abe1bd0, prpAttribute=@0xbfffd590) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/dom/NamedAttrMap.cpp:250
#18 0x0343d773 in WebCore::Element::setAttribute (this=0x1abe1a40, name=@0xbfffd620, value=@0xbfffd61c, ec=@0xbfffd610) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/dom/Element.cpp:525
#19 0x037dc70e in WebCore::SVGAnimationElement::setTargetAttributeAnimatedValue (this=0x1abe27f0, value=@0xbfffd680) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/svg/SVGAnimationElement.cpp:307
#20 0x037d62aa in WebCore::SVGAnimateElement::applyResultsToTarget (this=0x1abe27f0) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/svg/SVGAnimateElement.cpp:258
#21 0x039d600f in WebCore::SMILTimeContainer::updateAnimations (this=0x1abc70f0, elapsed=@0xbfffd7d8) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/svg/animation/SMILTimeContainer.cpp:275
#22 0x039d6238 in WebCore::SMILTimeContainer::begin (this=0x1abc70f0) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/svg/animation/SMILTimeContainer.cpp:102
#23 0x037ee77d in WebCore::SVGDocumentExtensions::startAnimations (this=0x1abc73c0) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/svg/SVGDocumentExtensions.cpp:72
#24 0x033f39ee in WebCore::Document::implicitClose (this=0x6918600) at /Volumes/Eclair/WebKit-OpenSource.git/WebCore/dom/Document.cpp:1643
Comment 2 Simon Fraser (smfr) 2008-09-22 14:01:34 PDT
Here's a hacky fix:

diff --git a/WebCore/svg/SVGDocumentExtensions.cpp b/WebCore/svg/SVGDocumentExtensions.cpp
index 98e6d68..c5fc040 100644
--- a/WebCore/svg/SVGDocumentExtensions.cpp
+++ b/WebCore/svg/SVGDocumentExtensions.cpp
@@ -66,10 +66,19 @@ void SVGDocumentExtensions::startAnimations()
 {
     // FIXME: Eventually every "Time Container" will need a way to latch on to some global timer
     // starting animations for a document will do this "latching"
-#if ENABLE(SVG_ANIMATION)    
-    HashSet<SVGSVGElement*>::iterator end = m_timeContainers.end();
-    for (HashSet<SVGSVGElement*>::iterator itr = m_timeContainers.begin(); itr != end; ++itr)
-        (*itr)->timeContainer()->begin();
+#if ENABLE(SVG_ANIMATION)
+
+    // Make a copy, since calling begin() on a timeContainer may call back into
+    // addTimeContainer/removeTimeContainer and change the HashSet.
+    HashSet<SVGSVGElement*> timeContainersCopy(m_timeContainers);
+    
+    HashSet<SVGSVGElement*>::iterator end = timeContainersCopy.end();
+    for (HashSet<SVGSVGElement*>::iterator itr = timeContainersCopy.begin(); itr != end; ++itr)
+    {
+        // FIXME: hack
+        if (m_timeContainers.find(*itr) != m_timeContainers.end())
+            (*itr)->timeContainer()->begin();
+    }
 #endif
 }
     

Note that copying the HashSet is required to avoid modification during enumeration, and the .find() check is required because SVGSVGElements can be destroyed in begin() callbacks. It seems like m_timeContainers needs to hold references to SVG elements.
Comment 3 Nikolas Zimmermann 2008-09-24 06:43:40 PDT
This hack is probably needed, because internal SVGSVGElement's created during use-symbol-expansion, register themselves as time container, see bug 19432.
Comment 4 mitz 2008-09-24 07:44:18 PDT
<rdar://problem/6236387>
Comment 5 Nikolas Zimmermann 2010-01-18 20:02:32 PST
No assertion anymore in ToT, works just fine but this example triggers a bug in the new <use> implementation, that I expected to show up (we had no tests covering this). When width/height is a percentual value, window size changes are not handled correctly. Will fix this soon, needs a reduced testcase.
Comment 6 Nikolas Zimmermann 2010-01-20 18:14:02 PST
Oops, I mixed up the bug report - this example works fine now, no problems on resize, no assertions.