Bug 15848 – REGRESSION: Assertion failure viewing comments page on digg.com

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

* SUMMARY
An assertion failure still occurs viewing a comments page on digg.com.

* STEPS TO REPRODUCE
1. Launch Safari/WebKit with a debug build of WebKit.
2. Go to URL:  http://digg.com/
3. Sign in.
4. Search for "Leopard Windows bsod".
5. Click on the "Comments" link for the "Remove the Windows BSOD icon in Leopard, make OS X a little less smug" story.
6. Wait.  (Feel free to scroll around the page if it helps make the time pass.)

* RESULTS
WebKit crashes with an assertion failure.

* REGRESSION
This is a regression from Safari 3 on Leopard.

* NOTES
Console output:

ASSERTION FAILED: !HashTranslator::equal(KeyTraits::emptyValue(), key)
(./wtf/HashTable.h:426 Value* WTF::HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>::lookup(const T&) [with T = unsigned int, HashTranslator = WTF::IdentityHashTranslator<unsigned int, std::pair<unsigned int, int>, WTF::IntHash<unsigned int> >, Key = unsigned int, Value = std::pair<unsigned int, int>, Extractor = WTF::PairFirstExtractor<std::pair<unsigned int, int> >, HashFunctions = WTF::IntHash<unsigned int>, Traits = WTF::PairHashTraits<WTF::HashTraits<unsigned int>, WTF::HashTraits<int32_t> >, KeyTraits = WTF::HashTraits<unsigned int>])
Segmentation fault

Stack trace:

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_INVALID_ADDRESS (0x0001) at 0xbbadbeef

Thread 0 Crashed:
0   com.apple.JavaScriptCore 	0x00639768 std::pair<unsigned, int>* WTF::HashTable<unsigned, std::pair<unsigned, int>, WTF::PairFirstExtractor<std::pair<unsigned, int> >, WTF::IntHash<unsigned>, WTF::PairHashTraits<WTF::HashTraits<unsigned>, WTF::HashTraits<int> >, WTF::HashTraits<unsigned> >::lookup<unsigned, WTF::IdentityHashTranslator<unsigned, std::pair<unsigned, int>, WTF::IntHash<unsigned> > >(unsigned const&) + 184 (HashTable.h:426)
1   com.apple.JavaScriptCore 	0x0063ccac WTF::HashTableIterator<unsigned, std::pair<unsigned, int>, WTF::PairFirstExtractor<std::pair<unsigned, int> >, WTF::IntHash<unsigned>, WTF::PairHashTraits<WTF::HashTraits<unsigned>, WTF::HashTraits<int> >, WTF::HashTraits<unsigned> > WTF::HashTable<unsigned, std::pair<unsigned, int>, WTF::PairFirstExtractor<std::pair<unsigned, int> >, WTF::IntHash<unsigned>, WTF::PairHashTraits<WTF::HashTraits<unsigned>, WTF::HashTraits<int> >, WTF::HashTraits<unsigned> >::find<unsigned, WTF::IdentityHashTranslator<unsigned, std::pair<unsigned, int>, WTF::IntHash<unsigned> > >(unsigned const&) + 80 (HashTable.h:729)
2   com.apple.JavaScriptCore 	0x0063cd34 WTF::HashTable<unsigned, std::pair<unsigned, int>, WTF::PairFirstExtractor<std::pair<unsigned, int> >, WTF::IntHash<unsigned>, WTF::PairHashTraits<WTF::HashTraits<unsigned>, WTF::HashTraits<int> >, WTF::HashTraits<unsigned> >::find(unsigned const&) + 52 (HashTable.h:314)
3   com.apple.JavaScriptCore 	0x0063cd84 WTF::HashMap<unsigned, KJS::JSValue*, WTF::IntHash<unsigned>, WTF::HashTraits<unsigned>, WTF::HashTraits<KJS::JSValue*> >::find(unsigned const&) + 56 (HashMap.h:251)
4   com.apple.JavaScriptCore 	0x00664be0 KJS::ArrayInstance::inlineGetOwnPropertySlot(KJS::ExecState*, unsigned, KJS::PropertySlot&) + 328 (array_instance.cpp:154)
5   com.apple.JavaScriptCore 	0x0059efbc KJS::ArrayInstance::getOwnPropertySlot(KJS::ExecState*, unsigned, KJS::PropertySlot&) + 56 (array_instance.cpp:180)
6   com.apple.JavaScriptCore 	0x00578160 KJS::JSObject::getPropertySlot(KJS::ExecState*, unsigned, KJS::PropertySlot&) + 88 (object.cpp:182)
7   com.apple.JavaScriptCore 	0x0059e6f0 KJS::JSObject::get(KJS::ExecState*, unsigned) const + 52 (object.cpp:171)
8   com.apple.JavaScriptCore 	0x005b23a0 KJS::BracketAccessorNode::evaluate(KJS::ExecState*) + 328 (nodes.cpp:583)
9   com.apple.JavaScriptCore 	0x005b1080 KJS::TypeOfValueNode::evaluate(KJS::ExecState*) + 84 (nodes.cpp:1322)
10  com.apple.JavaScriptCore 	0x005af1b0 KJS::EqualNode::evaluate(KJS::ExecState*) + 84 (nodes.cpp:1941)
11  com.apple.JavaScriptCore 	0x005ad520 KJS::IfNode::execute(KJS::ExecState*) + 204 (nodes.cpp:2736)
12  com.apple.JavaScriptCore 	0x0058a5b8 KJS::statementListExecute(WTF::Vector<WTF::RefPtr<KJS::StatementNode>, (unsigned long)0>&, KJS::ExecState*) + 148 (nodes.cpp:2648)
13  com.apple.JavaScriptCore 	0x0058a7c8 KJS::BlockNode::execute(KJS::ExecState*) + 120 (nodes.cpp:2689)
14  com.apple.JavaScriptCore 	0x005ad674 KJS::IfNode::execute(KJS::ExecState*) + 544 (nodes.cpp:2749)
15  com.apple.JavaScriptCore 	0x0058a5b8 KJS::statementListExecute(WTF::Vector<WTF::RefPtr<KJS::StatementNode>, (unsigned long)0>&, KJS::ExecState*) + 148 (nodes.cpp:2648)
16  com.apple.JavaScriptCore 	0x0058a7c8 KJS::BlockNode::execute(KJS::ExecState*) + 120 (nodes.cpp:2689)
17  com.apple.JavaScriptCore 	0x005a920c KJS::FunctionBodyNode::execute(KJS::ExecState*) + 68 (nodes.cpp:3607)
18  com.apple.JavaScriptCore 	0x00577550 KJS::FunctionImp::execute(KJS::ExecState*) + 96 (function.cpp:252)
19  com.apple.JavaScriptCore 	0x005b5df0 KJS::FunctionImp::callAsFunction(KJS::ExecState*, KJS::JSObject*, KJS::List const&) + 464 (function.cpp:93)
20  com.apple.JavaScriptCore 	0x005a2178 KJS::JSObject::call(KJS::ExecState*, KJS::JSObject*, KJS::List const&) + 304 (object.cpp:95)
21  com.apple.JavaScriptCore 	0x005c2d58 KJS::FunctionCallResolveNode::evaluate(KJS::ExecState*) + 824 (nodes.cpp:762)
22  com.apple.JavaScriptCore 	0x005ad77c KJS::ExprStatementNode::execute(KJS::ExecState*) + 204 (nodes.cpp:2713)
23  com.apple.JavaScriptCore 	0x0058a5b8 KJS::statementListExecute(WTF::Vector<WTF::RefPtr<KJS::StatementNode>, (unsigned long)0>&, KJS::ExecState*) + 148 (nodes.cpp:2648)
24  com.apple.JavaScriptCore 	0x0058a7c8 KJS::BlockNode::execute(KJS::ExecState*) + 120 (nodes.cpp:2689)
25  com.apple.JavaScriptCore 	0x005a920c KJS::FunctionBodyNode::execute(KJS::ExecState*) + 68 (nodes.cpp:3607)
26  com.apple.JavaScriptCore 	0x005d1dac KJS::Interpreter::evaluate(KJS::UString const&, int, KJS::UChar const*, int, KJS::JSValue*) + 920 (interpreter.cpp:366)
27  com.apple.WebCore        	0x015c84e4 WebCore::KJSProxy::evaluate(WebCore::String const&, int, WebCore::String const&) + 280 (kjs_proxy.cpp:87)
28  com.apple.WebCore        	0x011a8c00 WebCore::FrameLoader::executeScript(WebCore::String const&, int, WebCore::String const&) + 128 (FrameLoader.cpp:761)
29  com.apple.WebCore        	0x011a8cdc WebCore::FrameLoader::executeScript(WebCore::String const&, bool) + 136 (FrameLoader.cpp:749)
30  com.apple.WebCore        	0x015ccb28 KJS::ScheduledAction::execute(KJS::Window*) + 1248 (kjs_window.cpp:1520)
31  com.apple.WebCore        	0x015ccc24 KJS::Window::timerFired(KJS::DOMWindowTimer*) + 104 (kjs_window.cpp:1638)
32  com.apple.WebCore        	0x015cce58 KJS::DOMWindowTimer::fired() + 72 (kjs_window.cpp:1912)
33  com.apple.WebCore        	0x01561444 WebCore::TimerBase::fireTimers(double, WTF::Vector<WebCore::TimerBase*, (unsigned long)0> const&) + 244 (Timer.cpp:339)
34  com.apple.WebCore        	0x01561524 WebCore::TimerBase::sharedTimerFired() + 132 (Timer.cpp:359)
35  com.apple.WebCore        	0x0153ba38 WebCore::timerFired(__CFRunLoopTimer*, void*) + 140 (SharedTimerMac.cpp:85)
36  com.apple.CoreFoundation 	0x907f1578 __CFRunLoopDoTimer + 184
37  com.apple.CoreFoundation 	0x907ddef8 __CFRunLoopRun + 1680
38  com.apple.CoreFoundation 	0x907dd4ac CFRunLoopRunSpecific + 268
39  com.apple.HIToolbox      	0x93298b20 RunCurrentEventLoopInMode + 264
40  com.apple.HIToolbox      	0x932981b4 ReceiveNextEventCommon + 380
41  com.apple.HIToolbox      	0x93298020 BlockUntilNextEventMatchingListInMode + 96
42  com.apple.AppKit         	0x9379eae4 _DPSNextEvent + 384
43  com.apple.AppKit         	0x9379e7a8 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 116
44  com.apple.Safari         	0x00006770 0x1000 + 22384
45  com.apple.AppKit         	0x9379acec -[NSApplication run] + 472
46  com.apple.AppKit         	0x9388b87c NSApplicationMain + 452
47  com.apple.Safari         	0x0000244c 0x1000 + 5196
48  com.apple.Safari         	0x0004f1b0 0x1000 + 319920

[David Kilzer (:ddkilzer)]

(In reply to comment #0)
> 4. Search for "Leopard Windows bsod".
> 5. Click on the "Comments" link for the "Remove the Windows BSOD icon in
> Leopard, make OS X a little less smug" story.

Direct link:

http://digg.com/apple/Remove_the_Windows_BSOD_icon_in_Leopard_make_OS_X_a_little_less_smug

[David Kilzer (:ddkilzer)]

(In reply to comment #0)
> * SUMMARY
> An assertion failure still occurs viewing a comments page on digg.com.

See Bug 15747 Comment #5.

[David Kilzer (:ddkilzer)]

<rdar://problem/5619330>

[David Kilzer (:ddkilzer)]

*** Bug 16155 has been marked as a duplicate of this bug. ***

[David Kilzer (:ddkilzer)]

*** Bug 16016 has been marked as a duplicate of this bug. ***

[David Kilzer (:ddkilzer)]

*** Bug 16031 has been marked as a duplicate of this bug. ***

Created attachment 17661 [details]
Reduction (will ASSERT)

The ASSERT is hit when accessing the 0th element of a sparse array. 0 is the empty value in the sparse array hash map so the assertion fails.

The ASSERT was added in <http://trac.webkit.org/projects/webkit/changeset/27196> where a similar problem was fixed for timer IDs.

Created attachment 17662 [details]
Reduction (will crash)

A version that crashes release builds.

I noticed another HashMap in JavaScriptCore that might be prone to the same problem: the intIdentifierMap in npruntime.cpp. There might be more in WebCore.

[Darin Adler]

Created attachment 17665 [details]
patch

[Darin Adler]

(In reply to comment #10)
> I noticed another HashMap in JavaScriptCore that might be prone to the same
> problem: the intIdentifierMap in npruntime.cpp. There might be more in WebCore.

The intIdentifierMap will have a worse problem, because you can't use either a 0 or -1. In the array case, it's just 0 -- there's no issue with -1. We should probably track that issue with a separate bug.

Created attachment 17666 [details]
Different approach, without an extra branch

I started working on this before I saw Darin took the bug. This approach avoids extra work when accessing the array but initializing the empty values to -2 costs more than initializing to 0.

[Darin Adler]

(In reply to comment #13)
> Created an attachment (id=17666) [edit]
> Different approach, without an extra branch
> 
> I started working on this before I saw Darin took the bug. This approach avoids
> extra work when accessing the array but initializing the empty values to -2
> costs more than initializing to 0.

It's the JavaScript language standard that makes FFFFFFFF special, not our implementation. We can't just change maxArrayIndex; FFFFFFFD and FFFFFFFE have to work as array indices, whereas FFFFFFFF can be treated as just another non-array property.

So I suspect this won't work. We'd need a test case that sets a property with FFFFFFFE and checks the length of the array to demonstrate what's wrong with it.

(In reply to comment #14)
> So I suspect this won't work. We'd need a test case that sets a property with
> FFFFFFFE and checks the length of the array to demonstrate what's wrong with
> it.

Yup, to make it work we'd need to add an extra branch somewhere :-)

Comment on attachment 17665 [details]
patch

\ No newline at end of file
Please add a newline.

r=me

[Darin Adler]

Committed revision 28346.