finally blocks should not set the exception stack trace when re-throwing the exception.
https://bugs.webkit.org/show_bug.cgi?id=145525
Reviewed by Geoffrey Garen.
Source/JavaScriptCore:
How exceptions presently work:
=============================
- op_throw can throw any JSValue.
- the VM tries to capture the stack at the throw point and propagate that as needed.
- finally blocks are implemented using op_catch to catch the thrown value, and throws it again using op_throw.
What's wrong with how it presently works:
========================================
- finally's makes for bad exception throw line numbers in the Inspector console.
The op_throw in finally will throw the value anew i.e. it captures a stack from the re-throw point.
As a result, the Inspector sees the finally block as the throw point. The original stack is lost.
- finally's breaks the Inspector's "Breaks on Uncaught Exception"
This is because finally blocks are indistinguishable from catch blocks. As a result, a try-finally,
which should break in the Inspector on the throw, does not because the Inspector thought the
exception was "caught".
- finally's yields confusing break points when the Inspector "Breaks on All Exceptions"
- In a try-finally scenario, the Inspector breaks 2 times: 1 at the throw, 1 at the finally.
- In a for-of loop (which has synthesized finallys), the Inspector will do another break.
Similarly for other cases of JS code which synthesize finallys.
- At VM re-entry boundaries (e.g. js throws & returns to native code, which returns to js),
the Inspector will do another break if there's an uncaught exception.
How this patch fixes the issues:
===============================
- We introduce an Exception object that wraps the thrown value and the exception stack.
When throwing an exception, the VM will check if the thrown value is an Exception
object or not. If it is not an Exception object, then we must be throwing a new
exception. The VM will create an Exception object to wrap the thrown value and
capture the current stack for it.
If the thrown value is already an Exception object, then the requested throw operation
must be a re-throw. The VM will not capture a new stack for it.
- op_catch will now populate 2 locals: 1 for the Exception, 1 for the thrown JSValue.
The VM is aware of the Exception object and uses it for rethrows in finally blocks.
JS source code is never aware of the Exception object.
JS code is aware of the thrown value. If it throws the caught thrown value, that
constitutes a new throw, and a new Exception object will be created for it.
- The VM no longer tracks the thrown JSValue and the exception stack. It will only
track a m_exception field which is an Exception*.
- The BytecodeGenerator has already been updated in a prior patch to distinguish
between Catch, Finally, and SynthesizedFinally blocks. The interpreter runtime will
now report to the debugger whether we have a Catch handler, not just any handlers.
The debugger will use this detail to determine whether to break or not. "Break on
uncaught exceptions" will only break if no Catch handler was found.
This solves the issue of the debugger breaking at finally blocks, and for-of statements.
- The Exception object will also have a flag to indicate whether the debugger has been
notified of the Exception being thrown. Once the Interpreter notifies the debugger
of the Exception object, it will mark this flag and not repeat the notify the debugger
again of the same Exception.
This solves the issue of the debugger breaking at VM re-entry points due to uncaught
exceptions.
- The life-cycle of the captured exception stack trace will now follow the life-cycle
of the Exception object.
Other changes:
- Change all clients of the VM::exception() to expect an Exception* instead of JSValue.
- Fixed a few bugs where thrown exceptions are not cleared before exiting the VM.
- Also renamed some variables and classes to better describe what they are.
(JSEvaluateScript):
(JSCheckScriptSyntax):
(handleExceptionIfNeeded):
- The functions below all do the same exception check. Added this helper
to simplify the code.
(JSClassCreate):
(JSObjectMakeFunction):
(JSObjectMakeArray):
(JSObjectMakeDate):
(JSObjectMakeError):
(JSObjectMakeRegExp):
(JSObjectGetProperty):
(JSObjectSetProperty):
(JSObjectGetPropertyAtIndex):
(JSObjectSetPropertyAtIndex):
(JSObjectDeleteProperty):
(JSObjectCallAsFunction):
(JSObjectCallAsConstructor):
- API/JSScriptRef.cpp:
- API/JSValue.mm:
(JSContainerConvertor::take):
(reportExceptionToInspector):
(handleExceptionIfNeeded):
- The functions below all do the same exception check. Added this helper
to simplify the code.
(evernoteHackNeeded):
(JSValueIsEqual):
(JSValueIsInstanceOfConstructor):
(JSValueCreateJSONString):
(JSValueToNumber):
(JSValueToStringCopy):
(JSValueToObject):
- bindings/ScriptFunctionCall.cpp:
(Deprecated::ScriptFunctionCall::call):
- bindings/ScriptFunctionCall.h:
- bytecode/BytecodeList.json:
- op_catch now had 2 operands: the exception register, and the thrown value register.
- bytecode/BytecodeUseDef.h:
(JSC::computeDefsForBytecodeOffset):
(JSC::CodeBlock::dumpBytecode):
(JSC::CodeBlock::handlerForBytecodeOffset):
- bytecode/CodeBlock.h:
- handlerForBytecodeOffset() now can look for just Catch handlers only.
- bytecode/HandlerInfo.h:
- Cleaned up some white space I accidentally added in a previous patch.
- bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::pushTry):
(JSC::BytecodeGenerator::popTryAndEmitCatch):
(JSC::BytecodeGenerator::emitThrowReferenceError):
(JSC::BytecodeGenerator::emitEnumeration):
- bytecompiler/BytecodeGenerator.h:
(JSC::BytecodeGenerator::emitThrow):
- bytecompiler/NodesCodegen.cpp:
(JSC::TryNode::emitBytecode):
- Adding support for op_catch's 2 operands.
(JSC::Debugger::hasBreakpoint):
(JSC::Debugger::pauseIfNeeded):
(JSC::Debugger::exception):
- debugger/Debugger.h:
- debugger/DebuggerCallFrame.cpp:
(JSC::DebuggerCallFrame::thisValue):
(JSC::DebuggerCallFrame::evaluate):
- debugger/DebuggerCallFrame.h:
(JSC::DebuggerCallFrame::isValid):
- inspector/InjectedScriptManager.cpp:
(Inspector::InjectedScriptManager::createInjectedScript):
- inspector/InspectorEnvironment.h:
- inspector/JSGlobalObjectInspectorController.cpp:
(Inspector::JSGlobalObjectInspectorController::appendAPIBacktrace):
(Inspector::JSGlobalObjectInspectorController::reportAPIException):
- inspector/JSGlobalObjectInspectorController.h:
- inspector/JSGlobalObjectScriptDebugServer.h:
- inspector/JSJavaScriptCallFrame.cpp:
(Inspector::JSJavaScriptCallFrame::evaluate):
- inspector/JavaScriptCallFrame.h:
(Inspector::JavaScriptCallFrame::vmEntryGlobalObject):
(Inspector::JavaScriptCallFrame::thisValue):
(Inspector::JavaScriptCallFrame::evaluate):
- inspector/ScriptCallStackFactory.cpp:
(Inspector::extractSourceInformationFromException):
(Inspector::createScriptCallStackFromException):
- inspector/ScriptCallStackFactory.h:
- inspector/ScriptDebugServer.cpp:
(Inspector::ScriptDebugServer::evaluateBreakpointAction):
(Inspector::ScriptDebugServer::handleBreakpointHit):
(Inspector::ScriptDebugServer::handleExceptionInBreakpointCondition):
- inspector/ScriptDebugServer.h:
- interpreter/CallFrame.h:
(JSC::ExecState::clearException):
(JSC::ExecState::exception):
(JSC::ExecState::hadException):
(JSC::ExecState::atomicStringTable):
(JSC::ExecState::propertyNames):
(JSC::ExecState::clearSupplementaryExceptionInfo): Deleted.
- interpreter/Interpreter.cpp:
(JSC::unwindCallFrame):
(JSC::Interpreter::stackTraceAsString):
(JSC::GetCatchHandlerFunctor::GetCatchHandlerFunctor):
(JSC::GetCatchHandlerFunctor::operator()):
(JSC::Interpreter::unwind):
- Added a check for didNotifyInspectorOfThrow() here to prevent duplicate reports
of the same Exception to the debugger.
(JSC::GetExceptionHandlerFunctor::GetExceptionHandlerFunctor): Deleted.
(JSC::GetExceptionHandlerFunctor::operator()): Deleted.
- Renamed GetExceptionHandlerFunctor to GetCatchHandlerFunctor since the debugger
is only interested in knowing whether we have Catch handlers.
- interpreter/Interpreter.h:
(JSC::SuspendExceptionScope::SuspendExceptionScope):
(JSC::SuspendExceptionScope::~SuspendExceptionScope):
(JSC::Interpreter::sampler):
(JSC::ClearExceptionScope::ClearExceptionScope): Deleted.
(JSC::ClearExceptionScope::~ClearExceptionScope): Deleted.
- Renamed ClearExceptionScope to SuspendExceptionScope because "clear" implies that
we're purging the exception. Instead, we're merely suspending any handling of
that exception for a period defined by the scope.
(JSC::AssemblyHelpers::emitExceptionCheck):
(JSC::genericUnwind):
- Removed the exception argument. It is always the value in VM::exception() anyway.
genericUnwind() can just get it from the VM, and save everyone some work.
- jit/JITExceptions.h:
- jit/JITOpcodes.cpp:
(JSC::JIT::emit_op_catch):
(JSC::JIT::privateCompileCTINativeCall):
(JSC::JIT::emit_op_catch):
- Add support for the new op_catch operands.
- jit/JITOperations.cpp:
- jit/ThunkGenerators.cpp:
(JSC::nativeForGenerator):
(functionRun):
(functionLoad):
(runWithScripts):
(runInteractive):
- llint/LLIntOffsetsExtractor.cpp:
- llint/LLIntSlowPaths.cpp:
(JSC::LLInt::LLINT_SLOW_PATH_DECL):
- llint/LowLevelInterpreter32_64.asm:
- llint/LowLevelInterpreter64.asm:
- Add support for the new op_catch operands. Also update the code to handle
VM::m_exception being an Exception pointer, not a JSValue.
- parser/NodeConstructors.h:
(JSC::TryNode::TryNode):
- parser/Nodes.h:
- runtime/CallData.cpp:
(JSC::call):
(JSC::evaluate):
(JSC::evaluate):
- Change evaluate() to take a reference to the returned exception value instead
of a pointer. In all but 2 or 3 cases, we want the returned exception anyway.
Might as well simplify the code by requiring the reference.
(JSC::throwVMError):
(JSC::throwVMTypeError):
- runtime/Exception.cpp: Added.
(JSC::Exception::create):
(JSC::Exception::destroy):
(JSC::Exception::createStructure):
(JSC::Exception::visitChildren):
(JSC::Exception::Exception):
(JSC::Exception::~Exception):
- runtime/Exception.h: Added.
(JSC::Exception::valueOffset):
(JSC::Exception::cast):
(JSC::Exception::value):
(JSC::Exception::stack):
(JSC::Exception::didNotifyInspectorOfThrow):
(JSC::Exception::setDidNotifyInspectorOfThrow):
- runtime/ExceptionHelpers.cpp:
(JSC::createTerminatedExecutionException):
(JSC::isTerminatedExecutionException):
(JSC::createStackOverflowError):
- runtime/ExceptionHelpers.h:
- runtime/GetterSetter.cpp:
(JSC::callGetter):
- runtime/IteratorOperations.cpp:
(JSC::iteratorClose):
- runtime/JSObject.cpp:
- runtime/JSPromiseConstructor.cpp:
(JSC::constructPromise):
- runtime/JSPromiseDeferred.cpp:
(JSC::updateDeferredFromPotentialThenable):
(JSC::abruptRejection):
- runtime/JSPromiseReaction.cpp:
(JSC::ExecutePromiseReactionMicrotask::run):
(JSC::VM::VM):
(JSC::VM::releaseExecutableMemory):
(JSC::VM::throwException):
(JSC::VM::setStackPointerAtVMEntry):
(JSC::VM::getExceptionInfo): Deleted.
(JSC::VM::setExceptionInfo): Deleted.
(JSC::VM::clearException): Deleted.
(JSC::clearExceptionStack): Deleted.
(JSC::VM::targetMachinePCForThrowOffset):
(JSC::VM::clearException):
(JSC::VM::setException):
(JSC::VM::exception):
(JSC::VM::addressOfException):
(JSC::VM::exceptionStack): Deleted.
- runtime/VMEntryScope.cpp:
(JSC::VMEntryScope::VMEntryScope):
(JSC::VMEntryScope::setEntryScopeDidPopListener):
Source/WebCore:
Update to use the new JSC::Exception object.
Test: inspector/debugger/break-on-exceptions.html
- ForwardingHeaders/runtime/Exception.h: Added.
- bindings/js/JSCallbackData.cpp:
(WebCore::JSCallbackData::invokeCallback):
- bindings/js/JSCustomXPathNSResolver.cpp:
(WebCore::JSCustomXPathNSResolver::lookupNamespaceURI):
- bindings/js/JSDOMBinding.cpp:
(WebCore::jsArray):
(WebCore::reportException):
(WebCore::reportCurrentException):
- bindings/js/JSDOMBinding.h:
- bindings/js/JSErrorHandler.cpp:
(WebCore::JSErrorHandler::handleEvent):
- bindings/js/JSEventListener.cpp:
(WebCore::JSEventListener::handleEvent):
- bindings/js/JSMainThreadExecState.cpp:
(WebCore::JSMainThreadExecState::didLeaveScriptContext):
(WebCore::functionCallHandlerFromAnyThread):
(WebCore::evaluateHandlerFromAnyThread):
- bindings/js/JSMainThreadExecState.h:
(WebCore::JSMainThreadExecState::currentState):
(WebCore::JSMainThreadExecState::call):
(WebCore::JSMainThreadExecState::evaluate):
(WebCore::JSMainThreadExecState::runTask):
- bindings/js/JSMediaDevicesCustom.cpp:
(WebCore::JSMediaDevices::getUserMedia):
- Fixed a bug where the exception was not cleared before entering the VM to
call JS code.
- bindings/js/JSMutationCallback.cpp:
(WebCore::JSMutationCallback::call):
- bindings/js/ReadableJSStream.cpp:
(WebCore::getPropertyFromObject):
(WebCore::callFunction):
(WebCore::ReadableJSStream::Source::start):
- bindings/js/ScheduledAction.cpp:
(WebCore::ScheduledAction::executeFunctionInContext):
- bindings/js/ScriptController.cpp:
(WebCore::ScriptController::evaluateInWorld):
- bindings/js/SerializedScriptValue.cpp:
(WebCore::SerializedScriptValue::create):
(WebCore::SerializedScriptValue::deserialize):
- bindings/js/WorkerScriptController.cpp:
(WebCore::WorkerScriptController::evaluate):
(WebCore::WorkerScriptController::setException):
(WebCore::WorkerScriptController::scheduleExecutionTermination):
- bindings/js/WorkerScriptController.h:
(WebCore::WorkerScriptController::workerGlobalScopeWrapper):
- bindings/js/WorkerScriptDebugServer.cpp:
(WebCore::WorkerScriptDebugServer::runEventLoopWhilePaused):
(WebCore::WorkerScriptDebugServer::reportException):
- bindings/js/WorkerScriptDebugServer.h:
- bindings/objc/WebScriptObject.mm:
(WebCore::createJSWrapper):
(WebCore::addExceptionToConsole):
(-[WebScriptObject callWebScriptMethod:withArguments:]):
(-[WebScriptObject evaluateWebScript:]):
- Changed to call a version of JSMainThreadExecState::evaluate() that provides
a stub returnedException because evaluateWebScript: doesn't need the exception.
- inspector/PageScriptDebugServer.cpp:
(WebCore::PageScriptDebugServer::isContentScript):
(WebCore::PageScriptDebugServer::reportException):
- inspector/PageScriptDebugServer.h:
- workers/WorkerGlobalScope.cpp:
(WebCore::WorkerGlobalScope::importScripts):
Source/WebKit/mac:
(+[WebView _reportException:inContext:]):
(WebKitInitializeApplicationCachePathIfNecessary):
- Changed to use the new Exception object.
Source/WebKit/win:
(WebView::reportException):
- Changed to use the new Exception object.
Source/WebKit2:
- WebProcess/InjectedBundle/InjectedBundle.cpp:
(WebKit::InjectedBundle::reportException):
- Changed to use the new Exception object.
LayoutTests:
- fast/dom/regress-131530-expected.txt:
- Rebased results because we now have a proper line number.
- http/tests/inspector/inspector-test.js:
(InspectorTestProxy.clearResults):
(InspectorTestProxy.reportUncaughtException):
- Add the feature to sanitize the url reported by reportUncaughtException() since
we can have tests that do expect uncaught exceptions, and we need the test
results to be invariant. Sanitization of the url, in this case means, stripping
off the preceding path.
- inspector/debugger/break-on-exception-expected.txt: Added.
- inspector/debugger/break-on-exception.html: Added.
- inspector/debugger/break-on-exception-catch-expected.txt: Added.
- inspector/debugger/break-on-exception-catch.html: Added.
- inspector/debugger/break-on-exception-finally-expected.txt: Added.
- inspector/debugger/break-on-exception-finally.html: Added.
- inspector/debugger/break-on-exception-native-expected.txt: Added.
- inspector/debugger/break-on-exception-native.html: Added.
- inspector/debugger/break-on-exception-throw-in-promise-expected.txt: Added.
- inspector/debugger/break-on-exception-throw-in-promise.html: Added.
- inspector/debugger/break-on-exception-throw-in-promise-with-catch-expected.txt: Added.
- inspector/debugger/break-on-exception-throw-in-promise-with-catch.html: Added.
- inspector/debugger/break-on-exception-throw-in-promise-then-expected.txt: Added.
- inspector/debugger/break-on-exception-throw-in-promise-then.html: Added.
- inspector/debugger/break-on-exception-throw-in-promise-then-with-catch-expected.txt: Added.
- inspector/debugger/break-on-exception-throw-in-promise-then-with-catch.html: Added.
- inspector/debugger/break-on-exception-throw-in-promise-rethrow-in-catch-expected.txt: Added.
- inspector/debugger/break-on-exception-throw-in-promise-rethrow-in-catch.html: Added.
- inspector/debugger/break-on-exception-window-onerror-expected.txt: Added.
- inspector/debugger/break-on-exception-window-onerror.html: Added.
- inspector/debugger/break-on-uncaught-exception-expected.txt: Added.
- inspector/debugger/break-on-uncaught-exception.html: Added.
- inspector/debugger/break-on-uncaught-exception-catch-expected.txt: Added.
- inspector/debugger/break-on-uncaught-exception-catch.html: Added.
- inspector/debugger/break-on-uncaught-exception-finally-expected.txt: Added.
- inspector/debugger/break-on-uncaught-exception-finally.html: Added.
- inspector/debugger/break-on-uncaught-exception-native-expected.txt: Added.
- inspector/debugger/break-on-uncaught-exception-native.html: Added.
- inspector/debugger/break-on-uncaught-exception-throw-in-promise-expected.txt: Added.
- inspector/debugger/break-on-uncaught-exception-throw-in-promise.html: Added.
- inspector/debugger/break-on-uncaught-exception-throw-in-promise-with-catch-expected.txt: Added.
- inspector/debugger/break-on-uncaught-exception-throw-in-promise-with-catch.html: Added.
- inspector/debugger/break-on-uncaught-exception-throw-in-promise-then-expected.txt: Added.
- inspector/debugger/break-on-uncaught-exception-throw-in-promise-then.html: Added.
- inspector/debugger/break-on-uncaught-exception-throw-in-promise-then-with-catch-expected.txt: Added.
- inspector/debugger/break-on-uncaught-exception-throw-in-promise-then-with-catch.html: Added.
- inspector/debugger/break-on-uncaught-exception-throw-in-promise-rethrow-in-catch-expected.txt: Added.
- inspector/debugger/break-on-uncaught-exception-throw-in-promise-rethrow-in-catch.html: Added.
- inspector/debugger/break-on-uncaught-exception-window-onerror-expected.txt: Added.
- inspector/debugger/break-on-uncaught-exception-window-onerror.html: Added.
- inspector/debugger/resources/break-on-exception-tests.js: Added.
(doThrow):
(testCatch):
(testFinally):
(testThrowingThruNativeCode):
(testThrowingInPromise):
(testThrowingInPromiseWithCatch):
(testThrowingInPromiseThen):
(testThrowingInPromiseThenWithCatch):
(testThrowingInPromiseWithRethrowInCatch):
