Bug 237176 - [JSC] Use DeferTerminationForAWhile in Interpreter::unwind
Summary: [JSC] Use DeferTerminationForAWhile in Interpreter::unwind
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 237222
Blocks:
  Show dependency treegraph
 
Reported: 2022-02-24 19:45 PST by Yusuke Suzuki
Modified: 2022-02-28 10:01 PST (History)
8 users (show)

See Also:


Attachments
Patch (2.66 KB, patch)
2022-02-24 19:49 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (11.24 KB, patch)
2022-02-25 13:54 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (6.14 KB, patch)
2022-02-25 17:17 PST, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff
Patch (7.16 KB, patch)
2022-02-25 19:15 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2022-02-24 19:45:36 PST
[JSC] Use DeferTerminationForAWhile in Interpreter::unwind
Comment 1 Yusuke Suzuki 2022-02-24 19:49:05 PST
Created attachment 453168 [details]
Patch
Comment 2 Mark Lam 2022-02-24 19:51:35 PST
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/
Comment 3 Mark Lam 2022-02-24 19:55:06 PST
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.
Comment 4 Yusuke Suzuki 2022-02-25 10:53:41 PST
Committed r290516 (247800@trunk): <https://commits.webkit.org/247800@trunk>
Comment 5 Radar WebKit Bug Importer 2022-02-25 10:54:24 PST
<rdar://problem/89482527>
Comment 6 WebKit Commit Bot 2022-02-25 12:59:10 PST
Re-opened since this is blocked by bug 237222
Comment 7 Yusuke Suzuki 2022-02-25 13:54:15 PST
Created attachment 453255 [details]
Patch
Comment 8 Mark Lam 2022-02-25 14:50:49 PST
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/
Comment 9 Yusuke Suzuki 2022-02-25 17:17:12 PST
Created attachment 453278 [details]
Patch
Comment 10 Mark Lam 2022-02-25 18:21:33 PST
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?
Comment 11 Yusuke Suzuki 2022-02-25 19:15:02 PST
Created attachment 453282 [details]
Patch
Comment 12 Yusuke Suzuki 2022-02-28 10:01:12 PST
Committed r290603 (?): <https://commits.webkit.org/r290603>