Bug 19762 – Crash in svg/webarchive/svg-cursor-subresources.svg

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Alexey Proskuryakov]

I'm getting a semi-reproducible crash in svg/webarchive/svg-cursor-subresources.svg. When run twice in a row, it crashes almost reliably.

run-webkit-tests svg/webarchive/svg-cursor-subresources.svg svg/webarchive/svg-cursor-subresources.svg

Looks like SVGCursorElement is used after being deleted:

#0	0x0285e004 in WTF::IdentityHashTranslator<WebCore::SVGElement*, WebCore::SVGElement*, WTF::PtrHash<WebCore::SVGElement*> >::equal at HashTable.h:269
#1	0x0285e51a in WTF::HashTable<WebCore::SVGElement*, WebCore::SVGElement*, WTF::IdentityExtractor<WebCore::SVGElement*>, WTF::PtrHash<WebCore::SVGElement*>, WTF::HashTraits<WebCore::SVGElement*>, WTF::HashTraits<WebCore::SVGElement*> >::lookup<WebCore::SVGElement*, WTF::IdentityHashTranslator<WebCore::SVGElement*, WebCore::SVGElement*, WTF::PtrHash<WebCore::SVGElement*> > > at HashTable.h:479
#2	0x0285e59e in WTF::HashTable<WebCore::SVGElement*, WebCore::SVGElement*, WTF::IdentityExtractor<WebCore::SVGElement*>, WTF::PtrHash<WebCore::SVGElement*>, WTF::HashTraits<WebCore::SVGElement*>, WTF::HashTraits<WebCore::SVGElement*> >::find<WebCore::SVGElement*, WTF::IdentityHashTranslator<WebCore::SVGElement*, WebCore::SVGElement*, WTF::PtrHash<WebCore::SVGElement*> > > at HashTable.h:751
#3	0x0285e604 in WTF::HashTable<WebCore::SVGElement*, WebCore::SVGElement*, WTF::IdentityExtractor<WebCore::SVGElement*>, WTF::PtrHash<WebCore::SVGElement*>, WTF::HashTraits<WebCore::SVGElement*>, WTF::HashTraits<WebCore::SVGElement*> >::find at HashTable.h:314
#4	0x02d7a2f8 in WTF::HashSet<WebCore::SVGElement*, WTF::PtrHash<WebCore::SVGElement*>, WTF::HashTraits<WebCore::SVGElement*> >::find at HashSet.h:163
#5	0x02d7a4af in WTF::HashSet<WebCore::SVGElement*, WTF::PtrHash<WebCore::SVGElement*>, WTF::HashTraits<WebCore::SVGElement*> >::remove at HashSet.h:231
#6	0x02d78b90 in WebCore::SVGCursorElement::removeClient at SVGCursorElement.cpp:76
#7	0x0285d39c in WebCore::CSSCursorImageValue::~CSSCursorImageValue at CSSCursorImageValue.cpp:73
#8	0x02856f5c in WTF::RefCounted<WebCore::StyleBase>::deref at RefCounted.h:53
#9	0x028bca36 in WTF::RefPtr<WebCore::CSSValue>::~RefPtr at RefPtr.h:51
#10	0x028bca49 in WTF::RefPtr<WebCore::CSSValue>::~RefPtr at RefPtr.h:51
#11	0x02872e7b in WTF::VectorDestructor<true, WTF::RefPtr<WebCore::CSSValue> >::destruct at Vector.h:54
#12	0x02872ea4 in WTF::VectorTypeOperations<WTF::RefPtr<WebCore::CSSValue> >::destruct at Vector.h:209
#13	0x02872f22 in WTF::Vector<WTF::RefPtr<WebCore::CSSValue>, 0ul>::shrink at Vector.h:656
#14	0x02872f54 in WTF::Vector<WTF::RefPtr<WebCore::CSSValue>, 0ul>::clear at Vector.h:469
#15	0x02872f67 in WTF::Vector<WTF::RefPtr<WebCore::CSSValue>, 0ul>::~Vector at Vector.h:420
#16	0x02872f89 in WTF::Vector<WTF::RefPtr<WebCore::CSSValue>, 0ul>::~Vector at Vector.h:420
#17	0x028cfcd9 in WebCore::CSSValueList::~CSSValueList at CSSValueList.cpp:49
#18	0x02856f5c in WTF::RefCounted<WebCore::StyleBase>::deref at RefCounted.h:53
#19	0x028bca36 in WTF::RefPtr<WebCore::CSSValue>::~RefPtr at RefPtr.h:51
#20	0x028bca49 in WTF::RefPtr<WebCore::CSSValue>::~RefPtr at RefPtr.h:51
#21	0x02838898 in WebCore::CSSProperty::~CSSProperty at CSSProperty.h:32
#22	0x028388ab in WebCore::CSSProperty::~CSSProperty at CSSProperty.h:32
#23	0x02872722 in WebCore::DeprecatedValueListNode<WebCore::CSSProperty>::~DeprecatedValueListNode at DeprecatedValueList.h:36
#24	0x02872735 in WebCore::DeprecatedValueListNode<WebCore::CSSProperty>::~DeprecatedValueListNode at DeprecatedValueList.h:36
#25	0x02874227 in WebCore::DeprecatedValueList<WebCore::CSSProperty>::deleteNode at DeprecatedValueList.h:136
#26	0x02985023 in WebCore::DeprecatedValueListImpl::Private::deleteList at DeprecatedValueListImpl.cpp:108
#27	0x02985b9f in WebCore::DeprecatedValueListImpl::Private::~Private at DeprecatedValueListImpl.cpp:74
#28	0x02985bbd in WebCore::DeprecatedValueListImpl::Private::~Private at DeprecatedValueListImpl.cpp:75
#29	0x02985d4a in WTF::RefCounted<WebCore::DeprecatedValueListImpl::Private>::deref at RefCounted.h:53
#30	0x02985dfb in WTF::RefPtr<WebCore::DeprecatedValueListImpl::Private>::~RefPtr at RefPtr.h:51
#31	0x02985e0f in WTF::RefPtr<WebCore::DeprecatedValueListImpl::Private>::~RefPtr at RefPtr.h:51
#32	0x029852bb in WebCore::DeprecatedValueListImpl::~DeprecatedValueListImpl at DeprecatedValueListImpl.cpp:125
#33	0x029852cf in WebCore::DeprecatedValueListImpl::~DeprecatedValueListImpl at DeprecatedValueListImpl.cpp:125
#34	0x0286e61f in WebCore::DeprecatedValueList<WebCore::CSSProperty>::~DeprecatedValueList at DeprecatedValueList.h:89
#35	0x0286e633 in WebCore::DeprecatedValueList<WebCore::CSSProperty>::~DeprecatedValueList at DeprecatedValueList.h:89
#36	0x02874263 in WebCore::CSSMutableStyleDeclaration::~CSSMutableStyleDeclaration at CSSMutableStyleDeclaration.h:34
#37	0x02856f5c in WTF::RefCounted<WebCore::StyleBase>::deref at RefCounted.h:53
#38	0x02983e7a in WTF::RefPtr<WebCore::CSSMutableStyleDeclaration>::operator= at RefPtr.h:119
#39	0x02e7dc70 in WebCore::StyledElement::destroyInlineStyleDecl at StyledElement.cpp:145
#40	0x02e7e6b0 in WebCore::StyledElement::~StyledElement at StyledElement.cpp:124
#41	0x02d84c18 in WebCore::SVGElement::~SVGElement at SVGElement.cpp:58
#42	0x02e2fb17 in WebCore::SVGStyledElement::~SVGStyledElement at SVGStyledElement.cpp:55
#43	0x02e32091 in WebCore::SVGStyledLocatableElement::~SVGStyledLocatableElement at SVGStyledLocatableElement.cpp:43
#44	0x02e32ed5 in WebCore::SVGStyledTransformableElement::~SVGStyledTransformableElement at SVGStyledTransformableElement.cpp:47
#45	0x02e0a0f8 in WebCore::SVGRectElement::~SVGRectElement at SVGRectElement.cpp:50
#46	0x028fc1d6 in WebCore::ContainerNode::removeAllChildren at ContainerNode.cpp:111
#47	0x02991001 in WebCore::Document::removedLastRef at Document.cpp:376
#48	0x02856d43 in WebCore::TreeShared<WebCore::Node>::deref at TreeShared.h:69
#49	0x028581b7 in WTF::RefPtr<WebCore::Node>::~RefPtr at RefPtr.h:51
#50	0x02e388dd in WTF::RefPtr<WebCore::Node>::~RefPtr at RefPtr.h:51
#51	0x02baa910 in WebCore::JSNode::~JSNode at JSNode.cpp:185
#52	0x02b25c44 in WebCore::JSEventTargetNode::~JSEventTargetNode at JSEventTargetNode.h:39
#53	0x02b554b5 in WebCore::JSDocument::~JSDocument at JSDocument.cpp:235
#54	0x02bcfd34 in WebCore::JSSVGDocument::~JSSVGDocument at JSSVGDocument.h:33
#55	0x02bcfd65 in WebCore::JSSVGDocument::~JSSVGDocument at JSSVGDocument.h:33
#56	0x0032e1fe in KJS::Heap::sweep<(KJS::Heap::HeapType)0> at collector.cpp:910
#57	0x002eaad9 in KJS::Heap::collect at collector.cpp:986
#58	0x02a4ded0 in WebCore::GCController::gcTimerFired at GCController.cpp:72
#59	0x02a4e175 in WebCore::Timer<WebCore::GCController>::fired at Timer.h:99
#60	0x02e97b6e in WebCore::TimerBase::fireTimers at Timer.cpp:347
#61	0x02e97c16 in WebCore::TimerBase::sharedTimerFired at Timer.cpp:368

[Alexey Proskuryakov]

Apparently, crashing or not depends on the order in which GC happens to delete objects.

[Rob Buis]

Created attachment 21958 [details]
First attempt

The problem was that CSSCursorImageValue used getElementById to lookup the stored ids, but the
element pointed to by the id could be already removed from the document. I think this only happens
during GC calls, using removeChild API does remove the element from the id cache in the document.
Cheers,

Rob.

[Alexey Proskuryakov]

Comment on attachment 21958 [details]
First attempt

This may crash if the document happens to be destructed before the element. Also, adding such code to all element destructors will likely affect performance.

Maybe CSSCursorImageValue could check if the document is being destroyed, and avoid getting by id in this case?

+        WARNING: NO TEST CASES ADDED OR CHANGED

I do not think that we need this line in ChangeLog.

[Rob Buis]

Created attachment 22100 [details]
Different check

This is the best check for checking whether the document is still valid I can come up with. I started with document()->renderer() but AFAIK the code should work too when the document has no renderer. A different approach would be to clear the whole element id map before calling removeAllChildren, but I am not sure if that would be right.
Cheers,

Rob.

[Darin Adler]

Comment on attachment 22100 [details]
Different check

This is unnecessarily fragile. While it's true that a document can reach the state where its true refCount is 0 and the only references keeping it a live are the self-only references from its children, it's not at all guaranteed that this state corresponds with the case you care about here. I know you're looking for "the right check" to detect this bad situation, and sadly this is not the one.

I'd like to understand exactly what makes the document "bad to work with" here. Then I can help you figure out the right way to code it.

[Darin Adler]

(In reply to comment #4)
> A different approach would be to
> clear the whole element id map before calling removeAllChildren, but I am not
> sure if that would be right.

I think that would be an acceptable change. But I also think it should be illegal to try to get an element by ID while the document is being torn down. Walking the DOM would also be bad during that time. I think we should to add code to Document to ASSERT that getElementById is not used that way.

The real problem here is that CSSCursorImageValue gets a cursor element by ID multiple times and assumes that it's the same one. There's no guarantee that the same ID will lead to the same element when the CSSCursorImageValue is destroyed and when cachedImage is called as when updateIfSVGCursorIsUsed was called. A new element could have been added earlier in the document with the same ID. In fact, we should construct a test case like this.

The SVGElement needs to use a pointer of some sort to record the SVGCursorElement it got associated with.

I also don't understand how we're guaranteed that the elements in m_referencedElements are all still present when CSSCursorImageValue is destroyed. We can't just store a pointer to an element and hope that it won't be removed!

I also don't see any code to handle the case where the value of CSSCursorImageValue is changed with a setStringValue call. That could change the cursor identifier -- we'd need code to remove it from the old element.

[Alexey Proskuryakov]

I've temporarily disabled the test in r35832.

[Eric Seidel (no email)]

*** Bug 21121 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

*** Bug 21183 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

*** Bug 22006 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

I'm getting one or two crashes on SVG cursor cases each time I run tests, really irritating!

Would anyone disagree with dropping SVG cursor support for now? Fixing it is apparently hard, given that no one could yet.

[Nikolas Zimmermann]

(In reply to comment #11)
> I'm getting one or two crashes on SVG cursor cases each time I run tests,
> really irritating!
> 
> Would anyone disagree with dropping SVG cursor support for now? Fixing it is
> apparently hard, given that no one could yet.
> 

Feel free to disable SVG cursor support for now (maybe wrap in ENABLE(SVG_CURSOR) blocks. I have a fix almost done, though I'm out of time since weeks :( I hope I can work a bit more on it tonight, but if you see persistent crashes, we should disable it until I get around fixing it.

Thanks in advance,
Niko

[Nikolas Zimmermann]

I've made some progress, the new SVG cursor code correctly handles:
- cursor element gets removed -> all affected CSS decls referencing it will be rebuild
- element referencing cursor gets removed -> all affected CSS decls will be rebuild

I've run into a really serious issue, and finally found out why these crashes appeared after 2008-06-25. It's related to changes in the SVGImage code. The CSSCursorImageValue code loads the external cursor data, and stores it as CachedImage. By unknown reasons, an SVGImage object (!) is constructed as well, parsing the SVG file that contains the <cursor> element! So we end up with two representations of the same document interfering.

So <svg><cursor id="foo" xlink:href="foo.png"/> </svg> is parsed _twice_.

I only noticed this, because of setting a breakpoint on SVGCursorElements constructor. I've created a simple testcase, where hovering a <rect> causes a SVG cursor to appear. Clicking on the rect should remove the associated <cursor> element, it's actually deleted but recreated immediately because updating the CSS decls causes the internal SVGImage (which should NEVER exist) to be reparsed (which creates a new cursor element with the same id as the old cursor).

Backtrace:
Breakpoint 2, WebCore::SVGCursorElement::SVGCursorElement (this=0x1bc14ed0, tagName=@0x46488dc, doc=0x69cac00) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/svg/SVGCursorElement.cpp:40
40	    , m_y(this, SVGNames::yAttr, LengthModeHeight)
(gdb) bt
#0  WebCore::SVGCursorElement::SVGCursorElement (this=0x1bc14ed0, tagName=@0x46488dc, doc=0x69cac00) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/svg/SVGCursorElement.cpp:40
#1  0x0388fef5 in WebCore::cursorConstructor (doc=0x69cac00, createdByParser=true) at /Users/nikolaszimmermann/Coding/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/SVGElementFactory.cpp:144
#2  0x03892124 in WebCore::SVGElementFactory::createSVGElement (qName=@0xbfffdc60, doc=0x69cac00, createdByParser=true) at /Users/nikolaszimmermann/Coding/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/SVGElementFactory.cpp:437
#3  0x0348b107 in WebCore::Document::createElement (this=0x69cac00, qName=@0xbfffdc60, createdByParser=true, ec=@0xbfffdc64) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/dom/Document.cpp:746
#4  0x03af5bd6 in WebCore::XMLTokenizer::startElementNs (this=0x1bc10470, xmlLocalName=0x69cda85 "cursor", xmlPrefix=0x0, xmlURI=0x69cda47 "http://www.w3.org/2000/svg", nb_namespaces=0, libxmlNamespaces=0x0, nb_attributes=2, nb_defaulted=0, libxmlAttributes=0x1bc15fd0) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/dom/XMLTokenizerLibxml2.cpp:728
#5  0x03af5f86 in WebCore::startElementNsHandler (closure=0x1bc10860, localname=0x69cda85 "cursor", prefix=0x0, uri=0x69cda47 "http://www.w3.org/2000/svg", nb_namespaces=0, namespaces=0x0, nb_attributes=2, nb_defaulted=0, libxmlAttributes=0x1bc15fd0) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/dom/XMLTokenizerLibxml2.cpp:980
#6  0x9010219a in xmlIOParseDTD ()
#7  0x900daf08 in xmlParseChunk ()
#8  0x03af3569 in WebCore::XMLTokenizer::doWrite (this=0x1bc10470, parseString=@0xbfffdf5c) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/dom/XMLTokenizerLibxml2.cpp:629
#9  0x0396b01c in WebCore::XMLTokenizer::write (this=0x1bc10470, s=@0xbfffdfb4) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/dom/XMLTokenizer.cpp:119
#10 0x03531a69 in WebCore::FrameLoader::write (this=0x69bd824, str=0x68c5600 "<svg xmlns=\"http://www.w3.org/2000/svg\" xmlns:xlink=\"http://www.w3.org/1999/xlink\">\n  <cursor id=\"mycursor\" xlink:href=\"resources/green-checker.png\" />\n  <rect style=\"cursor: url(#mycursor)\" width=\"10"..., len=440, flush=false) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/loader/FrameLoader.cpp:1040
#11 0x038a07a6 in WebCore::SVGImage::dataChanged (this=0x1bc08370, allDataReceived=true) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/svg/graphics/SVGImage.cpp:212
#12 0x035edd39 in WebCore::Image::setData (this=0x1bc08370, data=@0xbfffe1f0, allDataReceived=true) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/platform/graphics/Image.cpp:79
#13 0x033758ff in WebCore::CachedImage::data (this=0x1bc0e670, data=@0xbfffe26c, allDataReceived=true) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/loader/CachedImage.cpp:263
#14 0x039b8409 in WebCore::Loader::Host::didFinishLoading (this=0x6880230, loader=0x69bf800) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/loader/loader.cpp:300
#15 0x03933283 in WebCore::SubresourceLoader::didFinishLoading (this=0x69bf800) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/loader/SubresourceLoader.cpp:194
#16 0x03856a98 in WebCore::ResourceLoader::didFinishLoading (this=0x69bf800) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/loader/ResourceLoader.cpp:398
#17 0x038545a2 in -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] (self=0x1bc0d790, _cmd=0x91541564, con=0x1bc0d7b0) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/platform/network/mac/ResourceHandleMac.mm:565

Very very weird situation. I bet this was not the case when I initially wrote the code. I do remember there was no SVGImage involved in it. Needs furhter investigations, just wanted to let others know what's going on.

[Alexey Proskuryakov]

This was fixed in <http://trac.webkit.org/projects/webkit/changeset/39149> by Sam Weinig: "We did not fix the design that resulted in this issue, we just fixed the pointer lifetimes." Test re-enabled in <http://trac.webkit.org/changeset/39165>.

[David Kilzer (:ddkilzer)]

See also:
<rdar://problem/6421988>
<rdar://problem/6422015>