Bug 12342 - REGRESSION: destroying a frame from its own script causes various crashes
Summary: REGRESSION: destroying a frame from its own script causes various crashes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Critical
Assignee: Darin Adler
URL: http://www.rhapsody.com/home.html
Keywords: Regression
Depends on:
Blocks:
 
Reported: 2007-01-20 01:13 PST by Alexey Proskuryakov
Modified: 2007-07-25 19:04 PDT (History)
2 users (show)

See Also:


Attachments
a reduction (almost) (328 bytes, text/html)
2007-01-20 01:17 PST, Alexey Proskuryakov
no flags Details
a reduction (almost) (248 bytes, text/html)
2007-01-22 09:42 PST, Alexey Proskuryakov
no flags Details
patch, fixes bug; need to turn test case into layout test (3.53 KB, patch)
2007-01-22 10:28 PST, Darin Adler
no flags Details | Formatted Diff | Diff
improved patch, but still no layout test (8.24 KB, patch)
2007-01-24 23:12 PST, Darin Adler
no flags Details | Formatted Diff | Diff
a real reduction (1.40 KB, application/zip)
2007-01-25 10:20 PST, Alexey Proskuryakov
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2007-01-20 01:13:07 PST
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.
Comment 1 Alexey Proskuryakov 2007-01-20 01:17:09 PST
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.
Comment 2 Mark Rowe (bdash) 2007-01-21 16:12:02 PST
Alexey, your reduction doesn't crash for me with ToT.
Comment 3 Mark Rowe (bdash) 2007-01-21 16:12:59 PST
For that matter, the original page doesn't crash either.
Comment 4 Darin Adler 2007-01-21 17:54:12 PST
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.
Comment 5 Alexey Proskuryakov 2007-01-21 21:31:41 PST
I also cannot reproduce this anymore. Going to keep open for a little while to investigate what happened.
Comment 6 Alexey Proskuryakov 2007-01-22 09:42:52 PST
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.
Comment 7 Darin Adler 2007-01-22 10:28:15 PST
Created attachment 12607 [details]
patch, fixes bug; need to turn test case into layout test
Comment 8 Adam Roben (:aroben) 2007-01-22 10:30:41 PST
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
Comment 9 Alexey Proskuryakov 2007-01-22 10:41:44 PST
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
Comment 10 Darin Adler 2007-01-22 13:05:50 PST
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.
Comment 11 Alexey Proskuryakov 2007-01-23 12:29:20 PST
Still trying to make a test case for the actual rhapsody.com bug.
Comment 12 Alexey Proskuryakov 2007-01-24 21:01:18 PST
(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.
Comment 13 Darin Adler 2007-01-24 22:38:56 PST
(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.
Comment 14 Darin Adler 2007-01-24 23:10:53 PST
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.
Comment 15 Darin Adler 2007-01-24 23:12:34 PST
Created attachment 12662 [details]
improved patch, but still no layout test
Comment 16 Geoffrey Garen 2007-01-25 09:39:42 PST
(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!
Comment 17 Alexey Proskuryakov 2007-01-25 10:20:01 PST
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 :)
Comment 18 Alexey Proskuryakov 2007-01-25 10:51:27 PST
(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.
Comment 19 Geoffrey Garen 2007-01-25 12:40:32 PST
Committed revision 19130.