Bug 12310 – Crash on refresh when using SVG as background image

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Created attachment 12529 [details]
simple test case (crashes safari)

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

The line of code it crashes on is:

        } else if (n->inDocument())
            n->removedFromDocument(); // crashes here

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Mark Rowe (bdash)]

<rdar://problem/4944768>

[Geoffrey Garen]

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.

[Eric Seidel (no email)]

Seems to crash in the exact same place under gmalloc.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Maciej Stachowiak]

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.

[Maciej Stachowiak]

We decided to disable SVG image support for now, since it is insufficiently tested. That makes this no longer a P1.

[Eric Seidel (no email)]

Created attachment 16513 [details]
(small) fix, test cases and results

[Eric Seidel (no email)]

Comment on attachment 16513 [details]
(small) fix, test cases and results

Maciej is really the one who should look at this.

[Eric Seidel (no email)]

Created attachment 16527 [details]
Final patch with mjs's suggested changes

mjs said he would perf test this, and then I'll land.

[Eric Seidel (no email)]

Created attachment 16528 [details]
final patch

[Maciej Stachowiak]

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.

[Eric Seidel (no email)]

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).