Crash in Frame::tree appending iframe

[Eric Roman]

http://grs-motorsports.com/index/index.html crashes every time when loading in Safari 3.1 or webkit nightly (r31363)

[Eric Roman]

Created attachment 20117 [details]
frame loading / removal sequence that generates crash

load index.html to reproduce the crash.

[Matt Lilek]

Confirmed with r31371:

0   com.apple.WebCore             	0x01ce0207 WebCore::Frame::tree() const + 9 (Frame.cpp:1677)
1   com.apple.WebCore             	0x01d0382d WebCore::FrameTree::removeChild(WebCore::Frame*) + 155 (FrameTree.cpp:83)
2   com.apple.WebCore             	0x01cf9805 WebCore::FrameLoader::detachFromParent() + 261 (FrameLoader.cpp:3347)
3   com.apple.WebCore             	0x01cf9888 WebCore::FrameLoader::frameDetached() + 28 (FrameLoader.cpp:3328)
4   com.apple.WebCore             	0x01d36086 WebCore::HTMLFrameOwnerElement::willRemove() + 56 (HTMLFrameOwnerElement.cpp:49)
5   com.apple.WebCore             	0x01bed0a5 WebCore::willRemoveChild(WebCore::Node*) + 77 (ContainerNode.cpp:363)
6   com.apple.WebCore             	0x01bed4a8 WebCore::ContainerNode::removeChild(WebCore::Node*, int&) + 590 (ContainerNode.cpp:396)
7   com.apple.WebCore             	0x01e47480 WebCore::JSNode::removeChild(KJS::ExecState*, KJS::List const&) + 80 (JSNodeCustom.cpp:92)
8   com.apple.WebCore             	0x01e45294 WebCore::jsNodePrototypeFunctionRemoveChild(KJS::ExecState*, KJS::JSObject*, KJS::List const&) + 96 (JSNode.cpp:328)

[Matt Lilek]

Created attachment 20124 [details]
Full debug crashlog

[Feng Qian]

WebKit fires events immediately when a frame is finishing loading or unloading. The event can call JavaScript to remove the event generator in many cases. How about delay the event handling until next time cycle by using a timer? Similar to setTimeout(0, fire events). 

[Eric Roman]

I'm not an onload expert, but the ordering that the events are dispatched looks wrong
Notably, appending the iframe to parent is triggering the parent window's onload handler!

Here is the specific order of events I observe in the reduced test-case:

(1) mainFrame attaches load handler on its window
(2) subFrame attaches load handler on its window
(3) subFrame's window load handler invoked
..(4) append an iframe "x" to mainFrame's body (top.document.body)
....(5) mainFrame's window load handler invoked synchronously
......(6) remove iframe "x"
[CRASH]

Step (5) looks wrong.

The more plausible ordering which I get when running in Firefox is:

(1) mainFrame attaches load handler on its window
(2) subFrame attaches load handler on its window
(3) subFrame's window load handler invoked
..(4) append an iframe "x" to mainFrame's body
(5) mainFrame's window load handler invoked
..(6) remove iframe "x"
..(7) add back iframe "x"

[Eric Roman]

Created attachment 20135 [details]
Reproduce the crash by adding/removing iframes

Slightly cleaned up version of earlier repro case.

[Feng Qian]

I think the problem is in 
35  com.apple.WebKit              	0x001b3a70 +[WebFrame(WebInternal) _createFrameWithPage:frameName:frameView:ownerElement:] + 314 (WebFrame.mm:283)
36  com.apple.WebKit              	0x001af315 +[WebFrame(WebInternal) _createSubframeWithOwnerElement:frameName:frameView:] + 107 (WebFrame.mm:295)
37  com.apple.WebKit              	0x001bcf05 WebFrameLoaderClient::createFrame(WebCore::KURL const&, WebCore::String const&, WebCore::HTMLFrameOwnerElement*, WebCore::String const&, bool, int, int) + 527 (WebFrameLoaderClient.mm:1052)

frame->init() is called before it was appended to the tree, but it has parent set already. init() triggers removeChild operation which thinks the child frame is in the tree, but it is not.

[Feng Qian]

Created attachment 20320 [details]
a patch and a layout test

My attempt to fix the crash. Frame::init() should be called after the subframe is appended to the frame tree.

I am still not good at right layout test, feel free to make changes.

[Maciej Stachowiak]

Comment on attachment 20320 [details]
a patch and a layout test

r=me

[Feng Qian]

Thanks Maciej, someone can land the patch, I don't have svn checkin right.

Comment on attachment 20320 [details]
a patch and a layout test

I applied the patch and then opened the test in the browser and got this crash:

#0  0x01fa2ed5 in WTF::RefPtr<WebCore::ApplicationCache>::get (this=0x4e0) at RefPtr.h:49
#1  0x01fa2ef0 in WebCore::DocumentLoader::applicationCache (this=0x0) at DocumentLoader.h:200
#2  0x024efa2c in WebCore::ApplicationCacheGroup::selectCacheWithoutManifestURL (frame=0x18fe5100) at /Volumes/Data/Safari/OpenSource/WebCore/loader/appcache/ApplicationCacheGroup.cpp:219
#3  0x02054301 in WebCore::HTMLHtmlElement::insertedIntoDocument (this=0x18f23740) at /Volumes/Data/Safari/OpenSource/WebCore/html/HTMLHtmlElement.cpp:77
#4  0x01effbde in WebCore::ContainerNode::addChild (this=0x439d600, newChild=@0xbfffd18c) at /Volumes/Data/Safari/OpenSource/WebCore/dom/ContainerNode.cpp:599
#5  0x0207e20f in WebCore::HTMLParser::insertNode (this=0x18f236d0, n=0x18f23740, flat=false) at /Volumes/Data/Safari/OpenSource/WebCore/html/HTMLParser.cpp:316
#6  0x0207e529 in WebCore::HTMLParser::finished (this=0x18f236d0) at /Volumes/Data/Safari/OpenSource/WebCore/html/HTMLParser.cpp:1441
#7  0x02095d7b in WebCore::HTMLTokenizer::end (this=0x43a5a00) at /Volumes/Data/Safari/OpenSource/WebCore/html/HTMLTokenizer.cpp:1827
#8  0x02096123 in WebCore::HTMLTokenizer::finish (this=0x43a5a00) at /Volumes/Data/Safari/OpenSource/WebCore/html/HTMLTokenizer.cpp:1867
#9  0x01f82d1e in WebCore::Document::finishParsing (this=0x439d600) at /Volumes/Data/Safari/OpenSource/WebCore/dom/Document.cpp:1690
#10 0x020104a8 in WebCore::FrameLoader::endIfNotLoadingMainResource (this=0x43a3424) at /Volumes/Data/Safari/OpenSource/WebCore/loader/FrameLoader.cpp:1055
#11 0x020104dd in WebCore::FrameLoader::end (this=0x43a3424) at /Volumes/Data/Safari/OpenSource/WebCore/loader/FrameLoader.cpp:1039
#12 0x02010728 in WebCore::FrameLoader::init (this=0x43a3424) at /Volumes/Data/Safari/OpenSource/WebCore/loader/FrameLoader.cpp:294
#13 0x01ff995d in WebCore::Frame::init (this=0x18fe5100) at /Volumes/Data/Safari/OpenSource/WebCore/page/Frame.cpp:203
warning: internal error: no C/C++ fundamental type 1
#14 0x0035f00a in WebFrameLoaderClient::createFrame (this=0x3f50b10, url=@0xbfffd6ec, name=@0x18f201c4, ownerElement=0x18f20170, referrer=@0xbfffd680, allowsScrolling=1, marginWidth=-1, marginHeight=-1) at /Volumes/Data/Safari/OpenSource/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1063
#15 0x02010b0f in WebCore::FrameLoader::loadSubframe (this=0x4069e24, ownerElement=0x18f20170, url=@0xbfffd6ec, name=@0x18f201c4, referrer=@0x4069ef8) at /Volumes/Data/Safari/OpenSource/WebCore/loader/FrameLoader.cpp:462
#16 0x02013407 in WebCore::FrameLoader::requestFrame (this=0x4069e24, ownerElement=0x18f20170, urlString=@0x18f201c0, frameName=@0x18f201c4) at /Volumes/Data/Safari/OpenSource/WebCore/loader/FrameLoader.cpp:432
#17 0x0204ef7c in WebCore::HTMLFrameElementBase::openURL (this=0x18f20170) at /Volumes/Data/Safari/OpenSource/WebCore/html/HTMLFrameElementBase.cpp:105
#18 0x0204f562 in WebCore::HTMLFrameElementBase::setNameAndOpenURL (this=0x18f20170) at /Volumes/Data/Safari/OpenSource/WebCore/html/HTMLFrameElementBase.cpp:163
#19 0x0204f57b in WebCore::HTMLFrameElementBase::setNameAndOpenURLCallback (n=0x18f20170) at /Volumes/Data/Safari/OpenSource/WebCore/html/HTMLFrameElementBase.cpp:168
(More stack frames follow...)

This may be a separate bug but it's blocking this patch for now.

[Eric Seidel (no email)]

Just curious what the status of this patch is...

[Eric Seidel (no email)]

The patch no longer applies, but looks like it should be relatively straightforward to update it.

[Eric Seidel (no email)]

Do we still believe this crashes?

[Eric Roman]

Looks to be obsolete; closing.