Bug 237176 – [JSC] Use DeferTerminationForAWhile in Interpreter::unwind

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[JSC] Use DeferTerminationForAWhile in Interpreter::unwind

[Yusuke Suzuki]

Created attachment 453168 [details]
Patch

[Mark Lam]

Comment on attachment 453168 [details]
Patch

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

r=me

> Source/JavaScriptCore/interpreter/Interpreter.cpp:704
> +    // otherwise, we can invoke a found erorr hanlder with a termination exception.

/erorr hanlder/error handler/

[Mark Lam]

Comment on attachment 453168 [details]
Patch

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

>> Source/JavaScriptCore/interpreter/Interpreter.cpp:704
>> +    // otherwise, we can invoke a found erorr hanlder with a termination exception.
> 
> /erorr hanlder/error handler/

Actually, let's add a bit more detail:
otherwise, we can end up in a found error handler with a TerminationException, and effectively "catch" the TerminationException which should not be catchable.

[Yusuke Suzuki]

Committed r290516 (247800@trunk): <https://commits.webkit.org/247800@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/89482527>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 237222

[Yusuke Suzuki]

Created attachment 453255 [details]
Patch

[Mark Lam]

Comment on attachment 453255 [details]
Patch

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

r=me

> Source/JavaScriptCore/interpreter/Interpreter.cpp:712
> +    // On the other hand, we do not clear already a raised termination exception because it is possible

/already a raised/an already raised/

> Source/JavaScriptCore/interpreter/Interpreter.cpp:715
> +    // What we would like to achieve here is do not raise a new termination exception while running

/is do not/is to not/

[Yusuke Suzuki]

Created attachment 453278 [details]
Patch

[Mark Lam]

Comment on attachment 453278 [details]
Patch

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

r=me

> Source/JavaScriptCore/interpreter/Interpreter.cpp:720
> +    // While running Interpreter::unwind, we must not accept concurrently set termination exception,
> +    // otherwise, we can end up in a found error handler with a TerminationException, and effectively
> +    // "catch" the TerminationException which should not be catchable.
> +    //
> +    // On the other hand, we do not clear already a raised termination exception because it is possible
> +    // that we are calling this unwind because we are handling this termination exception.
> +    //
> +    // What we would like to achieve here is not raising a new termination exception while running
> +    // Interpreter::unwind, including just after finishing Interpreter::unwind, but we do not want
> +    // to clear already raised exception.
> +    //
> +    // But, we do not want to use DeferTerminationForAWhile when we are handling termination exception: this
> +    // is because we will unwind everything here, and in that case DeferTerminationForAWhile leaves
> +    // NeedTermination in the VM. This is not correct since after unwinding, NeedTermination bit should be
> +    // cleared for VM.

Let's rephrase this as:
```
If we're unwinding the stack due to a regular exception (not a TerminationException), then we want to use a DeferTerminationForAWhile scope. This is because we want to avoid a TerminationException being raised (due to a concurrent termination request) in the middle of unwinding. The unwinding code only checks if we're handling a TerminationException before it starts unwinding and is not expecting this status to change in the middle.  Without the DeferTerminationForAWhile scope, control flow may end up in an exception handler, and effectively "catch" the newly raised TerminationException, which should not be catchable.

On the other hand, if we're unwinding the stack due to a TerminationException, we do not need nor want the DeferTerminationForAWhile scope.  This is because on exit, DeferTerminationForAWhile will set the VMTraps NeedTermination bit if termination is in progress.  The system expects the NeedTermination bit to be have been cleared by VMTraps::handleTraps() once the TerminationException has been raised.  Some legacy client apps relies on this and expects to be able to re-enter the VM after it exits due to termination.  If the NeedTermination bit is set, upon re-entry, the VM will behave as if a termination request is pending and terminate almost immediately, thereby breaking the legacy client apps.

FIXME: Revisit this once we can deprecate this legacy behavior of being able to re-enter the VM after termination.
```

Can you also use this blurb (without the FIXME part) in the ChangeLog or an adaption of it?

[Yusuke Suzuki]

Created attachment 453282 [details]
Patch

[Yusuke Suzuki]

Committed r290603 (?): <https://commits.webkit.org/r290603>