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

PR-URL: https://github.com/nodejs/node/pull/28257
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

10 files changed, 127 insertions, 109 deletions

diff --git a/src/api/exceptions.cc b/src/api/exceptions.cc
index ee5ea984eb..1632574c6b 100644
--- a/src/api/exceptions.cc
+++ b/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
diff --git a/src/env.cc b/src/env.cc
index 9a251a9306..df59eaaa94 100644
--- a/src/env.cc
+++ b/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
diff --git a/src/js_stream.cc b/src/js_stream.cc
index 2663106ba7..23bdb9b489 100644
--- a/src/js_stream.cc
+++ b/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;
 }
diff --git a/src/node.cc b/src/node.cc
index c74dd91abe..8faac1212b 100644
--- a/src/node.cc
+++ b/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().
diff --git a/src/node_api.cc b/src/node_api.cc
index c4d8634286..3aab7a2257 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<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 {
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index ee5a9db574..70c63a0d8a 100644
--- a/src/node_contextify.cc
+++ b/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;
diff --git a/src/node_errors.cc b/src/node_errors.cc
index 50469b09a8..f89b147210 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> message, Local<Value> error) {
       break;
     }
     case Isolate::MessageErrorLevel::kMessageError:
-      FatalException(isolate, error, message);
+      TriggerUncaughtException(isolate, error, message);
       break;
   }
 }
@@ -775,6 +771,27 @@ void SetPrepareStackTraceCallback(const FunctionCallbackInfo<Value>& args) {
   env->set_prepare_stack_trace_callback(args[0].As<Function>());
 }
 
+// Side effect-free stringification that will never throw exceptions.
+static void NoSideEffectsToString(const FunctionCallbackInfo<Value>& args) {
+  Local<Context> context = args.GetIsolate()->GetCurrentContext();
+  Local<String> detail_string;
+  if (args[0]->ToDetailString(context).ToLocal(&detail_string))
+    args.GetReturnValue().Set(detail_string);
+}
+
+static void TriggerUncaughtException(const FunctionCallbackInfo<Value>& args) {
+  Isolate* isolate = args.GetIsolate();
+  Environment* env = Environment::GetCurrent(isolate);
+  Local<Value> exception = args[0];
+  Local<Message> 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<Object> target,
                 Local<Value> unused,
                 Local<Context> context,
@@ -782,10 +799,11 @@ void Initialize(Local<Object> 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<Value> 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<Value> error,
-                    Local<Message> message,
-                    bool from_promise) {
+void TriggerUncaughtException(Isolate* isolate,
+                              Local<Value> error,
+                              Local<Message> message,
+                              bool from_promise) {
   CHECK(!error.IsEmpty());
   HandleScope scope(isolate);
 
@@ -843,59 +861,95 @@ 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<Object> process_object = env->process_object();
   Local<String> fatal_exception_string = env->fatal_exception_string();
   Local<Value> 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, exit
+  // the current Node.js instance.
   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<Value> handled;
+  {
+    // We do not expect the global uncaught exception itself to throw any more
+    // 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<Value> argv[2] = { error,
                              Boolean::New(env->isolate(), from_promise) };
 
-    // This will return true if the JS layer handled it, false otherwise
-    MaybeLocal<Value> caught = fatal_exception_function.As<Function>()->Call(
+    handled = fatal_exception_function.As<Function>()->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);
+  // 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;
+  }
 
-    } 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<String> exit_code = env->exit_code_string();
-      Local<Value> code;
-      if (!process_object->Get(env->context(), exit_code).ToLocal(&code) ||
-          !code->IsInt32()) {
-        env->Exit(1);
-      }
-      env->Exit(code.As<Int32>()->Value());
-    }
+  ReportException(env, error, message);
+  // If the global uncaught exception handler sets process.exitCode,
+  // exit with that code. Otherwise, exit with 1.
+  Local<String> exit_code = env->exit_code_string();
+  Local<Value> code;
+  if (process_object->Get(env->context(), exit_code).ToLocal(&code) &&
+      code->IsInt32()) {
+    env->Exit(code.As<Int32>()->Value());
+  } else {
+    env->Exit(1);
   }
 }
 
-void FatalException(Isolate* isolate,
-                    Local<Value> error,
-                    Local<Message> 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 689911f996..939f93a489 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<Value> er,
-                     Local<Message> message);
-
-void FatalException(v8::Isolate* isolate,
-                    Local<Value> error,
-                    Local<Message> message);
-
-void FatalException(v8::Isolate* isolate,
-                    Local<Value> error,
-                    Local<Message> 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<Value> error,
+                              Local<Message> message,
+                              bool from_promise = false);
+
 const char* errno_string(int errorno);
 void PerIsolateMessageListener(v8::Local<v8::Message> message,
                                v8::Local<v8::Value> 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 a277b8bd2a..e6b4d0b8e2 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<Function>());
 }
 
-static void TriggerFatalException(const FunctionCallbackInfo<Value>& args) {
-  Isolate* isolate = args.GetIsolate();
-  Environment* env = Environment::GetCurrent(isolate);
-  Local<Value> exception = args[0];
-  Local<Message> 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<Object> target,
                        Local<Value> unused,
                        Local<Context> context,
@@ -143,7 +129,6 @@ static void Initialize(Local<Object> 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 ab54c84379..518865fe53 100644
--- a/src/node_util.cc
+++ b/src/node_util.cc
@@ -123,14 +123,6 @@ static void PreviewEntries(const FunctionCallbackInfo<Value>& args) {
       Array::New(env->isolate(), ret, arraysize(ret)));
 }
 
-// Side effect-free stringification that will never throw exceptions.
-static void SafeToString(const FunctionCallbackInfo<Value>& args) {
-  Local<Context> context = args.GetIsolate()->GetCurrentContext();
-  Local<String> detail_string;
-  if (args[0]->ToDetailString(context).ToLocal(&detail_string))
-    args.GetReturnValue().Set(detail_string);
-}
-
 inline Local<Private> IndexToPrivateSymbol(Environment* env, uint32_t index) {
 #define V(name, _) &Environment::name,
   static Local<Private> (Environment::*const methods[])() const = {
@@ -270,7 +262,6 @@ void Initialize(Local<Object> 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);