[webkit-reviews] review granted: [Bug 237176] [JSC] Use DeferTerminationForAWhile in Interpreter::unwind : [Attachment 453278] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 237176: [JSC] Use DeferTerminationForAWhile in Interpreter::unwind
https://bugs.webkit.org/show_bug.cgi?id=237176

Attachment 453278: Patch

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




--- Comment #10 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 453278
  --> https://bugs.webkit.org/attachment.cgi?id=453278
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?