There should be 2 separate reserved zone sizes for JS and host code (we currently only have one reserved zone size for both), and the JS code reserved zone size needs to be larger than the host code reserved zone size. For example, the JS reserved zone size needs to be large enough to provide sufficient CPU stack capacity for the max recursion taken by the parser to recurse when parsing any builtin. In contrast, the host code reserved zone size only needs to be large enough for one recursion entry into the parser.
<rdar://problem/26889188>
Zones of what? Stack, or something else?
(In reply to comment #2) > Zones of what? Stack, or something else? Stacks.
Created attachment 282857 [details] work in progress: still needs ChangeLogs. Let's try this patch on the EWS for size while we wait for https://bugs.webkit.org/show_bug.cgi?id=159451 to be reviewed.
Comment on attachment 282857 [details] work in progress: still needs ChangeLogs. Attachment 282857 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1634438 New failing tests: js/regress-139548.html
Created attachment 282860 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 282857 [details] work in progress: still needs ChangeLogs. Attachment 282857 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1634446 New failing tests: js/regress-139548.html
Created attachment 282861 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 282857 [details] work in progress: still needs ChangeLogs. Attachment 282857 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1634452 New failing tests: js/regress-139548.html
Created attachment 282862 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Created attachment 283010 [details] work in progress: for EWS testing.
Created attachment 283351 [details] proposed patch. Let's get some EWS testing.
Comment on attachment 283351 [details] proposed patch. EWS is having a bad time, but not due to my patch. Lets get a review. I'll try to test this locally.
Comment on attachment 283351 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=283351&action=review > Source/JavaScriptCore/interpreter/CLoopStack.cpp:156 > +bool CLoopStack::isSafeToRecurseCLoop(size_t neededStackInBytes) const Let's not put the class name inside the function name. > Source/JavaScriptCore/runtime/Options.h:116 > + v(unsigned, reservedZoneSize, 128 * KB, Normal, "Stack reserved zone size for JS code usage") \ This is the space we reserve for host code, not for JS code. > Source/JavaScriptCore/runtime/Options.h:117 > + v(unsigned, errorModeReservedZoneSize, 64 * KB, Normal, "Stack reserved zone size for JS code usage when handling errors") \ Same. > Source/JavaScriptCore/runtime/VM.h:630 > + bool isSafeToRecurseInternal(void* stackLimit, size_t neededStackInBytes) const "Internal" is an anti-pattern in naming. Let's just move this code into the caller. > Source/JavaScriptCore/runtime/VMInlines.h:50 > +bool VM::isSafeToRecurseWithReserve(size_t neededStackInBytes) const Does anybody use the neededStackInBytes parameter? I don't think so. Let's remove it.
Comment on attachment 283351 [details] proposed patch. Attachment 283351 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1665334 New failing tests: js/regress-139548.html
Created attachment 283363 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.5
Created attachment 283372 [details] proposed patch:addressed Geoff's feedback + speculative fix for the iOS EWS tummy ache.
Created attachment 283373 [details] proposed patch.
Created attachment 283389 [details] proposed patch.
Comment on attachment 283389 [details] proposed patch. Attachment 283389 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1667075 New failing tests: js/regress-139548.html
Created attachment 283393 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.5
Created attachment 283424 [details] proposed patch.
The EWS bots are now complaining about: Regressions: Unexpected timeouts (1) http/tests/preload/single_download_preload_runner.html [ Timeout ] I think that this failure is not related to my patch. Testing locally, I cannot reproduce this issue. The good news is: the speculative fix for js/regress-139548.html worked. The iOS SIM EWS is no longer failing on that. I think we're ready for a review.
Comment on attachment 283424 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=283424&action=review It looks like this patch doesn't quite fix the builtin crash. If you do toString() on an exception, and it recurses a lot, and then it parses a builtin, parsing will still fail, since toString() on an exception and builtin parsing use the same stack limit. It's probably OK for this very edge case to throw an exception -- but we need a follow-up patch to make sure that it does not RELEASE_ASSERT and crash. > Source/JavaScriptCore/runtime/Options.h:116 > + v(unsigned, reservedZoneSize, 128 * KB, Normal, "Stack reserved zone size for host code that may re-enter JS code") \ The amount of stack JSC usually reserves for host code. Let's call this softReservedZoneSize. > Source/JavaScriptCore/runtime/Options.h:118 > + v(unsigned, errorModeReservedZoneSize, 64 * KB, Normal, "Stack reserved zone size when handling errors for host code that may re-enter JS code") \ > + v(unsigned, osReservedZoneSize, 64 * KB, Normal, "Stack reserved zone size for host code that will not re-enter JS code") \ These two numbers are the same. Let's use one. This is the amount of stack JSC guarantees for client and VM code. Let's call this reservedZoneSize. The point of this value is that, if you're going to call out to client or VM code, you must guarantee at least this much space. If you're not going to call out to client code, or if you're already running in the VM, you are free to use this space if you want. > Source/JavaScriptCore/runtime/VM.cpp:664 > m_osStackLimitWithReserve = wtfThreadData().stack().recursionLimit(startOfStack, Options::maxPerThreadStackUsage(), m_reservedZoneSize); Let's call this m_softStackLimit. > Source/JavaScriptCore/runtime/VM.cpp:665 > + m_osStackLimit = wtfThreadData().stack().recursionLimit(startOfStack, Options::maxPerThreadStackUsage(), osReservedZoneSize); Let's call this m_stackLimit. > Source/JavaScriptCore/runtime/VM.h:476 > + inline bool isSafeToRecurseWithReserve() const; Let's call this isSafeToRecurseSoft. > Source/JavaScriptCore/runtime/VMInlines.h:50 > +bool VM::isSafeToRecurseWithReserve() const isSafeToRecurseSoft
Created attachment 283471 [details] proposed patch.
Comment on attachment 283471 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=283471&action=review > Source/JavaScriptCore/ChangeLog:28 > + Options::reservedZoneSize() is the amount of the stack space that JSC guarantees > + to the VM and client host code for it's use. Host code that has well known > + stack usage characteristics (i.e. doesn't call arbitrary code) may do stack > + checks against the VM::m_stackLimit limit (which is computed using > + Options::reservedZoneSize()). > + > + Options::softReservedZoneSize() is a more conservative amount of reserved stack > + space. This is used to compute the VM::m_softStackLimit limit. Any code that > + is difficult to have its stack usage characterized (i.e. may call arbitrary code) > + may need more stack space for its work. Hence, these should do stack checks > + against the VM::m_softStackLimit limit. Let's put our best understanding of these values in code, so people will see it most. I think that looks like this: v(unsigned, softReservedZoneSize, 128 * KB, Normal, "A buffer on top of reservedZoneSize that reserves space for stringifying exceptions.") \ v(unsigned, reservedZoneSize, 64 * KB, Normal, "The amount of stack space we guarantee to our clients (and to interal VM code that does not call out to clients).") \
Thanks for the review. Landed in r203142: <http://trac.webkit.org/r203142>.
C Loop build fix landed in r203144: <http://trac.webkit.org/r203144>.
(In reply to comment #28) > C Loop build fix landed in r203144: <http://trac.webkit.org/r203144>. C loop is still broken: Source/JavaScriptCore/CMakeFiles/LLIntOffsetsExtractor.dir/llint/LLIntOffsetsExtractor.cpp.o:LLIntOffsetsExtractor.cpp:function JSC::CLoopStack::isSafeToRecurse() const: error: undefined reference to 'JSC::ExecState::topOfFrameInternal()' collect2: error: ld returned 1 exit status
(In reply to comment #29) > (In reply to comment #28) > > C Loop build fix landed in r203144: <http://trac.webkit.org/r203144>. > > C loop is still broken: > Source/JavaScriptCore/CMakeFiles/LLIntOffsetsExtractor.dir/llint/ > LLIntOffsetsExtractor.cpp.o:LLIntOffsetsExtractor.cpp:function > JSC::CLoopStack::isSafeToRecurse() const: error: undefined reference to > 'JSC::ExecState::topOfFrameInternal()' > collect2: error: ld returned 1 exit status new bug report to fix the build: bug159706