| 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: | P1 | 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
Alexey Proskuryakov
2014-12-11 11:57:14 PST
Skipped in <http://trac.webkit.org/r177174>. 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();
}
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. 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. Updated expectations in <http://trac.webkit.org/r177268>. 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. (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. 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. Created attachment 243419 [details]
Patch
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? (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. 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? (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 Created attachment 243452 [details]
Updated patch
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. Committed r177460: <http://trac.webkit.org/changeset/177460> |