Bug 126009 – CStack: Update the VMEntryScope's stack limit when the VM enters/exits ErrorMode

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

[Mark Lam]

See JSStack::enableErrorStackReserve() and JSStack::disableErrorStackReserve().  Just like this is needed for handling StackOverflowErrors when using the separate JSStack, we need to implement the equivalent for the C stack.

[Geoffrey Garen]

Can we just rely on handling errors inside the host zone? Do we need another concept?

[Mark Lam]

(In reply to comment #1)
> Can we just rely on handling errors inside the host zone? Do we need another concept?

We need a distinction because we need to allow the VM to use some additional stack space in order to do the work of throwing a StackOverflowError after we have reached the stack limit for normal execution.  Otherwise, we’ll encounter a stack overflow while trying to throw the StackOverflowError.

[Geoffrey Garen]

(In reply to comment #2)
> (In reply to comment #1)
> > Can we just rely on handling errors inside the host zone? Do we need another concept?
> 
> We need a distinction because we need to allow the VM to use some additional stack space in order to do the work of throwing a StackOverflowError after we have reached the stack limit for normal execution.  Otherwise, we’ll encounter a stack overflow while trying to throw the StackOverflowError.

Yes, that explains what we're trying to do. I have a suggestion for how: What you're calling "JSStack:: enableErrorStackReserve()" should just adjust the stack limit by half the size of the host zone.

[Mark Lam]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > Can we just rely on handling errors inside the host zone? Do we need another concept?
> > 
> > We need a distinction because we need to allow the VM to use some additional stack space in order to do the work of throwing a StackOverflowError after we have reached the stack limit for normal execution.  Otherwise, we’ll encounter a stack overflow while trying to throw the StackOverflowError.
> 
> Yes, that explains what we're trying to do. I have a suggestion for how: What you're calling "JSStack:: enableErrorStackReserve()" should just adjust the stack limit by half the size of the host zone.

Yes, that is pretty much what I had in mind.  I hadn’t thought of how much to adjust it by yet, but half the host zone sounds reasonable.

[Mark Lam]

The C stack limit tracking mechanism in the VMEntryScope class already has allowance for a smaller host zone when the Interpreter is in an ErrorMode.  We just need to update the VMEntryScope's stack limit whenever the Interpreter enters and exits its ErrorMode.

[Mark Lam]

Created attachment 219845 [details]
the patch.

[Mark Lam]

Comment on attachment 219845 [details]
the patch.

Wrong patch.

[Mark Lam]

Created attachment 219846 [details]
the real patch.

[Mark Lam]

Comment on attachment 219846 [details]
the real patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=219846&action=review

> Source/JavaScriptCore/interpreter/JSStack.cpp:189
> +#else
> +    if (m_vm.entryScope)
> +        m_vm.entryScope->updateStackLimit();
> +#endif

After thinking about this some more, the C stack update should be unconditional independent of whether the JS stack is on the C stack or its own separate stack.  We may have detected a stack overflow due to the C stack reaching it's limit.  Hence, the VM may need more C stack space to process the error even if the JS stack is on its own separate stack.  I will upload an updated patch.

[Mark Lam]

Created attachment 219847 [details]
patch 2.

[Mark Lam]

Landed in r160967 on the jsCStack branch: <http://trac.webkit.org/r160967>.

[Mark Lam]

Thanks for the review.  Review status updated in r161117: <http://trac.webkit.org/r161117>.