Bug 159442 – We should use different stack limits for stack checks from JS and host code.

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

[Mark Lam]

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.

[Mark Lam]

<rdar://problem/26889188>

[Filip Pizlo]

Zones of what?  Stack, or something else?

[Mark Lam]

(In reply to comment #2)
> Zones of what?  Stack, or something else?

Stacks.

[Mark Lam]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Mark Lam]

Created attachment 283010 [details]
work in progress: for EWS testing.

[Mark Lam]

Created attachment 283351 [details]
proposed patch.

Let's get some EWS testing.

[Mark Lam]

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.

[Geoffrey Garen]

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.

[Build Bot]

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

[Build Bot]

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

[Mark Lam]

Created attachment 283372 [details]
proposed patch:addressed Geoff's feedback + speculative fix for the iOS EWS tummy ache.

[Mark Lam]

Created attachment 283373 [details]
proposed patch.

[Mark Lam]

Created attachment 283389 [details]
proposed patch.

[Build Bot]

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

[Build Bot]

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

[Mark Lam]

Created attachment 283424 [details]
proposed patch.

[Mark Lam]

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.

[Geoffrey Garen]

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

[Mark Lam]

Created attachment 283471 [details]
proposed patch.

[Geoffrey Garen]

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

[Mark Lam]

Thanks for the review.  Landed in r203142: <http://trac.webkit.org/r203142>.

[Mark Lam]

C Loop build fix landed in r203144: <http://trac.webkit.org/r203144>.

[Csaba Osztrogonác]

(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

[Csaba Osztrogonác]

(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