From b28a80964fa6e7b5ed9ebc55b6e4e69900d59ce1 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 15 Jun 2019 08:07:15 +0800 Subject: [PATCH 1/2] src: refactor uncaught exception handling The C++ land `node::FatalException()` is not in fact fatal anymore. It gives the user a chance to handle the uncaught exception globally by listening to the `uncaughtException` event. This patch renames it to `TriggerUncaughtException` in C++ to avoid the confusion. In addition rename the JS land handler to `onGlobalUncaughtException` to reflect its purpose - we have to keep the alias `process._fatalException` and use that for now since it has been monkey-patchable in the user land. This patch also - Adds more comments to the global uncaught exception handling routine - Puts a few other C++ error handling functions into the `errors` namespace - Moves error-handling-related bindings to the `errors` binding. Refs: https://github.com/nodejs/node/commit/2b252acea47af3ebeac3d7e68277f015667264cc --- lib/internal/bootstrap/node.js | 10 ++- lib/internal/main/worker_thread.js | 23 ++--- lib/internal/modules/cjs/loader.js | 2 +- lib/internal/process/execution.js | 14 +-- lib/internal/process/promises.js | 48 +++++------ lib/internal/process/task_queues.js | 15 ++-- src/api/exceptions.cc | 15 ++-- src/env.cc | 2 +- src/js_stream.cc | 10 +-- src/node.cc | 8 +- src/node_api.cc | 2 +- src/node_contextify.cc | 6 +- src/node_errors.cc | 128 ++++++++++++++++++---------- src/node_errors.h | 29 +++---- src/node_task_queue.cc | 15 ---- src/node_util.cc | 9 -- 16 files changed, 177 insertions(+), 159 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 8dee38a0e71698..22dabf4d7ed459 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -254,12 +254,18 @@ Object.defineProperty(process, 'features', { { const { - fatalException, + onGlobalUncaughtException, setUncaughtExceptionCaptureCallback, hasUncaughtExceptionCaptureCallback } = require('internal/process/execution'); - process._fatalException = fatalException; + // For legacy reasons this is still called `_fatalException`, even + // though it is now a global uncaught exception handler. + // The C++ land node::errors::TriggerUncaughtException grabs it + // from the process object because it has been monkey-patchable. + // TODO(joyeecheung): investigate whether process._fatalException + // can be deprecated. + process._fatalException = onGlobalUncaughtException; process.setUncaughtExceptionCaptureCallback = setUncaughtExceptionCaptureCallback; process.hasUncaughtExceptionCaptureCallback = diff --git a/lib/internal/main/worker_thread.js b/lib/internal/main/worker_thread.js index 932f282a1bae2c..3a5ac9cca4a47f 100644 --- a/lib/internal/main/worker_thread.js +++ b/lib/internal/main/worker_thread.js @@ -42,7 +42,7 @@ const { } = workerIo; const { - fatalException: originalFatalException + onGlobalUncaughtException } = require('internal/process/execution'); const publicWorker = require('worker_threads'); @@ -156,24 +156,23 @@ port.on('message', (message) => { } }); -// Overwrite fatalException -process._fatalException = (error, fromPromise) => { - debug(`[${threadId}] gets fatal exception`); - let caught = false; +function workerOnGlobalUncaughtException(error, fromPromise) { + debug(`[${threadId}] gets uncaught exception`); + let handled = false; try { - caught = originalFatalException.call(this, error, fromPromise); + handled = onGlobalUncaughtException(error, fromPromise); } catch (e) { error = e; } - debug(`[${threadId}] fatal exception caught = ${caught}`); + debug(`[${threadId}] uncaught exception handled = ${handled}`); - if (!caught) { + if (!handled) { let serialized; try { const { serializeError } = require('internal/error-serdes'); serialized = serializeError(error); } catch {} - debug(`[${threadId}] fatal exception serialized = ${!!serialized}`); + debug(`[${threadId}] uncaught exception serialized = ${!!serialized}`); if (serialized) port.postMessage({ type: ERROR_MESSAGE, @@ -187,7 +186,11 @@ process._fatalException = (error, fromPromise) => { process.exit(); } -}; +} + +// Patch the global uncaught exception handler so it gets picked up by +// node::errors::TriggerUncaughtException(). +process._fatalException = workerOnGlobalUncaughtException; markBootstrapComplete(); diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 0e4f9b103c394e..639877434e2e98 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -832,7 +832,7 @@ Module.runMain = function() { return loader.import(pathToFileURL(process.argv[1]).pathname); }) .catch((e) => { - internalBinding('task_queue').triggerFatalException( + internalBinding('errors').triggerUncaughtException( e, true /* fromPromise */ ); diff --git a/lib/internal/process/execution.js b/lib/internal/process/execution.js index 077d225556ca81..962bf0c8099210 100644 --- a/lib/internal/process/execution.js +++ b/lib/internal/process/execution.js @@ -115,10 +115,14 @@ function noop() {} // and exported to be written to process._fatalException, it has to be // returned as an *anonymous function* wrapped inside a factory function, // otherwise it breaks the test-timers.setInterval async hooks test - -// this may indicate that node::FatalException should fix up the callback scope -// before calling into process._fatalException, or this function should -// take extra care of the async hooks before it schedules a setImmediate. -function createFatalException() { +// this may indicate that node::errors::TriggerUncaughtException() should +// fix up the callback scope before calling into process._fatalException, +// or this function should take extra care of the async hooks before it +// schedules a setImmediate. +function createOnGlobalUncaughtException() { + // The C++ land node::errors::TriggerUncaughtException() will + // exit the process if it returns false, and continue execution if it + // returns true (which indicates that the exception is handled by the user). return (er, fromPromise) => { // It's possible that defaultTriggerAsyncId was set for a constructor // call that threw and was never cleared. So clear it now. @@ -206,7 +210,7 @@ module.exports = { tryGetCwd, evalModule, evalScript, - fatalException: createFatalException(), + onGlobalUncaughtException: createOnGlobalUncaughtException(), setUncaughtExceptionCaptureCallback, hasUncaughtExceptionCaptureCallback }; diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js index 031997fc1b3467..c68d7bb35490e1 100644 --- a/lib/internal/process/promises.js +++ b/lib/internal/process/promises.js @@ -2,9 +2,6 @@ const { Object } = primordials; -const { - safeToString -} = internalBinding('util'); const { tickInfo, promiseRejectEvents: { @@ -13,10 +10,14 @@ const { kPromiseResolveAfterResolved, kPromiseRejectAfterResolved }, - setPromiseRejectCallback, - triggerFatalException + setPromiseRejectCallback } = internalBinding('task_queue'); +const { + noSideEffectsToString, + triggerUncaughtException +} = internalBinding('errors'); + // *Must* match Environment::TickInfo::Fields in src/env.h. const kHasRejectionToWarn = 1; @@ -127,7 +128,7 @@ function handledRejection(promise) { const unhandledRejectionErrName = 'UnhandledPromiseRejectionWarning'; function emitWarning(uid, reason) { - const warning = getError( + const warning = getErrorWithoutStack( unhandledRejectionErrName, 'Unhandled promise rejection. This error originated either by ' + 'throwing inside of an async function without a catch block, ' + @@ -139,7 +140,8 @@ function emitWarning(uid, reason) { warning.stack = reason.stack; process.emitWarning(reason.stack, unhandledRejectionErrName); } else { - process.emitWarning(safeToString(reason), unhandledRejectionErrName); + process.emitWarning( + noSideEffectsToString(reason), unhandledRejectionErrName); } } catch {} @@ -184,7 +186,9 @@ function processPromiseRejections() { const { reason, uid } = promiseInfo; switch (unhandledRejectionsMode) { case kThrowUnhandledRejections: { - fatalException(reason); + const err = reason instanceof Error ? + reason : generateUnhandledRejectionError(reason); + triggerUncaughtException(err, true /* fromPromise */); const handled = process.emit('unhandledRejection', reason, promise); if (!handled) emitWarning(uid, reason); break; @@ -210,7 +214,7 @@ function processPromiseRejections() { pendingUnhandledRejections.length !== 0; } -function getError(name, message) { +function getErrorWithoutStack(name, message) { // Reset the stack to prevent any overhead. const tmp = Error.stackTraceLimit; Error.stackTraceLimit = 0; @@ -226,21 +230,17 @@ function getError(name, message) { return err; } -function fatalException(reason) { - let err; - if (reason instanceof Error) { - err = reason; - } else { - err = getError( - 'UnhandledPromiseRejection', - 'This error originated either by ' + - 'throwing inside of an async function without a catch block, ' + - 'or by rejecting a promise which was not handled with .catch().' + - ` The promise rejected with the reason "${safeToString(reason)}".` - ); - err.code = 'ERR_UNHANDLED_REJECTION'; - } - triggerFatalException(err, true /* fromPromise */); +function generateUnhandledRejectionError(reason) { + const message = + 'This error originated either by ' + + 'throwing inside of an async function without a catch block, ' + + 'or by rejecting a promise which was not handled with .catch().' + + ' The promise rejected with the reason ' + + `"${noSideEffectsToString(reason)}".`; + + const err = getErrorWithoutStack('UnhandledPromiseRejection', message); + err.code = 'ERR_UNHANDLED_REJECTION'; + return err; } function listenForRejections() { diff --git a/lib/internal/process/task_queues.js b/lib/internal/process/task_queues.js index 34e925aaf57d08..68b5d8b343d206 100644 --- a/lib/internal/process/task_queues.js +++ b/lib/internal/process/task_queues.js @@ -9,10 +9,13 @@ const { // Used to run V8's micro task queue. runMicrotasks, setTickCallback, - enqueueMicrotask, - triggerFatalException + enqueueMicrotask } = internalBinding('task_queue'); +const { + triggerUncaughtException +} = internalBinding('errors'); + const { setHasRejectionToWarn, hasRejectionToWarn, @@ -143,11 +146,9 @@ function runMicrotask() { try { callback(); } catch (error) { - // TODO(devsnek): Remove this if - // https://bugs.chromium.org/p/v8/issues/detail?id=8326 - // is resolved such that V8 triggers the fatal exception - // handler for microtasks. - triggerFatalException(error, false /* fromPromise */); + // runInAsyncScope() swallows the error so we need to catch + // it and handle it here. + triggerUncaughtException(error, false /* fromPromise */); } finally { this.emitDestroy(); } diff --git a/src/api/exceptions.cc b/src/api/exceptions.cc index ee5ea984eb23c5..1632574c6b1b9c 100644 --- a/src/api/exceptions.cc +++ b/src/api/exceptions.cc @@ -244,17 +244,12 @@ Local WinapiErrnoException(Isolate* isolate, } #endif +// Implement the legacy name exposed in node.h. This has not been in fact +// fatal any more, as the user can handle the exception in the +// TryCatch by listening to `uncaughtException`. +// TODO(joyeecheung): deprecate it in favor of a more accurate name. void FatalException(Isolate* isolate, const v8::TryCatch& try_catch) { - // If we try to print out a termination exception, we'd just get 'null', - // so just crashing here with that information seems like a better idea, - // and in particular it seems like we should handle terminations at the call - // site for this function rather than by printing them out somewhere. - CHECK(!try_catch.HasTerminated()); - - HandleScope scope(isolate); - if (!try_catch.IsVerbose()) { - FatalException(isolate, try_catch.Exception(), try_catch.Message()); - } + errors::TriggerUncaughtException(isolate, try_catch); } } // namespace node diff --git a/src/env.cc b/src/env.cc index f0f4fc30688aad..f9e8c8c27e5e95 100644 --- a/src/env.cc +++ b/src/env.cc @@ -639,7 +639,7 @@ void Environment::RunAndClearNativeImmediates() { ref_count++; if (UNLIKELY(try_catch.HasCaught())) { if (!try_catch.HasTerminated()) - FatalException(isolate(), try_catch); + errors::TriggerUncaughtException(isolate(), try_catch); // We are done with the current callback. Increase the counter so that // the steps below make everything *after* the current item part of diff --git a/src/js_stream.cc b/src/js_stream.cc index 2663106ba7af51..23bdb9b4892512 100644 --- a/src/js_stream.cc +++ b/src/js_stream.cc @@ -49,7 +49,7 @@ bool JSStream::IsClosing() { Local value; if (!MakeCallback(env()->isclosing_string(), 0, nullptr).ToLocal(&value)) { if (try_catch.HasCaught() && !try_catch.HasTerminated()) - FatalException(env()->isolate(), try_catch); + errors::TriggerUncaughtException(env()->isolate(), try_catch); return true; } return value->IsTrue(); @@ -65,7 +65,7 @@ int JSStream::ReadStart() { if (!MakeCallback(env()->onreadstart_string(), 0, nullptr).ToLocal(&value) || !value->Int32Value(env()->context()).To(&value_int)) { if (try_catch.HasCaught() && !try_catch.HasTerminated()) - FatalException(env()->isolate(), try_catch); + errors::TriggerUncaughtException(env()->isolate(), try_catch); } return value_int; } @@ -80,7 +80,7 @@ int JSStream::ReadStop() { if (!MakeCallback(env()->onreadstop_string(), 0, nullptr).ToLocal(&value) || !value->Int32Value(env()->context()).To(&value_int)) { if (try_catch.HasCaught() && !try_catch.HasTerminated()) - FatalException(env()->isolate(), try_catch); + errors::TriggerUncaughtException(env()->isolate(), try_catch); } return value_int; } @@ -102,7 +102,7 @@ int JSStream::DoShutdown(ShutdownWrap* req_wrap) { argv).ToLocal(&value) || !value->Int32Value(env()->context()).To(&value_int)) { if (try_catch.HasCaught() && !try_catch.HasTerminated()) - FatalException(env()->isolate(), try_catch); + errors::TriggerUncaughtException(env()->isolate(), try_catch); } return value_int; } @@ -137,7 +137,7 @@ int JSStream::DoWrite(WriteWrap* w, argv).ToLocal(&value) || !value->Int32Value(env()->context()).To(&value_int)) { if (try_catch.HasCaught() && !try_catch.HasTerminated()) - FatalException(env()->isolate(), try_catch); + errors::TriggerUncaughtException(env()->isolate(), try_catch); } return value_int; } diff --git a/src/node.cc b/src/node.cc index 99e6f74b472fdc..9b4328d2ec8490 100644 --- a/src/node.cc +++ b/src/node.cc @@ -132,7 +132,6 @@ using options_parser::kDisallowedInEnvironment; using v8::Boolean; using v8::EscapableHandleScope; -using v8::Exception; using v8::Function; using v8::FunctionCallbackInfo; using v8::HandleScope; @@ -218,10 +217,9 @@ MaybeLocal ExecuteBootstrapper(Environment* env, arguments->size(), arguments->data()); - // If there was an error during bootstrap then it was either handled by the - // FatalException handler or it's unrecoverable (e.g. max call stack - // exceeded). Either way, clear the stack so that the AsyncCallbackScope - // destructor doesn't fail on the id check. + // If there was an error during bootstrap, it must be unrecoverable + // (e.g. max call stack exceeded). Clear the stack so that the + // AsyncCallbackScope destructor doesn't fail on the id check. // There are only two ways to have a stack size > 1: 1) the user manually // called MakeCallback or 2) user awaited during bootstrap, which triggered // _tickCallback(). diff --git a/src/node_api.cc b/src/node_api.cc index c4d86342868e7f..3aab7a225777c6 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -115,7 +115,7 @@ static inline void trigger_fatal_exception( napi_env env, v8::Local local_err) { v8::Local local_msg = v8::Exception::CreateMessage(env->isolate, local_err); - node::FatalException(env->isolate, local_err, local_msg); + node::errors::TriggerUncaughtException(env->isolate, local_err, local_msg); } class ThreadSafeFunction : public node::AsyncResource { diff --git a/src/node_contextify.cc b/src/node_contextify.cc index ee5a9db574f0f9..70c63a0d8a3ec1 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -734,7 +734,7 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { compile_options); if (v8_script.IsEmpty()) { - DecorateErrorStack(env, try_catch); + errors::DecorateErrorStack(env, try_catch); no_abort_scope.Close(); if (!try_catch.HasTerminated()) try_catch.ReThrow(); @@ -946,7 +946,7 @@ bool ContextifyScript::EvalMachine(Environment* env, if (try_catch.HasCaught()) { if (!timed_out && !received_signal && display_errors) { // We should decorate non-termination exceptions - DecorateErrorStack(env, try_catch); + errors::DecorateErrorStack(env, try_catch); } // If there was an exception thrown during script execution, re-throw it. @@ -1110,7 +1110,7 @@ void ContextifyContext::CompileFunction( if (maybe_fn.IsEmpty()) { if (try_catch.HasCaught() && !try_catch.HasTerminated()) { - DecorateErrorStack(env, try_catch); + errors::DecorateErrorStack(env, try_catch); try_catch.ReThrow(); } return; diff --git a/src/node_errors.cc b/src/node_errors.cc index 50469b09a8a151..3ac26608e7479c 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -343,10 +343,6 @@ void ReportException(Environment* env, #endif } -void ReportException(Environment* env, const v8::TryCatch& try_catch) { - ReportException(env, try_catch.Exception(), try_catch.Message()); -} - void PrintErrorString(const char* format, ...) { va_list ap; va_start(ap, format); @@ -764,7 +760,7 @@ void PerIsolateMessageListener(Local message, Local error) { break; } case Isolate::MessageErrorLevel::kMessageError: - FatalException(isolate, error, message); + TriggerUncaughtException(isolate, error, message); break; } } @@ -775,6 +771,27 @@ void SetPrepareStackTraceCallback(const FunctionCallbackInfo& args) { env->set_prepare_stack_trace_callback(args[0].As()); } +// Side effect-free stringification that will never throw exceptions. +static void NoSideEffectsToString(const FunctionCallbackInfo& args) { + Local context = args.GetIsolate()->GetCurrentContext(); + Local detail_string; + if (args[0]->ToDetailString(context).ToLocal(&detail_string)) + args.GetReturnValue().Set(detail_string); +} + +static void TriggerUncaughtException(const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + Environment* env = Environment::GetCurrent(isolate); + Local exception = args[0]; + Local message = Exception::CreateMessage(isolate, exception); + if (env != nullptr && env->abort_on_uncaught_exception()) { + ReportException(env, exception, message); + Abort(); + } + bool from_promise = args[1]->IsTrue(); + errors::TriggerUncaughtException(isolate, exception, message, from_promise); +} + void Initialize(Local target, Local unused, Local context, @@ -782,10 +799,11 @@ void Initialize(Local target, Environment* env = Environment::GetCurrent(context); env->SetMethod( target, "setPrepareStackTraceCallback", SetPrepareStackTraceCallback); + env->SetMethodNoSideEffect( + target, "noSideEffectsToString", NoSideEffectsToString); + env->SetMethod(target, "triggerUncaughtException", TriggerUncaughtException); } -} // namespace errors - void DecorateErrorStack(Environment* env, const errors::TryCatchScope& try_catch) { Local exception = try_catch.Exception(); @@ -822,10 +840,10 @@ void DecorateErrorStack(Environment* env, env->context(), env->decorated_private_symbol(), True(env->isolate())); } -void FatalException(Isolate* isolate, - Local error, - Local message, - bool from_promise) { +void TriggerUncaughtException(Isolate* isolate, + Local error, + Local message, + bool from_promise) { CHECK(!error.IsEmpty()); HandleScope scope(isolate); @@ -843,59 +861,81 @@ void FatalException(Isolate* isolate, Abort(); } + // Invoke process._fatalException() to give user a chance to handle it. + // We have to grab it from the process object since this has been + // monkey-patchable. Local process_object = env->process_object(); Local fatal_exception_string = env->fatal_exception_string(); Local fatal_exception_function = process_object->Get(env->context(), fatal_exception_string).ToLocalChecked(); - + // If the exception happens before process._fatalException is attached + // during bootstrap, or if the user has patched it incorrectly, just crash. if (!fatal_exception_function->IsFunction()) { - // Failed before the process._fatalException function was added! - // this is probably pretty bad. Nothing to do but report and exit. ReportException(env, error, message); env->Exit(6); - } else { - errors::TryCatchScope fatal_try_catch(env); - - // Do not call FatalException when _fatalException handler throws - fatal_try_catch.SetVerbose(false); + return; + } + MaybeLocal handled; + { + // We do not expect the global uncaught exception itself to throw any more + // exceptions. If it does, crash. + errors::TryCatchScope fatal_try_catch( + env, errors::TryCatchScope::CatchMode::kFatal); Local argv[2] = { error, Boolean::New(env->isolate(), from_promise) }; - // This will return true if the JS layer handled it, false otherwise - MaybeLocal caught = fatal_exception_function.As()->Call( + handled = fatal_exception_function.As()->Call( env->context(), process_object, arraysize(argv), argv); + } - if (fatal_try_catch.HasTerminated()) return; - - if (fatal_try_catch.HasCaught()) { - // The fatal exception function threw, so we must exit - ReportException(env, fatal_try_catch); - env->Exit(7); - - } else if (caught.ToLocalChecked()->IsFalse()) { - ReportException(env, error, message); + // The global uncaught exception handler returns true if the user handles it + // by e.g. listening to `uncaughtException`. In that case, continue program + // execution. + // TODO(joyeecheung): This has been only checking that the return value is + // exactly false. Investigate whether this can be turned to an "if true" + // similar to how the worker global uncaught exception handler handles it. + if (!handled.ToLocalChecked()->IsFalse()) { + return; + } - // fatal_exception_function call before may have set a new exit code -> - // read it again, otherwise use default for uncaughtException 1 - Local exit_code = env->exit_code_string(); - Local code; - if (!process_object->Get(env->context(), exit_code).ToLocal(&code) || - !code->IsInt32()) { - env->Exit(1); - } - env->Exit(code.As()->Value()); - } + ReportException(env, error, message); + // If the global uncaught exception handler sets process.exitCode, + // exit with that code. Otherwise, exit with 1. + Local exit_code = env->exit_code_string(); + Local code; + if (process_object->Get(env->context(), exit_code).ToLocal(&code) && + code->IsInt32()) { + env->Exit(code.As()->Value()); + } else { + env->Exit(1); } } -void FatalException(Isolate* isolate, - Local error, - Local message) { - FatalException(isolate, error, message, false /* from_promise */); +void TriggerUncaughtException(Isolate* isolate, const v8::TryCatch& try_catch) { + // If the try_catch is verbose, the per-isolate message listener is going to + // handle it (which is going to call into another overload of + // TriggerUncaughtException()). + if (try_catch.IsVerbose()) { + return; + } + + // If the user calls TryCatch::TerminateExecution() on this TryCatch + // they must call CancelTerminateExecution() again before invoking + // TriggerUncaughtException() because it will invoke + // process._fatalException() in the JS land. + CHECK(!try_catch.HasTerminated()); + CHECK(try_catch.HasCaught()); + HandleScope scope(isolate); + TriggerUncaughtException(isolate, + try_catch.Exception(), + try_catch.Message(), + false /* from_promise */); } +} // namespace errors + } // namespace node NODE_MODULE_CONTEXT_AWARE_INTERNAL(errors, node::errors::Initialize) diff --git a/src/node_errors.h b/src/node_errors.h index 689911f9963a55..939f93a4899f59 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -29,21 +29,6 @@ void OnFatalError(const char* location, const char* message); void PrintErrorString(const char* format, ...); -void ReportException(Environment* env, const v8::TryCatch& try_catch); - -void ReportException(Environment* env, - Local er, - Local message); - -void FatalException(v8::Isolate* isolate, - Local error, - Local message); - -void FatalException(v8::Isolate* isolate, - Local error, - Local message, - bool from_promise); - // Helpers to construct errors similar to the ones provided by // lib/internal/errors.js. // Example: with `V(ERR_INVALID_ARG_TYPE, TypeError)`, there will be @@ -190,14 +175,24 @@ class TryCatchScope : public v8::TryCatch { CatchMode mode_; }; +// Trigger the global uncaught exception handler `process._fatalException` +// in JS land (which emits the 'uncaughtException' event). If that returns +// true, continue program execution, otherwise exit the process. +void TriggerUncaughtException(v8::Isolate* isolate, + const v8::TryCatch& try_catch); +void TriggerUncaughtException(v8::Isolate* isolate, + Local error, + Local message, + bool from_promise = false); + const char* errno_string(int errorno); void PerIsolateMessageListener(v8::Local message, v8::Local error); -} // namespace errors - void DecorateErrorStack(Environment* env, const errors::TryCatchScope& try_catch); +} // namespace errors + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/node_task_queue.cc b/src/node_task_queue.cc index a277b8bd2abb92..e6b4d0b8e211cd 100644 --- a/src/node_task_queue.cc +++ b/src/node_task_queue.cc @@ -12,7 +12,6 @@ namespace node { using v8::Array; using v8::Context; -using v8::Exception; using v8::Function; using v8::FunctionCallbackInfo; using v8::Isolate; @@ -123,19 +122,6 @@ static void SetPromiseRejectCallback( env->set_promise_reject_callback(args[0].As()); } -static void TriggerFatalException(const FunctionCallbackInfo& args) { - Isolate* isolate = args.GetIsolate(); - Environment* env = Environment::GetCurrent(isolate); - Local exception = args[0]; - Local message = Exception::CreateMessage(isolate, exception); - if (env != nullptr && env->abort_on_uncaught_exception()) { - ReportException(env, exception, message); - Abort(); - } - bool from_promise = args[1]->IsTrue(); - FatalException(isolate, exception, message, from_promise); -} - static void Initialize(Local target, Local unused, Local context, @@ -143,7 +129,6 @@ static void Initialize(Local target, Environment* env = Environment::GetCurrent(context); Isolate* isolate = env->isolate(); - env->SetMethod(target, "triggerFatalException", TriggerFatalException); env->SetMethod(target, "enqueueMicrotask", EnqueueMicrotask); env->SetMethod(target, "setTickCallback", SetTickCallback); env->SetMethod(target, "runMicrotasks", RunMicrotasks); diff --git a/src/node_util.cc b/src/node_util.cc index ab54c84379de84..518865fe53689d 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -123,14 +123,6 @@ static void PreviewEntries(const FunctionCallbackInfo& args) { Array::New(env->isolate(), ret, arraysize(ret))); } -// Side effect-free stringification that will never throw exceptions. -static void SafeToString(const FunctionCallbackInfo& args) { - Local context = args.GetIsolate()->GetCurrentContext(); - Local detail_string; - if (args[0]->ToDetailString(context).ToLocal(&detail_string)) - args.GetReturnValue().Set(detail_string); -} - inline Local IndexToPrivateSymbol(Environment* env, uint32_t index) { #define V(name, _) &Environment::name, static Local (Environment::*const methods[])() const = { @@ -270,7 +262,6 @@ void Initialize(Local target, env->SetMethod(target, "setHiddenValue", SetHiddenValue); env->SetMethodNoSideEffect(target, "getPromiseDetails", GetPromiseDetails); env->SetMethodNoSideEffect(target, "getProxyDetails", GetProxyDetails); - env->SetMethodNoSideEffect(target, "safeToString", SafeToString); env->SetMethodNoSideEffect(target, "previewEntries", PreviewEntries); env->SetMethodNoSideEffect(target, "getOwnNonIndexProperties", GetOwnNonIndexProperties); From 112373e38ac10ac99ef7372c02b0ce3618e96afe Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 17 Jun 2019 12:31:12 +0800 Subject: [PATCH 2/2] fixup! src: refactor uncaught exception handling --- src/node_errors.cc | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/node_errors.cc b/src/node_errors.cc index 3ac26608e7479c..f89b1472101c70 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -870,7 +870,8 @@ void TriggerUncaughtException(Isolate* isolate, process_object->Get(env->context(), fatal_exception_string).ToLocalChecked(); // If the exception happens before process._fatalException is attached - // during bootstrap, or if the user has patched it incorrectly, just crash. + // during bootstrap, or if the user has patched it incorrectly, exit + // the current Node.js instance. if (!fatal_exception_function->IsFunction()) { ReportException(env, error, message); env->Exit(6); @@ -880,9 +881,14 @@ void TriggerUncaughtException(Isolate* isolate, MaybeLocal handled; { // We do not expect the global uncaught exception itself to throw any more - // exceptions. If it does, crash. - errors::TryCatchScope fatal_try_catch( - env, errors::TryCatchScope::CatchMode::kFatal); + // exceptions. If it does, exit the current Node.js instance. + errors::TryCatchScope try_catch(env, + errors::TryCatchScope::CatchMode::kFatal); + // Explicitly disable verbose exception reporting - + // if process._fatalException() throws an error, we don't want it to + // trigger the per-isolate message listener which will call this + // function and recurse. + try_catch.SetVerbose(false); Local argv[2] = { error, Boolean::New(env->isolate(), from_promise) }; @@ -890,6 +896,14 @@ void TriggerUncaughtException(Isolate* isolate, env->context(), process_object, arraysize(argv), argv); } + // If process._fatalException() throws, we are now exiting the Node.js + // instance so return to continue the exit routine. + // TODO(joyeecheung): return a Maybe here to prevent the caller from + // stepping on the exit. + if (handled.IsEmpty()) { + return; + } + // The global uncaught exception handler returns true if the user handles it // by e.g. listening to `uncaughtException`. In that case, continue program // execution.