Bug 22148 - nytime.com page crashes browser during load
Summary: nytime.com page crashes browser during load
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.5
: P1 Major
Assignee: Cameron Zwarich (cpst)
URL: http://www.nytimes.com/2008/11/10/hea...
Keywords: NeedsReduction
Depends on:
Blocks:
 
Reported: 2008-11-09 08:17 PST by Chris Dent
Modified: 2008-11-20 10:52 PST (History)
5 users (show)

See Also:


Attachments
crash log (24.43 KB, text/plain)
2008-11-09 10:44 PST, Chris Dent
no flags Details
second crash log, for confirmation (24.76 KB, text/plain)
2008-11-09 14:11 PST, Chris Dent
no flags Details
Webarchive of crashy page (1.13 MB, application/octet-stream)
2008-11-10 15:39 PST, Simon Fraser (smfr)
no flags Details
Patch to add tests (2.25 KB, patch)
2008-11-20 10:47 PST, Cameron Zwarich (cpst)
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dent 2008-11-09 08:17:11 PST
Version 3.1.2 (5525.20.1) and the nightly build just prior.

When loading the above url into a freshly started browser, the application crashes towards the end of the load (most of the layout is done, just getting ready for a good read, and the crash happens).

Consistently repeatable.
Comment 1 Chris Dent 2008-11-09 08:18:44 PST
If  I disable javascript from the develop menu, the crash does not happen.
Comment 2 Mark Rowe (bdash) 2008-11-09 08:59:45 PST
Please attach a crash log: <http://webkit.org/quality/crashlogs.html>.
Comment 3 Matt Lilek 2008-11-09 10:42:43 PST
Confirmed with r38239:

Exception Type:  EXC_BAD_ACCESS (SIGBUS)
Exception Codes: KERN_PROTECTION_FAILURE at 0x0000000000000114
Crashed Thread:  0

Thread 0 Crashed:
0   com.apple.JavaScriptCore      	0x0053dc2f bool WTF::HashTable<void*, std::pair<void*, unsigned int>, WTF::PairFirstExtractor<std::pair<void*, unsigned int> >, WTF::PtrHash<void*>, WTF::PairHashTraits<WTF::HashTraits<void*>, WTF::HashTraits<unsigned int> >, WTF::HashTraits<void*> >::contains<void*, WTF::IdentityHashTranslator<void*, std::pair<void*, unsigned int>, WTF::PtrHash<void*> > >(void* const&) const + 9 (HashTable.h:779)
1   com.apple.JavaScriptCore      	0x0053dc78 WTF::HashTable<void*, std::pair<void*, unsigned int>, WTF::PairFirstExtractor<std::pair<void*, unsigned int> >, WTF::PtrHash<void*>, WTF::PairHashTraits<WTF::HashTraits<void*>, WTF::HashTraits<unsigned int> >, WTF::HashTraits<void*> >::contains(void* const&) const + 24 (HashTable.h:315)
2   com.apple.JavaScriptCore      	0x0053dc96 WTF::HashMap<void*, unsigned int, WTF::PtrHash<void*>, WTF::HashTraits<void*>, WTF::HashTraits<unsigned int> >::contains(void* const&) const + 24 (HashMap.h:173)
3   com.apple.JavaScriptCore      	0x0052fa69 JSC::Machine::cti_vm_throw(void*, ...) + 83 (Machine.cpp:5960)
4   com.apple.JavaScriptCore      	0x00524020 jscGeneratedNativeCode + 0 (Machine.cpp:4296)
5   com.apple.JavaScriptCore      	0x0052afb2 JSC::Machine::execute(JSC::ProgramNode*, JSC::ExecState*, JSC::ScopeChainNode*, JSC::JSObject*, JSC::JSValue**) + 674 (Machine.cpp:934)
6   com.apple.JavaScriptCore      	0x004b358a JSC::Interpreter::evaluate(JSC::ExecState*, JSC::ScopeChain&, JSC::SourceCode const&, JSC::JSValue*) + 438 (Interpreter.cpp:68)
7   com.apple.WebCore             	0x0398b896 WebCore::ScriptController::evaluate(WebCore::String const&, int, WebCore::String const&) + 250 (ScriptController.cpp:111)
8   com.apple.WebCore             	0x0351368c WebCore::FrameLoader::executeScript(WebCore::String const&, int, WebCore::String const&) + 154 (FrameLoader.cpp:792)
9   com.apple.WebCore             	0x035a2054 WebCore::HTMLTokenizer::scriptExecution(WebCore::String const&, WebCore::HTMLTokenizer::State, WebCore::String const&, int) + 300 (HTMLTokenizer.cpp:563)
10  com.apple.WebCore             	0x035a24c3 WebCore::HTMLTokenizer::notifyFinished(WebCore::CachedResource*) + 623 (HTMLTokenizer.cpp:2011)
11  com.apple.WebCore             	0x0335c6ce WebCore::CachedScript::checkNotify() + 86 (CachedScript.cpp:92)
12  com.apple.WebCore             	0x0335c8cb WebCore::CachedScript::data(WTF::PassRefPtr<WebCore::SharedBuffer>, bool) + 279 (CachedScript.cpp:84)
13  com.apple.WebCore             	0x03991351 WebCore::Loader::Host::didFinishLoading(WebCore::SubresourceLoader*) + 413 (loader.cpp:300)
14  com.apple.WebCore             	0x0390c257 WebCore::SubresourceLoader::didFinishLoading() + 169 (SubresourceLoader.cpp:196)
15  com.apple.WebCore             	0x03833332 WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*) + 24 (ResourceLoader.cpp:399)
Comment 4 Chris Dent 2008-11-09 10:44:56 PST
Created attachment 25001 [details]
crash log
Comment 5 Cameron Zwarich (cpst) 2008-11-09 13:41:48 PST
I'll assign this to myself.
Comment 6 Cameron Zwarich (cpst) 2008-11-09 14:05:06 PST
I can't reproduce this. Can you still reproduce it, Chris?
Comment 7 Chris Dent 2008-11-09 14:10:34 PST
(In reply to comment #6)
> I can't reproduce this. Can you still reproduce it, Chris?

Yes. I'll attach a new crash report, just to be sure.

I also tried disabling caches, to see if that made any difference. It did not.
Comment 8 Chris Dent 2008-11-09 14:11:43 PST
Created attachment 25007 [details]
second crash log, for confirmation
Comment 9 Cameron Zwarich (cpst) 2008-11-09 14:19:23 PST
What is the precise revision of the nightly you are using? I have tried all recent ones, but none seem to crash.

Can you make a web archive (File / Save As...) and see whether loading the web archive crashes? If it does, then upload it to this bug, so that even if nytimes.com changes I can still try it on another machine at work.
Comment 10 Simon Fraser (smfr) 2008-11-10 15:27:35 PST
It crashes for me; latest nightly (11/9). Mac OS X 10.5.5, intel. Crash at:
Thread 0 Crashed:
0   com.apple.JavaScriptCore      	0x003cae39 JSC::Machine::cti_vm_throw(void*, ...) + 57
Comment 11 Simon Fraser (smfr) 2008-11-10 15:30:44 PST
In Machine::cti_vm_throw(), codeBlock is 0, so a debug build crashes in:
    ASSERT(codeBlock->ctiReturnAddressVPCMap.contains(ARG_globalData->throwReturnAddress));
Comment 12 Cameron Zwarich (cpst) 2008-11-10 15:32:59 PST
(In reply to comment #11)
> In Machine::cti_vm_throw(), codeBlock is 0, so a debug build crashes in:
>    
> ASSERT(codeBlock->ctiReturnAddressVPCMap.contains(ARG_globalData->throwReturnAddress));

That's not good. Does it crash with a local web archive copy?
Comment 13 Simon Fraser (smfr) 2008-11-10 15:39:20 PST
Created attachment 25030 [details]
Webarchive of crashy page

Yes, it crashes when loading from this webarchive.
Comment 14 Cameron Zwarich (cpst) 2008-11-19 13:11:32 PST
Now that my machine died and I have a loaner, I can reproduce this with the 11/9 nightly. I will try a ToT debug build and see if that works.
Comment 15 Cameron Zwarich (cpst) 2008-11-19 21:48:51 PST
I have a one line reduction:

function f(a) { f(); } f();

It dies throwing the recursion limit exception. Yes, this was actually happening on a NY Times article, because they load two copies of prototype.js in a weird way.

This was fixed in r38322 by Geoff:

http://trac.webkit.org/changeset/38322

Geoff, do you immediately see why your patch would have fixed this?

I'll make tests for this and similar cases and put them up for review.
Comment 16 Geoffrey Garen 2008-11-19 22:24:36 PST
> I have a one line reduction:
> 
> function f(a) { f(); } f();

Nice reduction.

> This was fixed in r38322 by Geoff:
> 
> http://trac.webkit.org/changeset/38322
> 
> Geoff, do you immediately see why your patch would have fixed this?

Yes.

The old "throwStackOverflowPreviousFrame" was buggy, but untested. My patch tickled fast/js/global-recursion-on-full-stack.html in a way that illustrated the problem. Since I had to rewrite the logic of throwStackOverflowPreviousFrame anyway, I fixed the bug along the way. Here's what I said in the ChangeLog:

"Fixed a latent bug in stack overflow checking that is now hit because
the register layout has changed a bit -- namely: when throwing a stack
overflow exception inside an op_call helper, we need to account for the
fact that the current call frame is only half-constructed, and use the
parent call frame instead."
Comment 17 Cameron Zwarich (cpst) 2008-11-20 09:49:01 PST
(In reply to comment #16)
> > Geoff, do you immediately see why your patch would have fixed this?
> 
> Yes.
> 
> The old "throwStackOverflowPreviousFrame" was buggy, but untested. My patch
> tickled fast/js/global-recursion-on-full-stack.html in a way that illustrated
> the problem. Since I had to rewrite the logic of
> throwStackOverflowPreviousFrame anyway, I fixed the bug along the way. Here's
> what I said in the ChangeLog:
> 
> "Fixed a latent bug in stack overflow checking that is now hit because
> the register layout has changed a bit -- namely: when throwing a stack
> overflow exception inside an op_call helper, we need to account for the
> fact that the current call frame is only half-constructed, and use the
> parent call frame instead."

Thanks. I saw that, but I wasn't quite sure at the moment if it applied in this case.
Comment 18 Cameron Zwarich (cpst) 2008-11-20 10:47:41 PST
Created attachment 25318 [details]
Patch to add tests

I confirmed that the tests added here cause DRT to crash in revisions where this bug existed.
Comment 19 Geoffrey Garen 2008-11-20 10:48:59 PST
Comment on attachment 25318 [details]
Patch to add tests

r=me
Comment 20 Cameron Zwarich (cpst) 2008-11-20 10:52:55 PST
Landed in r38624.