Bug 122255 – Make LLINT exception stack unwinding consistent with the JIT

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Currently, if you build a debug build and run the layout test fast/workers/stress-js-execution.html, you will see an assertion failure:

ASSERTION FAILED: !isTornOff()
/Volumes/Data/ws7/OpenSource/Source/JavaScriptCore/runtime/JSActivation.h(144) : void JSC::JSActivation::tearOff(JSC::VM &)
1   0x1078056a0 WTFCrash
2   0x10756c401 JSC::JSActivation::tearOff(JSC::VM&)
3   0x107569aac JSC::unwindCallFrame(JSC::StackVisitor&, JSC::JSValue)
4   0x10756c1e6 JSC::UnwindFunctor::operator()(JSC::StackVisitor&)
5   0x10756c0ba void JSC::StackVisitor::visit<JSC::UnwindFunctor>(JSC::ExecState*, JSC::UnwindFunctor&)
6   0x10756ab9d void JSC::ExecState::iterate<JSC::UnwindFunctor>(JSC::UnwindFunctor&)
7   0x107566baf JSC::Interpreter::unwind(JSC::ExecState*&, JSC::JSValue&, unsigned int)
8   0x107587543 JSC::genericUnwind(JSC::VM*, JSC::ExecState*, JSC::JSValue, unsigned int)
9   0x10758768c JSC::jitThrowNew(JSC::VM*, JSC::ExecState*, JSC::JSValue)
10  0x1075a8347 cti_vm_handle_exception
11  0x10759e289 ctiVMHandleException
12  0x107584d67 JSC::JITCode::execute(JSC::JSStack*, JSC::ExecState*, JSC::VM*)
13  0x107565024 JSC::Interpreter::execute(JSC::EvalExecutable*, JSC::ExecState*, JSC::JSValue, JSC::JSScope*)
14  0x107564761 JSC::eval(JSC::ExecState*)
15  0x1075a7054 cti_op_call_eval
16  0x1075a92c0 jscGeneratedNativeCode
17  0x107584d67 JSC::JITCode::execute(JSC::JSStack*, JSC::ExecState*, JSC::VM*)
18  0x107567efb JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*)
19  0x107341e7f JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*)
20  0x10a88c868 WebCore::WorkerScriptController::evaluate(WebCore::ScriptSourceCode const&, WebCore::ScriptValue*)
21  0x10a88c68c WebCore::WorkerScriptController::evaluate(WebCore::ScriptSourceCode const&)
22  0x10a8905d1 WebCore::WorkerThread::workerThread()
23  0x10a8902c5 WebCore::WorkerThread::workerThreadStart(void*)
…

This is because the WorkerThread is servicing a TerminationException which is trying to shut the worker thread down.  During exception handling, we end up unwinding the stack twice in the CommonSlowPath code: once in LLInt::returnToThrow() and the second time in ctiVMHandleException().

This occurs because the LLINT expects the CommonSlowPath code to unwind the stack before returning while the JIT expects the CommonSlowPath code to only set the exception object and leave the stack as is when it returns.  The fix is to make the LLINT unwind the stack if needed after the CommonSlowPath code returns just like the JIT, thereby making the CommonSlowPath code behave consistently for both the LLINT and the JIT.

Ref: <rdar://problem/14972616>

[Mark Lam]

Created attachment 213221 [details]
the patch.

[Filip Pizlo]

Comment on attachment 213221 [details]
the patch.

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

> Source/JavaScriptCore/llint/LLIntExceptions.cpp:47
> -static void doThrow(ExecState* exec)
> +void handleException(ExecState* exec)

Is this function called anywhere other than slow_path_handle_exception?

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:317
> +    // mlam jmp _llint_throw_from_slow_path_trampoline
> +

What is this?

[Mark Lam]

Comment on attachment 213221 [details]
the patch.

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

>> Source/JavaScriptCore/llint/LLIntExceptions.cpp:47
>> +void handleException(ExecState* exec)
> 
> Is this function called anywhere other than slow_path_handle_exception?

Nope.  I'll merge the code over.

>> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:317
>> +
> 
> What is this?

Darn!  That was an experiment to check if my change caused some floating point result rounding errors in the 32-bit build.  The result: rounding errors not due to my changes.  But I accidentally left this experimental code in there.  Will fix.

[Filip Pizlo]

Comment on attachment 213221 [details]
the patch.

r=me if you make the changes you said in Comment #3.

[Mark Lam]

Created attachment 213224 [details]
Revised patch.

Thanks for the review.  Will land after running some tests on this new patch.

[Mark Lam]

Landed in r156818: <http://trac.webkit.org/r156818>.