Bug 12342 – REGRESSION: destroying a frame from its own script causes various crashes

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Steps to reproduce:
1) Open the bug URL.
2) Wait.

Result: a crash in FrameLoader::executeScript() when trying to report an exception, because the frame has a null page.

[Alexey Proskuryakov]

Created attachment 12566 [details]
a reduction (almost)

This reduction produces a crash in a different place (actually, two different places!), but the root problem seems to be very similar.

[Mark Rowe (bdash)]

Alexey, your reduction doesn't crash for me with ToT.

[Mark Rowe (bdash)]

For that matter, the original page doesn't crash either.

[Darin Adler]

We can't hope reproduce the bug on the original site until the fix for bug 10934 is checked in.

I also don't see a crash with the reduction.

[Alexey Proskuryakov]

I also cannot reproduce this anymore. Going to keep open for a little while to investigate what happened.

[Alexey Proskuryakov]

Created attachment 12605 [details]
a reduction (almost)

Turns out the reduction didn't cause a crash because I added a comment that broke parsing just before uploading it.

I could also reproduce the crash at rhapsody.com, but it is intermittent; not sure what triggers it.

[Darin Adler]

Created attachment 12607 [details]
patch, fixes bug; need to turn test case into layout test

[Adam Roben (:aroben)]

Comment on attachment 12607 [details]
patch, fixes bug; need to turn test case into layout test

+    // FIXME: Why not use updateDocuemntsRendering to update rendering of all documents?

Typo: "Docuemnts"

r=me

[Alexey Proskuryakov]

I believe this doesn't fix the rhapsody.com issue, because it happens due to loading subresources, rather that using a timer:

0   com.apple.WebCore        	0x0162e3e4 WTF::OwnPtr<WebCore::Chrome>::get() const + 20 (OwnPtr.h:36)
1   com.apple.WebCore        	0x0162e420 WebCore::Page::chrome() const + 40 (Page.h:83)
2   com.apple.WebCore        	0x013255c4 WebCore::KJSProxy::evaluate(WebCore::String const&, int, WebCore::String const&, WebCore::Node*) + 1212 (kjs_proxy.cpp:75)
3   com.apple.WebCore        	0x014eb218 WebCore::FrameLoader::executeScript(WebCore::String const&, int, WebCore::Node*, WebCore::String const&) + 136 (FrameLoader.cpp:684)
4   com.apple.WebCore        	0x0102583c WebCore::HTMLTokenizer::scriptExecution(WebCore::DeprecatedString const&, WebCore::HTMLTokenizer::State, WebCore::DeprecatedString, int) + 392 (HTMLTokenizer.cpp:501)
5   com.apple.WebCore        	0x0102720c WebCore::HTMLTokenizer::notifyFinished(WebCore::CachedResource*) + 624 (HTMLTokenizer.cpp:1660)
6   com.apple.WebCore        	0x01157f60 WebCore::CachedScript::checkNotify() + 108 (CachedScript.cpp:91)
7   com.apple.WebCore        	0x011580a0 WebCore::CachedScript::data(WTF::Vector<char, (unsigned long)0>&, bool) + 180 (CachedScript.cpp:83)
8   com.apple.WebCore        	0x0115a544 WebCore::Loader::didFinishLoading(WebCore::SubresourceLoader*) + 340 (loader.cpp:108)
9   com.apple.WebCore        	0x014f3510 WebCore::SubresourceLoader::didFinishLoading() + 204 (SubresourceLoader.cpp:166)
10  com.apple.WebCore        	0x014f1854 WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*) + 60
11  com.apple.WebCore        	0x014c87f0 -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] + 144 (ResourceHandleMac.mm:368)
12  com.apple.Foundation     	0x9298f84c -[NSURLConnection(NSURLConnectionInternal) _sendDidFinishLoadingCallback] + 188
13  com.apple.Foundation     	0x9298dab8 -[NSURLConnection(NSURLConnectionInternal) _sendCallbacks] + 556
14  com.apple.Foundation     	0x9298d810 _sendCallbacks + 156
15  com.apple.CoreFoundation 	0x907dd4cc __CFRunLoopDoSources0 + 384
16  com.apple.CoreFoundation 	0x907dc9fc __CFRunLoopRun + 452
17  com.apple.CoreFoundation 	0x907dc47c CFRunLoopRunSpecific + 268
18  com.apple.HIToolbox      	0x93203740 RunCurrentEventLoopInMode + 264
19  com.apple.HIToolbox      	0x93202dd4 ReceiveNextEventCommon + 380
20  com.apple.HIToolbox      	0x93202c40 BlockUntilNextEventMatchingListInMode + 96
21  com.apple.AppKit         	0x93706ae4 _DPSNextEvent + 384
22  com.apple.AppKit         	0x937067a8 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 116
23  com.apple.Safari         	0x00006740 0x1000 + 22336
24  com.apple.AppKit         	0x93702cec -[NSApplication run] + 472
25  com.apple.AppKit         	0x937f387c NSApplicationMain + 452

[Darin Adler]

Yes, that's another problem not seen in the reduction. Also easy to fix (just need to 0-check the page), but needs another test case I guess.

[Alexey Proskuryakov]

Still trying to make a test case for the actual rhapsody.com bug.

[Alexey Proskuryakov]

(In reply to comment #10)
> Also easy to fix (just need to 0-check the page), but needs another test case I guess.

Actually, I'm not sure if it's good for addMessageToConsole() to depend on frame or page existence at all - it would probably be better for messages to be logged even if the page is gone.

[Darin Adler]

(In reply to comment #12)
> Actually, I'm not sure if it's good for addMessageToConsole() to depend on
> frame or page existence at all - it would probably be better for messages to be
> logged even if the page is gone.

I understand what you're saying, but on the other hand, WebKit is not supposed to do things like logging directly. It needs to communicate log messages to the client so the client can determine where to put them. No page, no client. We don't have a global WebKit client. Feel free to file a separate bug about your concern that we will miss log messages and maybe we can solve that problem.

[Darin Adler]

Comment on attachment 12607 [details]
patch, fixes bug; need to turn test case into layout test

Clearing the review flag because I don't have a test.

[Darin Adler]

Created attachment 12662 [details]
improved patch, but still no layout test

[Geoffrey Garen]

(In reply to comment #15)
> Created an attachment (id=12662) [edit]
> improved patch, but still no layout test

I know how to test this, if you need help.

CC Beth: I guess we don't need to file this bug, afterall!

[Alexey Proskuryakov]

Created attachment 12667 [details]
a real reduction

So, I was finally able to make a test that demonstrates the original problem! Many thanks to ggaren for inspiration :)

[Alexey Proskuryakov]

(In reply to comment #13)
> Feel free to file a separate bug about your
> concern that we will miss log messages and maybe we can solve that problem.

Done, bug 12406.

[Geoffrey Garen]

Committed revision 19130.