Fix the topCallFrame pointer usage so that it is reliable and more accurate. <rdar://problem/11721461> <rdar://problem/11721462>
Created attachment 168058 [details] preliminary
Comment on attachment 168058 [details] preliminary View in context: https://bugs.webkit.org/attachment.cgi?id=168058&action=review > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:365 > + const size_t stackRedZoneSize = 16 * 1024; > + return m_stack.recursionCheck(stackRedZoneSize) Should we just change s_defaultAvailabilityDelta to 16kB? > Source/JavaScriptCore/interpreter/Interpreter.cpp:731 > + if (checker.isInRedZone() || (m_reentryDepth >= MaxSmallThreadReentryDepth && m_reentryDepth >= callFrame->globalData().maxReentryDepth)) Since you're using a precise check, you should remove m_reentryDepth and its associated less precise checks.
Comment on attachment 168058 [details] preliminary Attachment 168058 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14260200 New failing tests: fast/js/global-recursion-on-full-stack.html
Created attachment 168631 [details] Draft 1. (In reply to comment #2) > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:365 > > + const size_t stackRedZoneSize = 16 * 1024; > > + return m_stack.recursionCheck(stackRedZoneSize) > > Should we just change s_defaultAvailabilityDelta to 16kB? I've changed this to 32kB to be consistent with the estimated worst case in the Interpreter loop entry points. Will run some more test for this value. > > Source/JavaScriptCore/interpreter/Interpreter.cpp:731 > > + if (checker.isInRedZone() || (m_reentryDepth >= MaxSmallThreadReentryDepth && m_reentryDepth >= callFrame->globalData().maxReentryDepth)) > > Since you're using a precise check, you should remove m_reentryDepth and its associated less precise checks. Done.
Comment on attachment 168631 [details] Draft 1. Attachment 168631 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14293628
Comment on attachment 168631 [details] Draft 1. Attachment 168631 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14298633 New failing tests: fast/xmlhttprequest/xmlhttprequest-recursive-sync-event.html
Created attachment 169560 [details] Draft 2: not ready for review yet, just testing fix for ews reported issues.
Comment on attachment 169560 [details] Draft 2: not ready for review yet, just testing fix for ews reported issues. Attachment 169560 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14464369 New failing tests: svg/foreignObject/viewport-foreignobject-crash.html
Created attachment 170034 [details] Fix.
Comment on attachment 170034 [details] Fix. Win EWS bot is happy. It's ready for a review.
Comment on attachment 170034 [details] Fix. View in context: https://bugs.webkit.org/attachment.cgi?id=170034&action=review r=me > Source/JavaScriptCore/jit/JITStubs.cpp:2145 > + callFrame->setCodeBlock(0); Why did you need to do this? Your ChangeLog doesn't say. > Source/JavaScriptCore/jit/JITStubs.cpp:2227 > + callFrame->setCodeBlock(0); Why did you need to do this? Your ChangeLog doesn't say.
(In reply to comment #11) > (From update of attachment 170034 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170034&action=review > > r=me > > > Source/JavaScriptCore/jit/JITStubs.cpp:2145 > > + callFrame->setCodeBlock(0); > > Why did you need to do this? Your ChangeLog doesn't say. > > > Source/JavaScriptCore/jit/JITStubs.cpp:2227 > > + callFrame->setCodeBlock(0); > > Why did you need to do this? Your ChangeLog doesn't say. These were done in the jitCompileFor() and lazyLinkFor() JIT stub helper functions. Both of these functions were called from JIT stub functions which in turn were called by JIT glue trampoline code which are trying to call a callee function (for call or construct), but the callee needs to be linked or JIT compiled. In both cases, a call frame has been pushed and partially initialized. It is expected that the prologue of the callee will handle the initialization of the frame's codeBlock field. But this these cases, the callee is not known yet (which is why we're doing a link and JIT compile), and hence has not been called yet. But these JIT stub helpers will do work that may trigger a GC. The GC will scan the JSStack and step on an uninitialized codeBlock pointer. Hence, we initialize the codeBlock pointer to be 0 here to keep the GC happy. I will capture the above in comments in the code before I land the change. Thanks for the review.
Landed in r132182: <http://trac.webkit.org/changeset/132182>.
I'm afraid this patch caused a regression. Some API tests are asserting on EFL. http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/4926/steps/API%20tests/logs/stdio ASSERTION FAILED: (requiredCapacity >= 0) && (requiredCapacity < size) /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/JavaScriptCore/interpreter/Interpreter.cpp(195) : JSC::Interpreter::StackPolicy::StackPolicy(JSC::Interpreter&, const WTF::StackBounds&) 1 0x2b94f7b3393e JSC::Interpreter::StackPolicy::StackPolicy(JSC::Interpreter&, WTF::StackBounds const&) 2 0x2b94f7b36bb4 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) 3 0x2b94f7bf69fc JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) 4 0x2b94f26ba4dd WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) 5 0x2b94f26d7234 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld*) 6 0x2b94f26d7336 WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&) 7 0x2b94f1c58347 WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&) 8 0x2b94f1c57b9f WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) 9 0x2b94f1e491f8 WebCore::HTMLScriptRunner::runScript(WebCore::Element*, WTF::TextPosition const&) 10 0x2b94f1e48845 WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element>, WTF::TextPosition const&) 11 0x2b94f1e39b6b WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() 12 0x2b94f1e39c1d WebCore::HTMLDocumentParser::canTakeNextToken(WebCore::HTMLDocumentParser::SynchronousMode, WebCore::PumpSession&) 13 0x2b94f1e3a030 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) 14 0x2b94f1e39a18 WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode) 15 0x2b94f1e3a5c2 WebCore::HTMLDocumentParser::append(WebCore::SegmentedString const&) 16 0x2b94f1b6531d WebCore::DecodedDataDocumentParser::flush(WebCore::DocumentWriter*) 17 0x2b94f1fb272f WebCore::DocumentWriter::end() 18 0x2b94f1f9f6b1 WebCore::DocumentLoader::finishedLoading() 19 0x2b94f1fdbfb9 WebCore::MainResourceLoader::didFinishLoading(double) 20 0x2b94f1feeda5 WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) 21 0x2b94f2b3a14a 22 0x2b94f66b9775 23 0x2b94f66ce8dd g_simple_async_result_complete 24 0x2b94f66ce978 25 0x2b94f6381e53 g_main_context_dispatch 26 0x2b94f54bd16e 27 0x2b94f54b6dc9 28 0x2b94f54b7955 29 0x2b94f54b7c57 ecore_main_loop_begin 30 0x2b94f2b1cee9 WebCore::RunLoop::run() 31 0x2b94eec5cb88 WebProcessMainEfl
(In reply to comment #14) > I'm afraid this patch caused a regression. Some API tests are asserting on EFL. > > http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/4926/steps/API%20tests/logs/stdio > > ASSERTION FAILED: (requiredCapacity >= 0) && (requiredCapacity < size) > /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/JavaScriptCore/interpreter/Interpreter.cpp(195) : JSC::Interpreter::StackPolicy::StackPolicy(JSC::Interpreter&, const WTF::StackBounds&) > 1 0x2b94f7b3393e JSC::Interpreter::StackPolicy::StackPolicy(JSC::Interpreter&, WTF::StackBounds const&) > 2 0x2b94f7b36bb4 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) > 3 0x2b94f7bf69fc JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) > 4 0x2b94f26ba4dd WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) > 5 0x2b94f26d7234 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld*) > 6 0x2b94f26d7336 WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&) > 7 0x2b94f1c58347 WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&) > 8 0x2b94f1c57b9f WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) > 9 0x2b94f1e491f8 WebCore::HTMLScriptRunner::runScript(WebCore::Element*, WTF::TextPosition const&) > 10 0x2b94f1e48845 WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element>, WTF::TextPosition const&) > 11 0x2b94f1e39b6b WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() > 12 0x2b94f1e39c1d WebCore::HTMLDocumentParser::canTakeNextToken(WebCore::HTMLDocumentParser::SynchronousMode, WebCore::PumpSession&) > 13 0x2b94f1e3a030 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) > 14 0x2b94f1e39a18 WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode) > 15 0x2b94f1e3a5c2 WebCore::HTMLDocumentParser::append(WebCore::SegmentedString const&) > 16 0x2b94f1b6531d WebCore::DecodedDataDocumentParser::flush(WebCore::DocumentWriter*) > 17 0x2b94f1fb272f WebCore::DocumentWriter::end() > 18 0x2b94f1f9f6b1 WebCore::DocumentLoader::finishedLoading() > 19 0x2b94f1fdbfb9 WebCore::MainResourceLoader::didFinishLoading(double) > 20 0x2b94f1feeda5 WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) > 21 0x2b94f2b3a14a > 22 0x2b94f66b9775 > 23 0x2b94f66ce8dd g_simple_async_result_complete > 24 0x2b94f66ce978 > 25 0x2b94f6381e53 g_main_context_dispatch > 26 0x2b94f54bd16e > 27 0x2b94f54b6dc9 > 28 0x2b94f54b7955 > 29 0x2b94f54b7c57 ecore_main_loop_begin > 30 0x2b94f2b1cee9 WebCore::RunLoop::run() > 31 0x2b94eec5cb88 WebProcessMainEfl Actually looks like the crash was caused by the depends of this bug.