Crash on refresh when using SVG as background image Simpler SVGs seem to work fine in the layout tests, but this particular SVG (when used as a background image) crashes safari on page refresh. It looks like possibly SVGImage should be keeping a reference to the SVGDocument and is not, or maybe it is keeping a reference when it shouldn't be. Either way, something subtle is wrong here with the SVGImage implementation to cause this.
Created attachment 12529 [details] simple test case (crashes safari)
Date/Time: 2007-01-18 05:41:37.724 -0800 OS Version: 10.4.8 (Build 8L2127) Report Version: 4 Command: Safari Path: /Applications/Safari.app/Contents/MacOS/Safari Parent: zsh [2484] Version: 2.0.4 (419.3) Build Version: 2 Project Name: WebBrowser Source Version: 4190300 PID: 8297 Thread: 0 Exception: EXC_BAD_ACCESS (0x0001) Codes: KERN_INVALID_ADDRESS (0x0001) at 0x17ec083f Thread 0 Crashed: 0 com.apple.WebCore 0x010f8b66 WebCore::ContainerNode::removeAllChildren() + 232 (ContainerNode.cpp:89) 1 com.apple.WebCore 0x010f8c51 WebCore::ContainerNode::~ContainerNode [not-in-charge]() + 55 (ContainerNode.cpp:114) 2 com.apple.WebCore 0x010f3bda WebCore::Document::~Document [not-in-charge]() + 1550 (Document.cpp:427) 3 com.apple.WebCore 0x0105f5cb WebCore::SVGDocument::~SVGDocument [in-charge deleting]() + 55 (SVGDocument.cpp:43) 4 com.apple.WebCore 0x014f1175 WebCore::Document::selfOnlyDeref() + 97 (Document.h:152) 5 com.apple.WebCore 0x014f19e3 WebCore::DocPtr<WebCore::Document>::~DocPtr [in-charge]() + 31 (DocPtr.h:33) 6 com.apple.WebCore 0x01244385 WebCore::Node::~Node [not-in-charge]() + 191 (Node.cpp:154) 7 com.apple.WebCore 0x01228cbc WebCore::EventTargetNode::~EventTargetNode [not-in-charge]() + 204 (EventTargetNode.cpp:72) 8 com.apple.WebCore 0x0120c4bb WebCore::CharacterData::~CharacterData [not-in-charge]() + 79 (CharacterData.cpp:54) 9 com.apple.WebCore 0x0120d6a1 WebCore::Text::~Text [in-charge deleting]() + 55 (Text.cpp:53) 10 com.apple.WebCore 0x010f8be1 WebCore::ContainerNode::removeAllChildren() + 355 (ContainerNode.cpp:94) 11 com.apple.WebCore 0x010f3cb8 WebCore::Document::removedLastRef() + 196 (Document.cpp:369) 12 com.apple.WebCore 0x0149758d WebCore::TreeShared<WebCore::Node>::deref() + 77 (Shared.h:83) 13 com.apple.WebCore 0x014ad844 WTF::RefPtr<WebCore::Document>::operator=(WebCore::Document*) + 56 (RefPtr.h:107) 14 com.apple.WebCore 0x013935dc WebCore::FrameLoader::clear(bool) + 366 (FrameLoader.cpp:737) 15 com.apple.WebCore 0x01398fbd WebCore::FrameLoader::begin(WebCore::KURL const&) + 61 (FrameLoader.cpp:796) 16 com.apple.WebCore 0x013994bf WebCore::FrameLoader::receivedFirstData() + 39 (FrameLoader.cpp:755) 17 com.apple.WebCore 0x0139969f WebCore::FrameLoader::setEncoding(WebCore::String const&, bool) + 45 (FrameLoader.cpp:1486) 18 com.apple.WebCore 0x0110043c -[WebCoreFrameBridge receivedData:textEncodingName:] + 220 (WebCoreFrameBridge.mm:1584) 19 com.apple.WebKit 0x00332681 -[WebHTMLRepresentation receivedData:withDataSource:] + 199 (WebHTMLRepresentation.mm:174) 20 com.apple.WebKit 0x0032de1b -[WebDataSource(WebInternal) _receivedData:] + 89 (WebDataSource.mm:178) 21 com.apple.WebKit 0x00394647 WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int) + 127 (WebFrameLoaderClient.mm:636) 22 com.apple.WebCore 0x0138fedf WebCore::FrameLoader::committedLoad(WebCore::DocumentLoader*, char const*, int) + 53 (FrameLoader.cpp:2922) 23 com.apple.WebCore 0x0139ff35 WebCore::DocumentLoader::commitLoad(char const*, int) + 87 (DocumentLoader.cpp:329) 24 com.apple.WebCore 0x0139ff8e WebCore::DocumentLoader::receivedData(char const*, int) + 76 (DocumentLoader.cpp:342) 25 com.apple.WebCore 0x0138f35b WebCore::FrameLoader::receivedData(char const*, int) + 41 (FrameLoader.cpp:1887) 26 com.apple.WebCore 0x013a11a2 WebCore::MainResourceLoader::addData(char const*, int, bool) + 80 (MainResourceLoader.cpp:135) 27 com.apple.WebCore 0x013a3021 WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool) + 83 28 com.apple.WebCore 0x013a14d7 WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool) + 281 (MainResourceLoader.cpp:304) 29 com.apple.WebCore 0x013a2cce WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int) + 58 30 com.apple.WebCore 0x01382c8c -[WebCoreResourceHandleAsDelegate connection:didReceiveData:lengthReceived:] + 172 (ResourceHandleMac.mm:350) 31 com.apple.Foundation 0x9265bb86 -[NSURLConnection(NSURLConnectionInternal) _sendDidReceiveDataCallback] + 641 32 com.apple.Foundation 0x92659e67 -[NSURLConnection(NSURLConnectionInternal) _sendCallbacks] + 686 33 com.apple.Foundation 0x92659b41 _sendCallbacks + 201 34 com.apple.CoreFoundation 0x90829379 CFRunLoopRunSpecific + 1213 35 com.apple.CoreFoundation 0x90828eb5 CFRunLoopRunInMode + 61 36 com.apple.HIToolbox 0x92dcdb90 RunCurrentEventLoopInMode + 285 37 com.apple.HIToolbox 0x92dcd1ce ReceiveNextEventCommon + 184 38 com.apple.HIToolbox 0x92dcd0ee BlockUntilNextEventMatchingListInMode + 81 39 com.apple.AppKit 0x9326f465 _DPSNextEvent + 572 40 com.apple.AppKit 0x9326f056 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 137 41 com.apple.Safari 0x00006f96 0x1000 + 24470 42 com.apple.AppKit 0x93268ddb -[NSApplication run] + 512 43 com.apple.AppKit 0x9325cd2f NSApplicationMain + 573 44 com.apple.Safari 0x0005f7de 0x1000 + 387038 45 com.apple.Safari 0x0005f6f9 0x1000 + 386809 Thread 1: 0 libSystem.B.dylib 0x90009857 mach_msg_trap + 7 1 com.unsanity.ape 0xc0001db2 __ape_agent + 307 2 libSystem.B.dylib 0x90023d87 _pthread_body + 84 Thread 2: 0 libSystem.B.dylib 0x90019d3c select + 12 1 libSystem.B.dylib 0x90023d87 _pthread_body + 84 Thread 3: 0 libSystem.B.dylib 0x90024427 semaphore_wait_signal_trap + 7 1 com.apple.Foundation 0x9264b2f8 -[NSConditionLock lockWhenCondition:] + 39 2 com.apple.Syndication 0x9a40e052 -[AsyncDB _run:] + 181 3 com.apple.Foundation 0x925f536c forkThreadForFunction + 123 4 libSystem.B.dylib 0x90023d87 _pthread_body + 84 Thread 4: 0 libSystem.B.dylib 0x90009857 mach_msg_trap + 7 1 com.apple.CoreFoundation 0x9082969a CFRunLoopRunSpecific + 2014 2 com.apple.CoreFoundation 0x90828eb5 CFRunLoopRunInMode + 61 3 com.apple.Foundation 0x9262aa9b +[NSURLConnection(NSURLConnectionInternal) _resourceLoadLoop:] + 259 4 com.apple.Foundation 0x925f536c forkThreadForFunction + 123 5 libSystem.B.dylib 0x90023d87 _pthread_body + 84 Thread 5: 0 libSystem.B.dylib 0x90009857 mach_msg_trap + 7 1 com.apple.CoreFoundation 0x9082969a CFRunLoopRunSpecific + 2014 2 com.apple.CoreFoundation 0x90828eb5 CFRunLoopRunInMode + 61 3 com.apple.Foundation 0x92651c4e +[NSURLCache _diskCacheSyncLoop:] + 206 4 com.apple.Foundation 0x925f536c forkThreadForFunction + 123 5 libSystem.B.dylib 0x90023d87 _pthread_body + 84 Thread 0 crashed with X86 Thread State (32-bit): eax: 0x17ec083f ebx: 0x010f8a8a ecx: 0x0149982f edx: 0x00000000 edi: 0x013a2c94 esi: 0x00000000 ebp: 0xbfffdc88 esp: 0xbfffdc60 ss: 0x0000001f efl: 0x00010206 eip: 0x010f8b66 cs: 0x00000017 ds: 0x0000001f es: 0x0000001f fs: 0x00000000 gs: 0x00000037
The line of code it crashes on is: } else if (n->inDocument()) n->removedFromDocument(); // crashes here
Actually, it seems that the loader might be holding on to the SVGDocument longer than it should be. Possibly there is a pointer that I'm not clearing properly in ~SVGImage
My guess is that somehow WebKit has not been told properly to forget about the SVGImage's frameLoader. It seems to be re-using the loader for another document, mistakenly, and tearing down the SVGDocument at a bad time (possibly after it's already been torn down). I'm really not sure. Bradee or Maciej would have to comment here.
<rdar://problem/4944768>
When you plug the SVG image leak report in DRT, this crash happens for all SVG images. The crash occurs because n is a pointer to a deallocated Node. I suspect that inDocument() is a red herring.
Seems to crash in the exact same place under gmalloc.
gmalloc is not needed. The crash is the same without it. Finally found the damn bug. removeAllChildren uses static variables (node lists and a bool) which are not safe across documents. the "isTopLevel" bool won't get set right for the second-level document, thus its nodes will get destroyed at the wrong time. http://paste.lisp.org/display/36465#6 That has an assert which demonstrates the problem. The fix is unclear. The bug comes because the RenderStyle in the HTMLDocument, references the CachedObject (CachedImage) which holds the SVGImage, when the RenderStyle goes away, the CacheObject is released, and the SVGDocument is destroyed while the HTMLDocument is also in the middle of destruction, and the above mentioned bug is hit.
Created attachment 12966 [details] a patch which reveals the bug I'm too tired to fix the bug tonight. But this is a patch which demonstrates exactly what's wrong. I also moved the m_firstChild and m_lastChild nulling, as it was wrong do do them last.
The root cause here is the use of static variables in ContainerNode to detect recursion, and to hold the linked list of nodes to be destroyed. But that is wrong in the case where destroying a child node may lead to destroying a whole separate document. The algorithm should be rewritten to work iteratively instead of recursively - it can do that by delinking child nodes that are to be destroyed in the original loop, before going around calling delete.
We decided to disable SVG image support for now, since it is insufficiently tested. That makes this no longer a P1.
Created attachment 16513 [details] (small) fix, test cases and results
Comment on attachment 16513 [details] (small) fix, test cases and results Maciej is really the one who should look at this.
Created attachment 16527 [details] Final patch with mjs's suggested changes mjs said he would perf test this, and then I'll land.
Created attachment 16528 [details] final patch
Comment on attachment 16528 [details] final patch Looks good to me. I also tested performance and it actually looked like a slight speed boost (1-2%) on HTML iBench, but that could have been a measurement error. Please don't commit the WebCore.xcodeproj change, but otherwise r=me for feature-branch.
Landed as r26042 on the feature branch. SVGs actually now kinda-work as background images! (The SVG must have an absolute size and the div/background must have non-zero intrinsic size).