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: 2b252ac

PR-URL: #28257
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>

Author: joyeecheung

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 =

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');
@@ -151,24 +151,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,
@@ -182,7 +181,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();
 

lib/internal/modules/cjs/loader.js

@@ -829,7 +829,7 @@ Module.runMain = function() {
       return loader.import(pathToFileURL(process.argv[1]).pathname);
     })
     .catch((e) => {
-      internalBinding('task_queue').triggerFatalException(
+      internalBinding('errors').triggerUncaughtException(
         e,
         true /* fromPromise */
       );

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
 };

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 emitUnhandledRejectionWarning(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 emitUnhandledRejectionWarning(uid, reason) {
       warning.stack = reason.stack;
       process.emitWarning(reason.stack, unhandledRejectionErrName);
     } else {
-      process.emitWarning(safeToString(reason), unhandledRejectionErrName);
+      process.emitWarning(
+        noSideEffectsToString(reason), unhandledRejectionErrName);
     }
   } catch {}
 
@@ -179,7 +181,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) emitUnhandledRejectionWarning(uid, reason);
         break;
@@ -209,7 +213,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;
@@ -225,21 +229,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() {

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();
     }

src/api/exceptions.cc

@@ -244,17 +244,12 @@ Local<Value> 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

src/env.cc

@@ -666,7 +666,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

src/js_stream.cc

@@ -49,7 +49,7 @@ bool JSStream::IsClosing() {
   Local<Value> 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;
 }

src/node.cc

@@ -121,7 +121,6 @@ using options_parser::kDisallowedInEnvironment;
 
 using v8::Boolean;
 using v8::EscapableHandleScope;
-using v8::Exception;
 using v8::Function;
 using v8::FunctionCallbackInfo;
 using v8::HandleScope;
@@ -212,10 +211,9 @@ MaybeLocal<Value> 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().

src/node_api.cc

@@ -115,7 +115,7 @@ static inline void trigger_fatal_exception(
     napi_env env, v8::Local<v8::Value> local_err) {
   v8::Local<v8::Message> 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 {

src/node_contextify.cc

@@ -734,7 +734,7 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& 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;