[JSC] Use DeferTerminationForAWhile in Interpreter::unwind
Created attachment 453168 [details] Patch
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 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.
Committed r290516 (247800@trunk): <https://commits.webkit.org/247800@trunk>
<rdar://problem/89482527>
Re-opened since this is blocked by bug 237222
Created attachment 453255 [details] Patch
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/
Created attachment 453278 [details] Patch
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?
Created attachment 453282 [details] Patch
Committed r290603 (?): <https://commits.webkit.org/r290603>