Bug 139548

Summary:

Tests with infinite recursion frequently crash

Product:

WebKit

Reporter:

Alexey Proskuryakov <ap>

Component:

JavaScriptCore

Assignee:

Michael Saboff <msaboff>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ggaren, msaboff, webkit-bug-importer

Priority:

Keywords:

InRadar, LayoutTestFailure, MakingBotsRed

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=132281
https://bugs.webkit.org/show_bug.cgi?id=139840

Attachments:

Description	Flags
Patch	ggaren: review-
Updated patch	ggaren: review+

Alexey Proskuryakov

Reported 2014-12-11 11:57:14 PST

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) ...

Attachments
Patch (6.36 KB, patch) 2014-12-16 18:25 PST, Michael Saboff	ggaren: review-	Details Formatted Diff Diff
Updated patch (9.04 KB, patch) 2014-12-17 12:34 PST, Michael Saboff	ggaren: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2014-12-11 12:40:30 PST

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

Michael Saboff

Comment 2 2014-12-11 13:33:01 PST

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

Comment 3 2014-12-11 13:51:54 PST

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

Comment 4 2014-12-14 18:34:25 PST

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

Comment 5 2014-12-14 18:38:27 PST

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

Geoffrey Garen

Comment 6 2014-12-15 11:44:02 PST

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

Comment 7 2014-12-15 11:50:22 PST

(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

Comment 8 2014-12-15 12:01:23 PST

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

Comment 9 2014-12-16 17:36:26 PST

rdar://problem/19272941

Michael Saboff

Comment 10 2014-12-16 18:25:42 PST

Created attachment 243419 [details] Patch

Alexey Proskuryakov

Comment 11 2014-12-16 18:45:17 PST

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

Comment 12 2014-12-16 19:13:45 PST

(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 13 2014-12-17 10:47:30 PST

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

Comment 14 2014-12-17 11:26:02 PST

(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

Comment 15 2014-12-17 12:34:40 PST

Created attachment 243452 [details] Updated patch

Geoffrey Garen

Comment 16 2014-12-17 13:12:30 PST

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

Comment 17 2014-12-17 13:37:54 PST

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

Note You need to log in before you can comment on or make changes to this bug.