Bug 18803 - CRASH: ContainerNode::willRemove() called on deleted node
Summary: CRASH: ContainerNode::willRemove() called on deleted node
Status: RESOLVED DUPLICATE of bug 15707
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 525.x (Safari 3.1)
Hardware: PC OS X 10.5
: P1 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords: HasReduction, InRadar
Depends on:
Blocks:
 
Reported: 2008-04-29 13:56 PDT by Eric Seidel (no email)
Modified: 2019-02-06 09:04 PST (History)
4 users (show)

See Also:


Attachments
multi-file reduced test case (will crash Safari!) (10.82 KB, application/x-compressed)
2008-04-29 13:58 PDT, Eric Seidel (no email)
no flags Details
First pass fix (5.19 KB, patch)
2008-04-30 12:37 PDT, Eric Seidel (no email)
darin: review-
Details | Formatted Diff | Diff
Fix updated to apply cleanly to ToT (5.39 KB, patch)
2008-11-18 10:20 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2008-04-29 13:56:45 PDT
I haven't looked into detail as to what's causing the crash, but I have created a reduction (probably could be even smaller).

This is a reduced copy of HTMLFrameElement09.html which started crashing for me when opened it after leaving a debugging alert in selfhtml.js (an overly complicated support file provided by the w3c).  That alert caused my copy of Safari to crash, and I reduced it to this...
Comment 1 Eric Seidel (no email) 2008-04-29 13:58:26 PDT
Created attachment 20892 [details]
multi-file reduced test case (will crash Safari!)
Comment 2 Eric Seidel (no email) 2008-04-29 14:00:38 PDT
function loadComplete() {
        // loadComplete is called by each subframe as it's loaded (as well as the main document)
	// I think this was an error on the author's part.
	alert("Geronimo!");  // CAUSES CRASH IN SAFARI
	var testNode = document.getElementById("Frame1");
	document.open();
}
Comment 3 Eric Seidel (no email) 2008-04-29 15:13:19 PDT
I don't see this on my mac system, but happens ever time on my windows box.  I haven't stopped it in the debugger.  It's probably because I still have heap checking on for Safari from long ago.
Comment 4 Eric Seidel (no email) 2008-04-29 16:46:57 PDT
If you run Safari under MallocScribble (on the mac), it's very easy to reproduce this given the test case.

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x0000000055555555
Crashed Thread:  0

Thread 0 Crashed:
0   com.apple.WebCore             	0x025fb4da WebCore::ContainerNode::willRemove() + 20 (ContainerNode.cpp:347)
1   com.apple.WebCore             	0x02775ee7 WebCore::HTMLFrameOwnerElement::willRemove() + 67 (HTMLFrameOwnerElement.cpp:50)
2   com.apple.WebCore             	0x025fb4eb WebCore::ContainerNode::willRemove() + 37 (ContainerNode.cpp:346)
3   com.apple.WebCore             	0x025fb4eb WebCore::ContainerNode::willRemove() + 37 (ContainerNode.cpp:346)
4   com.apple.WebCore             	0x025fcc4f WebCore::willRemoveChild(WebCore::Node*) + 77 (ContainerNode.cpp:363)
5   com.apple.WebCore             	0x025fcc8c WebCore::ContainerNode::removeChildren() + 48 (ContainerNode.cpp:464)
6   com.apple.WebCore             	0x026896ea WebCore::Document::clear() + 70 (Document.cpp:1702)
7   com.apple.WebCore             	0x0268f246 WebCore::Document::implicitOpen() + 28 (Document.cpp:1438)
8   com.apple.WebCore             	0x0269251f WebCore::Document::open() + 379 (Document.cpp:1416)
9   com.apple.WebCore             	0x02858355 WebCore::JSHTMLDocument::open(KJS::ExecState*, KJS::List const&) + 301 (JSHTMLDocumentCustom.cpp:116)
10  com.apple.WebCore             	0x02856c3c WebCore::jsHTMLDocumentPrototypeFunctionOpen(KJS::ExecState*, KJS::JSObject*, KJS::List const&) + 96 (JSHTMLDocument.cpp:276)
11  com.apple.JavaScriptCore      	0x0044dd5c KJS::PrototypeFunction::callAsFunction(KJS::ExecState*, KJS::JSObject*, KJS::List const&) + 34 (function.cpp:906)
12  com.apple.JavaScriptCore      	0x0046fffa KJS::JSObject::call(KJS::ExecState*, KJS::JSObject*, KJS::List const&) + 222 (object.cpp:99)
13  com.apple.JavaScriptCore      	0x004d2ad6 KJS::FunctionCallDotNode::inlineEvaluate(KJS::ExecState*) + 802 (nodes.cpp:1495)
14  com.apple.JavaScriptCore      	0x00486a4a KJS::FunctionCallDotNode::evaluate(KJS::ExecState*) + 30 (nodes.cpp:1501)
15  com.apple.JavaScriptCore      	0x00477559 KJS::ExprStatementNode::execute(KJS::ExecState*) + 43 (nodes.cpp:3993)
16  com.apple.JavaScriptCore      	0x00459a5b KJS::statementListExecute(WTF::Vector<WTF::RefPtr<KJS::StatementNode>, 0ul>&, KJS::ExecState*) + 85 (nodes.cpp:3946)
17  com.apple.JavaScriptCore      	0x00459ae8 KJS::BlockNode::execute(KJS::ExecState*) + 26 (nodes.cpp:3972)
18  com.apple.JavaScriptCore      	0x00467cb2 KJS::FunctionBodyNode::execute(KJS::ExecState*) + 34 (nodes.cpp:4891)
19  com.apple.JavaScriptCore      	0x004684bc KJS::FunctionImp::callAsFunction(KJS::ExecState*, KJS::JSObject*, KJS::List const&) + 134 (function.cpp:78)
20  com.apple.JavaScriptCore      	0x0046fffa KJS::JSObject::call(KJS::ExecState*, KJS::JSObject*, KJS::List const&) + 222 (object.cpp:99)
21  com.apple.JavaScriptCore      	0x004d2ad6 KJS::FunctionCallDotNode::inlineEvaluate(KJS::ExecState*) + 802 (nodes.cpp:1495)
22  com.apple.JavaScriptCore      	0x00486a4a KJS::FunctionCallDotNode::evaluate(KJS::ExecState*) + 30 (nodes.cpp:1501)
23  com.apple.JavaScriptCore      	0x00477559 KJS::ExprStatementNode::execute(KJS::ExecState*) + 43 (nodes.cpp:3993)
24  com.apple.JavaScriptCore      	0x00459a5b KJS::statementListExecute(WTF::Vector<WTF::RefPtr<KJS::StatementNode>, 0ul>&, KJS::ExecState*) + 85 (nodes.cpp:3946)
25  com.apple.JavaScriptCore      	0x00459ae8 KJS::BlockNode::execute(KJS::ExecState*) + 26 (nodes.cpp:3972)
26  com.apple.JavaScriptCore      	0x00467cb2 KJS::FunctionBodyNode::execute(KJS::ExecState*) + 34 (nodes.cpp:4891)
27  com.apple.JavaScriptCore      	0x004684bc KJS::FunctionImp::callAsFunction(KJS::ExecState*, KJS::JSObject*, KJS::List const&) + 134 (function.cpp:78)
28  com.apple.JavaScriptCore      	0x0046fffa KJS::JSObject::call(KJS::ExecState*, KJS::JSObject*, KJS::List const&) + 222 (object.cpp:99)
29  com.apple.WebCore             	0x02bd922e WebCore::JSAbstractEventListener::handleEvent(WebCore::Event*, bool) + 670 (kjs_events.cpp:100)
30  com.apple.WebCore             	0x02689c09 WebCore::Document::handleWindowEvent(WebCore::Event*, bool) + 281 (Document.cpp:2614)
31  com.apple.WebCore             	0x026e9964 WebCore::EventTargetNode::dispatchWindowEvent(WebCore::AtomicString const&, bool, bool) + 332 (EventTargetNode.cpp:149)
32  com.apple.WebCore             	0x0268ef43 WebCore::Document::implicitClose() + 675 (Document.cpp:1550)
33  com.apple.WebCore             	0x02721b5c WebCore::FrameLoader::checkCallImplicitClose() + 226 (FrameLoader.cpp:1330)
34  com.apple.WebCore             	0x0272da2a WebCore::FrameLoader::checkCompleted() + 268 (FrameLoader.cpp:1285)
35  com.apple.WebCore             	0x0272d330 WebCore::FrameLoader::completed() + 148 (FrameLoader.cpp:1996)
36  com.apple.WebCore             	0x0272da80 WebCore::FrameLoader::checkCompleted() + 354 (FrameLoader.cpp:1289)
37  com.apple.WebCore             	0x0273049a WebCore::FrameLoader::finishedParsing() + 90 (FrameLoader.cpp:1233)
38  com.apple.WebCore             	0x0268cf64 WebCore::Document::finishedParsing() + 204 (Document.cpp:3699)
39  com.apple.WebCore             	0x027e882f WebCore::ImageTokenizer::finish() + 577 (ImageDocument.cpp:129)
40  com.apple.WebCore             	0x02687122 WebCore::Document::finishParsing() + 40 (Document.cpp:1693)
41  com.apple.WebCore             	0x0272dbee WebCore::FrameLoader::endIfNotLoadingMainResource() + 118 (FrameLoader.cpp:1060)
42  com.apple.WebCore             	0x0272dc23 WebCore::FrameLoader::end() + 27 (FrameLoader.cpp:1045)
43  com.apple.WebCore             	0x026b3488 WebCore::DocumentLoader::finishedLoading() + 76 (DocumentLoader.cpp:337)
44  com.apple.WebCore             	0x02728d56 WebCore::FrameLoader::finishedLoading() + 72 (FrameLoader.cpp:2893)
45  com.apple.WebCore             	0x0291aedb WebCore::MainResourceLoader::didFinishLoading() + 207 (MainResourceLoader.cpp:320)
46  com.apple.WebCore             	0x02a2f290 WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*) + 24 (ResourceLoader.cpp:390)
47  com.apple.WebCore             	0x02a2c9f5 -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] + 101 (ResourceHandleMac.mm:521)
48  com.apple.Foundation          	0x94c8e8b7 -[NSURLConnection(NSURLConnectionReallyInternal) sendDidFinishLoading] + 87
49  com.apple.Foundation          	0x94c8e844 _NSURLConnectionDidFinishLoading + 68
50  com.apple.CFNetwork           	0x9059a7f3 sendDidFinishLoadingCallback + 148
51  com.apple.CFNetwork           	0x90597920 _CFURLConnectionSendCallbacks + 1994
52  com.apple.CFNetwork           	0x905970d9 muxerSourcePerform + 283
53  com.apple.CoreFoundation      	0x90b2562e CFRunLoopRunSpecific + 3166
54  com.apple.CoreFoundation      	0x90b25d18 CFRunLoopRunInMode + 88
55  com.apple.HIToolbox           	0x926296a0 RunCurrentEventLoopInMode + 283
56  com.apple.HIToolbox           	0x926294b9 ReceiveNextEventCommon + 374
57  com.apple.HIToolbox           	0x9262932d BlockUntilNextEventMatchingListInMode + 106
58  com.apple.AppKit              	0x90c3f7d9 _DPSNextEvent + 657
59  com.apple.AppKit              	0x90c3f08e -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 128
60  com.apple.Safari              	0x00007f2e 0x1000 + 28462
61  com.apple.AppKit              	0x90c380c5 -[NSApplication run] + 795
62  com.apple.AppKit              	0x90c0530a NSApplicationMain + 574
63  com.apple.Safari              	0x000b9906 0x1000 + 755974

Comment 5 Eric Seidel (no email) 2008-04-30 11:04:03 PDT
Ok, I stopped this in the debugger for DRT.  And have a better stack trace.  We're re-entering ContainerNode::removeChildren(), this is similar to a bug we fixed a year ago in ContainerNode::removeAllChildren().

In this case, we're re-entering JS and running a second on-load handler while we're running the first.  I doubt that's what we intended...  Probably we should have been in a forbid event dispatch block?

I'd be curious to hear other's thoughts.

#0	0x025fcc55 in WebCore::ContainerNode::removeChildren at ContainerNode.cpp:466
#1	0x026896ca in WebCore::Document::clear at Document.cpp:1700
#2	0x0268f226 in WebCore::Document::implicitOpen at Document.cpp:1437
#3	0x026924ff in WebCore::Document::open at Document.cpp:1414
#4	0x02858335 in WebCore::JSHTMLDocument::open at JSHTMLDocumentCustom.cpp:115
#5	0x02856c1c in WebCore::jsHTMLDocumentPrototypeFunctionOpen at JSHTMLDocument.cpp:276
#6	0x00271d5c in KJS::PrototypeFunction::callAsFunction at function.cpp:905
#7	0x00293ffa in KJS::JSObject::call at object.cpp:99
#8	0x002f6ad6 in KJS::FunctionCallDotNode::inlineEvaluate at nodes.cpp:1495
#9	0x002aaa4a in KJS::FunctionCallDotNode::evaluate at nodes.cpp:1500
#10	0x0029b559 in KJS::ExprStatementNode::execute at nodes.cpp:3993
#11	0x0027da5b in statementListExecute at nodes.cpp:3946
#12	0x0027dae8 in KJS::BlockNode::execute at nodes.cpp:3971
#13	0x0028bcb2 in KJS::FunctionBodyNode::execute at nodes.cpp:4890
#14	0x0028c4bc in KJS::FunctionImp::callAsFunction at function.cpp:78
#15	0x00293ffa in KJS::JSObject::call at object.cpp:99
#16	0x002f6ad6 in KJS::FunctionCallDotNode::inlineEvaluate at nodes.cpp:1495
#17	0x002aaa4a in KJS::FunctionCallDotNode::evaluate at nodes.cpp:1500
#18	0x0029b559 in KJS::ExprStatementNode::execute at nodes.cpp:3993
#19	0x0027da5b in statementListExecute at nodes.cpp:3946
#20	0x0027dae8 in KJS::BlockNode::execute at nodes.cpp:3971
#21	0x0028bcb2 in KJS::FunctionBodyNode::execute at nodes.cpp:4890
#22	0x0028c4bc in KJS::FunctionImp::callAsFunction at function.cpp:78
#23	0x00293ffa in KJS::JSObject::call at object.cpp:99
#24	0x02bd920e in WebCore::JSAbstractEventListener::handleEvent at kjs_events.cpp:100
#25	0x02689be9 in WebCore::Document::handleWindowEvent at Document.cpp:2616
#26	0x026e9944 in WebCore::EventTargetNode::dispatchWindowEvent at EventTargetNode.cpp:147
#27	0x0268ef23 in WebCore::Document::implicitClose at Document.cpp:1549
#28	0x02721b3c in WebCore::FrameLoader::checkCallImplicitClose at FrameLoader.cpp:1329
#29	0x0272da0a in WebCore::FrameLoader::checkCompleted at FrameLoader.cpp:1281
#30	0x0272d310 in WebCore::FrameLoader::completed at FrameLoader.cpp:1995
#31	0x0272da60 in WebCore::FrameLoader::checkCompleted at FrameLoader.cpp:1288 // NOW WE'RE IN TROUBLE...
#32	0x0272dade in WebCore::FrameLoader::mainReceivedCompleteError at FrameLoader.cpp:4583
#33	0x026b2e21 in WebCore::DocumentLoader::mainReceivedError at DocumentLoader.cpp:261
#34	0x0272e095 in WebCore::FrameLoader::receivedMainResourceError at FrameLoader.cpp:3506
#35	0x0291ab96 in WebCore::MainResourceLoader::didCancel at MainResourceLoader.cpp:102
#36	0x02a2f6e6 in WebCore::ResourceLoader::cancel at ResourceLoader.cpp:349
#37	0x02a2f721 in WebCore::ResourceLoader::cancel at ResourceLoader.cpp:339
#38	0x026b2f28 in WebCore::DocumentLoader::stopLoading at DocumentLoader.cpp:299
#39	0x027281dc in WebCore::FrameLoader::stopAllLoaders at FrameLoader.cpp:2488
#40	0x0272860d in WebCore::FrameLoader::stopLoadingSubframes at FrameLoader.cpp:2473
#41	0x027281ae in WebCore::FrameLoader::stopAllLoaders at FrameLoader.cpp:2486
#42	0x02730e49 in WebCore::FrameLoader::frameDetached at FrameLoader.cpp:3294
#43	0x02775ebc in WebCore::HTMLFrameOwnerElement::willRemove at HTMLFrameOwnerElement.cpp:46 // HERE WE GO....
#44	0x025fb4ab in WebCore::ContainerNode::willRemove at ContainerNode.cpp:347
#45	0x025fb4ab in WebCore::ContainerNode::willRemove at ContainerNode.cpp:347
#46	0x025fcc0f in willRemoveChild at ContainerNode.cpp:361
#47	0x025fcc68 in WebCore::ContainerNode::removeChildren at ContainerNode.cpp:467
#48	0x026896ca in WebCore::Document::clear at Document.cpp:1700
#49	0x0268f226 in WebCore::Document::implicitOpen at Document.cpp:1437
#50	0x026924ff in WebCore::Document::open at Document.cpp:1414
#51	0x02858335 in WebCore::JSHTMLDocument::open at JSHTMLDocumentCustom.cpp:115
#52	0x02856c1c in WebCore::jsHTMLDocumentPrototypeFunctionOpen at JSHTMLDocument.cpp:276
#53	0x00271d5c in KJS::PrototypeFunction::callAsFunction at function.cpp:905
#54	0x00293ffa in KJS::JSObject::call at object.cpp:99
#55	0x002f6ad6 in KJS::FunctionCallDotNode::inlineEvaluate at nodes.cpp:1495
#56	0x002aaa4a in KJS::FunctionCallDotNode::evaluate at nodes.cpp:1500
#57	0x0029b559 in KJS::ExprStatementNode::execute at nodes.cpp:3993
#58	0x0027da5b in statementListExecute at nodes.cpp:3946
#59	0x0027dae8 in KJS::BlockNode::execute at nodes.cpp:3971
#60	0x0028bcb2 in KJS::FunctionBodyNode::execute at nodes.cpp:4890
#61	0x0028c4bc in KJS::FunctionImp::callAsFunction at function.cpp:78
#62	0x00293ffa in KJS::JSObject::call at object.cpp:99
#63	0x002f6ad6 in KJS::FunctionCallDotNode::inlineEvaluate at nodes.cpp:1495
#64	0x002aaa4a in KJS::FunctionCallDotNode::evaluate at nodes.cpp:1500
#65	0x0029b559 in KJS::ExprStatementNode::execute at nodes.cpp:3993
#66	0x0027da5b in statementListExecute at nodes.cpp:3946
#67	0x0027dae8 in KJS::BlockNode::execute at nodes.cpp:3971
#68	0x0028bcb2 in KJS::FunctionBodyNode::execute at nodes.cpp:4890
#69	0x0028c4bc in KJS::FunctionImp::callAsFunction at function.cpp:78
#70	0x00293ffa in KJS::JSObject::call at object.cpp:99
#71	0x02bd920e in WebCore::JSAbstractEventListener::handleEvent at kjs_events.cpp:100
#72	0x02689be9 in WebCore::Document::handleWindowEvent at Document.cpp:2616
#73	0x026e9944 in WebCore::EventTargetNode::dispatchWindowEvent at EventTargetNode.cpp:147
#74	0x0268ef23 in WebCore::Document::implicitClose at Document.cpp:1549
#75	0x02721b3c in WebCore::FrameLoader::checkCallImplicitClose at FrameLoader.cpp:1329
#76	0x0272da0a in WebCore::FrameLoader::checkCompleted at FrameLoader.cpp:1281
#77	0x0272d310 in WebCore::FrameLoader::completed at FrameLoader.cpp:1995
#78	0x0272da60 in WebCore::FrameLoader::checkCompleted at FrameLoader.cpp:1288
#79	0x0273047a in WebCore::FrameLoader::finishedParsing at FrameLoader.cpp:1231
#80	0x0268cf44 in WebCore::Document::finishedParsing at Document.cpp:3698
#81	0x027e880f in WebCore::ImageTokenizer::finish at ImageDocument.cpp:128
#82	0x02687102 in WebCore::Document::finishParsing at Document.cpp:1692
#83	0x0272dbce in WebCore::FrameLoader::endIfNotLoadingMainResource at FrameLoader.cpp:1060
#84	0x0272dc03 in WebCore::FrameLoader::end at FrameLoader.cpp:1044
#85	0x026b3468 in WebCore::DocumentLoader::finishedLoading at DocumentLoader.cpp:335
#86	0x02728d36 in WebCore::FrameLoader::finishedLoading at FrameLoader.cpp:2892
#87	0x0291aebb in WebCore::MainResourceLoader::didFinishLoading at MainResourceLoader.cpp:319
#88	0x02a2f270 in WebCore::ResourceLoader::didFinishLoading at ResourceLoader.cpp:389
#89	0x02a2c9d5 in -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] at ResourceHandleMac.mm:521
#90	0x94c8e8b7 in -[NSURLConnection(NSURLConnectionReallyInternal) sendDidFinishLoading]
#91	0x94c8e844 in _NSURLConnectionDidFinishLoading
#92	0x9059a7f3 in sendDidFinishLoadingCallback
#93	0x90597920 in _CFURLConnectionSendCallbacks
#94	0x905970d9 in muxerSourcePerform
#95	0x90b2562e in CFRunLoopRunSpecific
#96	0x90b25d18 in CFRunLoopRunInMode
#97	0x94c5db15 in -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
#98	0x0000598a in runTest at DumpRenderTree.mm:896
#99	0x0000604c in dumpRenderTree (100 of 101 frames) (100 of 101 frames) (100 of 101 frames) (100 of 101 frames) (100 of 101 frames) (100 of 101 frames) (100 of 101 frames) (100 of 101 frames) at DumpRenderTree.mm:424
#100	0x000061d4 in main at DumpRenderTree.mm:464

Comment 6 Eric Seidel (no email) 2008-04-30 11:10:16 PDT
Ah!  I was wrong.  The problem is that willRemoveChild(n) could have side-effects of causing n to be deleted.

    for (n = m_firstChild; n; n = n->nextSibling())
        willRemoveChild(n);

If we end up re-entering this method below that call, n will have been deleted (along with all of the siblings).  One solution would be so simply hold a Ref to n during traversal.  Not sure if that's the right fix.
Comment 7 Eric Seidel (no email) 2008-04-30 12:37:38 PDT
Created attachment 20904 [details]
First pass fix

 LayoutTests/ChangeLog                              |   13 +++++++++++++
 LayoutTests/fast/dom/removeChildren-crash.html     |   14 ++++++++++++++
 .../fast/dom/resources/removeChildren-crash.html   |    3 +++
 .../fast/dom/resources/removeChildren-crash.png    |  Bin 0 -> 144 bytes
 .../fast/dom/resources/removeChildren-crash2.html  |    6 ++++++
 .../fast/dom/resources/removeChildren-crash2.png   |  Bin 0 -> 10292 bytes
 WebCore/ChangeLog                                  |   12 ++++++++++++
 WebCore/dom/ContainerNode.cpp                      |   20 ++++++++------------
 8 files changed, 56 insertions(+), 12 deletions(-)
Comment 8 Darin Adler 2008-05-01 09:56:44 PDT
Comment on attachment 20904 [details]
First pass fix

This looks good. It's great that it fixes the crash. I worry that the willRemoveChild could still be used to make an infinite loop.

+    for (RefPtr<Node> n = m_firstChild; m_firstChild; n = m_firstChild) {

I can't see any reason to not write this as:

    while (RefPtr<Node> n = m_firstChild)

+    // children), so we keep n in a RefPtr, lest n->nextSibling() crash (18803)

I don't think the bug number is really all that valuable, not the specific "lest n->nextSibling() crash".

Maybe the png files should have more generic names. Do we really need two separate images for this? Is there a way to make the timing right for the crash without having images?
Comment 9 Darin Adler 2008-06-08 12:00:20 PDT
Comment on attachment 20904 [details]
First pass fix

I tried to land this patch and:

    1) Couldn't apply the patch because svn-apply didn't seem to understand the copied .png files.
    2) Noticed the patch didn't include results for the test.
    3) Couldn't get the test case to crash without the fix applied.

So I need to change this to review-. Lets get a working regression test and then find a way to land this fix.
Comment 10 Eric Seidel (no email) 2008-06-11 00:58:31 PDT
(In reply to comment #9)
> (From update of attachment 20904 [details] [edit])
> I tried to land this patch and:
> 
>     1) Couldn't apply the patch because svn-apply didn't seem to understand the
> copied .png files.
>     2) Noticed the patch didn't include results for the test.
>     3) Couldn't get the test case to crash without the fix applied.
> 
> So I need to change this to review-. Lets get a working regression test and
> then find a way to land this fix.

The regression test only crashes if you run under MallocScribble.  I have not re-verified that today, but that was the case when I wrote the test (see earlier comments in the bug).

I've rebased that git branch to TOT, and can re-post tomorrow (including test results).
Comment 11 Alexey Proskuryakov 2008-06-21 11:57:45 PDT
See also: bug 15707.
Comment 12 Eric Seidel (no email) 2008-11-11 11:00:22 PST
I guess I should finally update and land this.  Assigning to myself.  Now I just have to find the git branch...
Comment 13 Deirdre Saoirse Moen 2008-11-14 16:19:49 PST
<rdar://problem/6361982 >
Comment 14 Eric Seidel (no email) 2008-11-18 09:41:35 PST
Odd.  I can't get the test case to crash either anymore. :(  I seems that we're no longer firing the main body onload like we were before.  This code still looks wrong to me, but I don't have a test case for it.  So I guess maybe we'll just wait and see if it crashes again... :(
Comment 15 Eric Seidel (no email) 2008-11-18 10:08:09 PST
I still believe the code can crash.  Applying my ninja debugging skillz to see if I can't make a test case crash.
Comment 16 Eric Seidel (no email) 2008-11-18 10:20:35 PST
Created attachment 25240 [details]
Fix updated to apply cleanly to ToT

 LayoutTests/ChangeLog                              |   13 +++++++++++++
 LayoutTests/fast/dom/removeChildren-crash.html     |   18 ++++++++++++++++++
 .../fast/dom/resources/removeChildren-crash.html   |    3 +++
 .../fast/dom/resources/removeChildren-crash.png    |  Bin 0 -> 144 bytes
 .../fast/dom/resources/removeChildren-crash2.html  |    6 ++++++
 .../fast/dom/resources/removeChildren-crash2.png   |  Bin 0 -> 10292 bytes
 WebCore/ChangeLog                                  |   12 ++++++++++++
 WebCore/dom/ContainerNode.cpp                      |   20 ++++++++------------
 8 files changed, 60 insertions(+), 12 deletions(-)
Comment 17 Eric Seidel (no email) 2008-11-18 12:27:15 PST
Ok, this test almost works:

<body>
<script>
function gc()
{
    if (window.GCController)
        return GCController.collect();
}

var outerDiv = document.createElement("div");
outerDiv.appendChild(document.createElement("div"));
outerDiv.appendChild(document.createElement("div"));
outerDiv.appendChild(document.createElement("div"));
document.body.appendChild(outerDiv);

// This function should be called right before innerDiv is removed
// by clearing the outerDiv, innerDiv->nextSibling() call should
// then crash once the event listener returns.  This crash was fixed
// by https://bugs.webkit.org/show_bug.cgi?id=18803
function kidRemoved(event) {
    outerDiv.removeEventListener("DOMNodeRemoved", kidRemoved, false);

    gc();

    // Remove any remaining kids to hopefully hit a crash.
    outerDiv.innerHTML = "";
}
outerDiv.addEventListener("DOMNodeRemoved", kidRemoved, false);

// Remove all children to kick off the event sending
outerDiv.innerHTML = "";
</script>
<div>PASS</div>

Unfortunately the MutationEvent is still holding onto the node at the time it should crash.  I still think it's possible to crash this code.
Comment 18 Eric Seidel (no email) 2008-11-18 14:33:04 PST
I think it's no longer crashing due to a change to the HTMLParser where we reject <frameset> elements found inside a body element.  I wonder if that's a tested change? ;)  Attempting to re-work the original test case now.
Comment 19 Eric Seidel (no email) 2008-11-18 17:01:27 PST
Ok, giving up.  I think this was partially caused by another bug.  Since now we seem to properly stop onload events if document.open was called between when the onload was registered and when it fired.
Comment 20 Alexey Proskuryakov 2009-02-18 10:06:31 PST
Bug 15707 has both a test and a fix, so marking as a duplicate.

*** This bug has been marked as a duplicate of 15707 ***
Comment 21 Lucas Forschler 2019-02-06 09:04:02 PST
Mass moving XML DOM bugs to the "DOM" Component.