Bug 139548 – Tests with infinite recursion frequently crash

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

fast/workers/worker-constructor.html crashes every time on 10.8 and 10.9 release bots. 

This started to happen in <http://trac.webkit.org/log/?verbose=on&rev=177149&stop_rev=177144>, although the test used to crash in the same way before that (quite rarely).

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010f1b3126 JSC::CodeBlock::handlerForBytecodeOffset(unsigned int) + 6 (RefCountedArray.h:124)
1   com.apple.JavaScriptCore      	0x000000010f41002f JSC::UnwindFunctor::operator()(JSC::StackVisitor&) + 111 (Interpreter.cpp:649)
2   com.apple.JavaScriptCore      	0x000000010f40cd8b JSC::Interpreter::unwind(void*&, JSC::ExecState*&, JSC::JSValue&) + 587 (StackVisitor.h:127)
3   com.apple.JavaScriptCore      	0x000000010f42b2db JSC::genericUnwind(JSC::VM*, JSC::ExecState*, JSC::JSValue) + 91 (JITExceptions.cpp:52)
4   com.apple.JavaScriptCore      	0x000000010f44f105 lookupExceptionHandlerFromCallerFrame + 85 (Interpreter.h:197)
5   ???                           	0x00003b1d5220aa88 0 + 64997117962888
6   com.apple.JavaScriptCore      	0x000000010f51d4b8 vmEntryToJavaScript + 326
7   com.apple.JavaScriptCore      	0x000000010f4299d9 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 169 (VM.h:378)
8   com.apple.JavaScriptCore      	0x000000010f40f039 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 489 (Interpreter.cpp:978)
9   com.apple.JavaScriptCore      	0x000000010f1a013e JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 62 (CallData.cpp:39)
10  com.apple.JavaScriptCore      	0x000000010f4a890d JSC::JSObject::defaultValue(JSC::JSObject const*, JSC::ExecState*, JSC::PreferredPrimitiveType) + 525 (Register.h:116)
11  com.apple.JavaScriptCore      	0x000000010f47ca0c JSC::JSValue::toStringSlowCase(JSC::ExecState*) const + 732 (JSCJSValue.cpp:369)
12  com.apple.WebCore             	0x00000001117651a4 WebCore::constructJSWorker(JSC::ExecState*) + 132 (JSString.h:667)
13  com.apple.JavaScriptCore      	0x000000010f44f6fd JSC::handleHostCall(JSC::ExecState*, JSC::JSValue, JSC::CodeSpecializationKind) + 189 (JITOperations.cpp:667)
14  com.apple.JavaScriptCore      	0x000000010f44f965 linkFor + 101 (JITOperations.cpp:690)
15  ???                           	0x00003b1d5220a2c4 0 + 64997117960900
16  ???                           	0x00003b1d5220a97f 0 + 64997117962623
17  com.apple.JavaScriptCore      	0x000000010f51d4b8 vmEntryToJavaScript + 326
18  com.apple.JavaScriptCore      	0x000000010f4299d9 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 169 (VM.h:378)
19  com.apple.JavaScriptCore      	0x000000010f40f039 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 489 (Interpreter.cpp:978)
20  com.apple.JavaScriptCore      	0x000000010f1a013e JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 62 (CallData.cpp:39)
21  com.apple.JavaScriptCore      	0x000000010f4a890d JSC::JSObject::defaultValue(JSC::JSObject const*, JSC::ExecState*, JSC::PreferredPrimitiveType) + 525 (Register.h:116)
22  com.apple.JavaScriptCore      	0x000000010f47ca0c JSC::JSValue::toStringSlowCase(JSC::ExecState*) const + 732 (JSCJSValue.cpp:369)
23  com.apple.WebCore             	0x00000001117651a4 WebCore::constructJSWorker(JSC::ExecState*) + 132 (JSString.h:667)
24  com.apple.JavaScriptCore      	0x000000010f44f6fd JSC::handleHostCall(JSC::ExecState*, JSC::JSValue, JSC::CodeSpecializationKind) + 189 (JITOperations.cpp:667)
25  com.apple.JavaScriptCore      	0x000000010f44f965 linkFor + 101 (JITOperations.cpp:690)
26  ???                           	0x00003b1d5220a2c4 0 + 64997117960900
27  ???                           	0x00003b1d5220a97f 0 + 64997117962623
28  com.apple.JavaScriptCore      	0x000000010f51d4b8 vmEntryToJavaScript + 326
29  com.apple.JavaScriptCore      	0x000000010f4299d9 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 169 (VM.h:378)
30  com.apple.JavaScriptCore      	0x000000010f40f039 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 489 (Interpreter.cpp:978)
31  com.apple.JavaScriptCore      	0x000000010f1a013e JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 62 (CallData.cpp:39)
32  com.apple.JavaScriptCore      	0x000000010f4a890d JSC::JSObject::defaultValue(JSC::JSObject const*, JSC::ExecState*, JSC::PreferredPrimitiveType) + 525 (Register.h:116)
33  com.apple.JavaScriptCore      	0x000000010f47ca0c JSC::JSValue::toStringSlowCase(JSC::ExecState*) const + 732 (JSCJSValue.cpp:369)
34  com.apple.WebCore             	0x00000001117651a4 WebCore::constructJSWorker(JSC::ExecState*) + 132 (JSString.h:667)
35  com.apple.JavaScriptCore      	0x000000010f44f6fd JSC::handleHostCall(JSC::ExecState*, JSC::JSValue, JSC::CodeSpecializationKind) + 189 (JITOperations.cpp:667)
36  com.apple.JavaScriptCore      	0x000000010f44f965 linkFor + 101 (JITOperations.cpp:690)
37  ???                           	0x00003b1d5220a2c4 0 + 64997117960900
38  ???                           	0x00003b1d5220a97f 0 + 64997117962623
39  com.apple.JavaScriptCore      	0x000000010f51d4b8 vmEntryToJavaScript + 326
40  com.apple.JavaScriptCore      	0x000000010f4299d9 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 169 (VM.h:378)
41  com.apple.JavaScriptCore      	0x000000010f40f039 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 489 (Interpreter.cpp:978)
42  com.apple.JavaScriptCore      	0x000000010f1a013e JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 62 (CallData.cpp:39)
...

[Alexey Proskuryakov]

Skipped in <http://trac.webkit.org/r177174>.

[Michael Saboff]

The test itself creates the recursion.  The bug appears to be that we crash sometimes when handling the stack overflow exception.

function testRecursiveWorkerCreation()
{
    try {
        var foo = {toString:function(){new Worker(foo)}}
        new Worker(foo);
        log("FAIL: no exception when trying to create workers recursively");
    } catch (ex) {
        log("PASS: trying to create workers recursively resulted in an exception (" + ex + ")");
    }
    runNextTest();
}

[Michael Saboff]

I suspect that the issue is in lookupExceptionHandlerFromCallerFrame() when it gets its caller's frame.  That it isn't getting a JS frame, but a native frame.  It is hard to prove without being able to reproduce the issue.

Thinking about how to write a reliable test.

[Alexey Proskuryakov]

Another test for recursion started to crash today in a similar manner, fast/xmlhttprequest/xmlhttprequest-recursive-sync-event.html. This started with <http://trac.webkit.org/changeset/177264>, which is clearly unrelated.

Given that we started to see multiple infinite recursion tests misbehave any time anything changes in stack depth, there must have been a very recent regression that broke this for real.

[Alexey Proskuryakov]

Updated expectations in <http://trac.webkit.org/r177268>.

[Geoffrey Garen]

It's possible that we had a bug all along, and <http://trac.webkit.org/changeset/177146>, by changing the depth of the stack, triggered the bug in our tests.

[Michael Saboff]

(In reply to comment #6)
> It's possible that we had a bug all along, and
> <http://trac.webkit.org/changeset/177146>, by changing the depth of the
> stack, triggered the bug in our tests.

That is the assumption I'm working under.  I'm developing a test case now to catch this.

[Alexey Proskuryakov]

This is what I think too, expect that I'm also saying that the underlying bug was likely introduced very recently.

Neither r177146 not r177264 introduced the root cause, but the fact that unrelated check-ins made within a few days triggered similar problems in different tests fairly strongly indicates that we didn't have the problem until recently.

[Michael Saboff]

rdar://problem/19272941

[Michael Saboff]

Created attachment 243419 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 243419 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243419&action=review

> LayoutTests/ChangeLog:10
> +        * js/regress-139548-expected.txt: Added.

Nice! Could you please also unskip the two tests skipped due to this issue?

[Michael Saboff]

(In reply to comment #11)
> Comment on attachment 243419 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243419&action=review
> 
> > LayoutTests/ChangeLog:10
> > +        * js/regress-139548-expected.txt: Added.
> 
> Nice! Could you please also unskip the two tests skipped due to this issue?

I will unskip the three tests skipped in <http://trac.webkit.org/changeset/177268> when I check this in.

[Geoffrey Garen]

Comment on attachment 243419 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243419&action=review

> Source/JavaScriptCore/interpreter/Interpreter.cpp:650
> +        if (!m_isTermination && !m_codeBlock)
> +            return StackVisitor::Continue;

Two things are incorrect about this:

(1) It will continue past a host call frame, even though unwinding is supposed to stop at host call frames;

(2) It will unwind a frame without notifying the profiler.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:-672
> -    ASSERT(codeBlock);

When/why is it valid to attempt to unwind the current frame without a code block?

[Michael Saboff]

(In reply to comment #13)
> Comment on attachment 243419 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243419&action=review
> 
> > Source/JavaScriptCore/interpreter/Interpreter.cpp:650
> > +        if (!m_isTermination && !m_codeBlock)
> > +            return StackVisitor::Continue;
> 
> Two things are incorrect about this:
> 
> (1) It will continue past a host call frame, even though unwinding is
> supposed to stop at host call frames;
> 
> (2) It will unwind a frame without notifying the profiler.

I'll post a patch with a fix.
 
> > Source/JavaScriptCore/interpreter/Interpreter.cpp:-672
> > -    ASSERT(codeBlock);
> 
> When/why is it valid to attempt to unwind the current frame without a code
> block?

If the stack overflow check at the top of a JS function fails, which is before we allocate locals on the stack, we throw an out of stack exception.  We can't process the exception with the callee's frame, it is incomplete.  Also, we have to handle the exception somewhere up the stack from the callee.  Therefore we start unwinding beginning with the caller's frame.  If the caller is native code it won't have a code block.  Here is the top of a back trace from that ASSERT in just such a case:

 bt 40
* thread #1: tid = 0x793ad, 0x00000001009d90aa JavaScriptCore`WTFCrash + 42 at Assertions.cpp:321, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbbadbeef)
  * frame #0: 0x00000001009d90aa JavaScriptCore`WTFCrash + 42 at Assertions.cpp:321
    frame #1: 0x000000010060574d JavaScriptCore`JSC::Interpreter::unwind(this=0x0000000105ff4000, vmEntryFrame=0x00007fff5f81f668, callFrame=0x00007fff5f81f678, exceptionValue=0x00007fff5f81f688) + 93 at Interpreter.cpp:674
    frame #2: 0x000000010062667a JavaScriptCore`JSC::genericUnwind(vm=0x0000000104010000, callFrame=0x00007fff5f81fb90, exceptionValue=JSValue at 0x00007fff5f81f688) + 202 at JITExceptions.cpp:52
    frame #3: 0x0000000100638b33 JavaScriptCore`lookupExceptionHandlerFromCallerFrame(vm=0x0000000104010000, exec=0x00007fff5f81f700) + 227 at JITOperations.cpp:1867
    frame #4: 0x00005177fc803261
    frame #5: 0x0000000100798dd9 JavaScriptCore`vmEntryToJavaScript + 361
    frame #6: 0x00000001006230aa JavaScriptCore`JSC::JITCode::execute(this=0x00000001057ce000, vm=0x0000000104010000, protoCallFrame=0x00007fff5f81f8f0) + 266 at JITCode.cpp:77
    frame #7: 0x00000001006076f4 JavaScriptCore`JSC::Interpreter::executeCall(this=0x0000000105ff4000, callFrame=0x00007fff5f81fb90, function=0x0000000103cec230, callType=CallTypeJS, callData=0x00007fff5f81fb30, thisValue=JSValue at 0x00007fff5f81f9d0, args=0x00007fff5f81faf0) + 1508 at Interpreter.cpp:980
    frame #8: 0x000000010010e45e JavaScriptCore`JSC::call(exec=0x00007fff5f81fb90, functionObject=JSValue at 0x00007fff5f81fab0, callType=CallTypeJS, callData=0x00007fff5f81fb30, thisValue=JSValue at 0x00007fff5f81faa8, args=0x00007fff5f81faf0) + 190 at CallData.cpp:39
    frame #9: 0x00000001007fb77c JavaScriptCore`JSC::objectProtoFuncToLocaleString(exec=0x00007fff5f81fb90) + 364 at ObjectPrototype.cpp:210
    frame #10: 0x00005177fc801034
    frame #11: 0x000051783c80050b
    frame #12: 0x000051783c800715
    frame #13: 0x000051783c800715
    frame #14: 0x000051783c800715

[Michael Saboff]

Created attachment 243452 [details]
Updated patch

[Geoffrey Garen]

Comment on attachment 243452 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243452&action=review

r=me

> Source/JavaScriptCore/interpreter/Interpreter.cpp:650
> +        if (m_isTermination || !(m_handler = m_codeBlock ? m_codeBlock->handlerForBytecodeOffset(bytecodeOffset) : 0)) {

Should be nullptr instead of 0.

[Michael Saboff]

Committed r177460: <http://trac.webkit.org/changeset/177460>