src: remove ClearFatalExceptionHandlers()

At its call sites, `ClearFatalExceptionHandlers()` was used to
make the process crash as soon as possible once an exception occurred,
without giving JS land a chance to interfere.

`ClearFatalExceptionHandlers()` awkwardly removed the current domain
and any `uncaughtException` handlers, whereas a clearer way is to
execute the relevant reporting (and `exit()`) code directly.

PR-URL: #17333
Refs: #17159
Refs: #17324
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

Author: addaleax

src/async_wrap.cc

@@ -140,7 +140,7 @@ RetainedObjectInfo* WrapperInfo(uint16_t class_id, Local<Value> wrapper) {
 static void DestroyAsyncIdsCallback(Environment* env, void* data) {
   Local<Function> fn = env->async_hooks_destroy_function();
 
-  TryCatch try_catch(env->isolate());
+  FatalTryCatch try_catch(env);
 
   do {
     std::vector<double> destroy_async_id_list;
@@ -153,11 +153,8 @@ static void DestroyAsyncIdsCallback(Environment* env, void* data) {
       MaybeLocal<Value> ret = fn->Call(
           env->context(), Undefined(env->isolate()), 1, &async_id_value);
 
-      if (ret.IsEmpty()) {
-        ClearFatalExceptionHandlers(env);
-        FatalException(env->isolate(), try_catch);
-        UNREACHABLE();
-      }
+      if (ret.IsEmpty())
+        return;
     }
   } while (!env->destroy_async_id_list()->empty());
 }
@@ -171,14 +168,9 @@ void AsyncWrap::EmitPromiseResolve(Environment* env, double async_id) {
 
   Local<Value> async_id_value = Number::New(env->isolate(), async_id);
   Local<Function> fn = env->async_hooks_promise_resolve_function();
-  TryCatch try_catch(env->isolate());
-  MaybeLocal<Value> ar = fn->Call(
-      env->context(), Undefined(env->isolate()), 1, &async_id_value);
-  if (ar.IsEmpty()) {
-    ClearFatalExceptionHandlers(env);
-    FatalException(env->isolate(), try_catch);
-    UNREACHABLE();
-  }
+  FatalTryCatch try_catch(env);
+  fn->Call(env->context(), Undefined(env->isolate()), 1, &async_id_value)
+      .FromMaybe(Local<Value>());
 }
 
 
@@ -205,14 +197,9 @@ void AsyncWrap::EmitBefore(Environment* env, double async_id) {
 
   Local<Value> async_id_value = Number::New(env->isolate(), async_id);
   Local<Function> fn = env->async_hooks_before_function();
-  TryCatch try_catch(env->isolate());
-  MaybeLocal<Value> ar = fn->Call(
-      env->context(), Undefined(env->isolate()), 1, &async_id_value);
-  if (ar.IsEmpty()) {
-    ClearFatalExceptionHandlers(env);
-    FatalException(env->isolate(), try_catch);
-    UNREACHABLE();
-  }
+  FatalTryCatch try_catch(env);
+  fn->Call(env->context(), Undefined(env->isolate()), 1, &async_id_value)
+      .FromMaybe(Local<Value>());
 }
 
 
@@ -241,14 +228,9 @@ void AsyncWrap::EmitAfter(Environment* env, double async_id) {
   // end of _fatalException().
   Local<Value> async_id_value = Number::New(env->isolate(), async_id);
   Local<Function> fn = env->async_hooks_after_function();
-  TryCatch try_catch(env->isolate());
-  MaybeLocal<Value> ar = fn->Call(
-      env->context(), Undefined(env->isolate()), 1, &async_id_value);
-  if (ar.IsEmpty()) {
-    ClearFatalExceptionHandlers(env);
-    FatalException(env->isolate(), try_catch);
-    UNREACHABLE();
-  }
+  FatalTryCatch try_catch(env);
+  fn->Call(env->context(), Undefined(env->isolate()), 1, &async_id_value)
+      .FromMaybe(Local<Value>());
 }
 
 class PromiseWrap : public AsyncWrap {
@@ -748,14 +730,9 @@ void AsyncWrap::EmitAsyncInit(Environment* env,
     object,
   };
 
-  TryCatch try_catch(env->isolate());
-  MaybeLocal<Value> ret = init_fn->Call(
-      env->context(), object, arraysize(argv), argv);
-
-  if (ret.IsEmpty()) {
-    ClearFatalExceptionHandlers(env);
-    FatalException(env->isolate(), try_catch);
-  }
+  FatalTryCatch try_catch(env);
+  init_fn->Call(env->context(), object, arraysize(argv), argv)
+      .FromMaybe(Local<Value>());
 }
 
 

src/node.cc

@@ -1471,6 +1471,8 @@ void AppendExceptionLine(Environment* env,
 static void ReportException(Environment* env,
                             Local<Value> er,
                             Local<Message> message) {
+  CHECK(!er.IsEmpty());
+  CHECK(!message.IsEmpty());
   HandleScope scope(env->isolate());
 
   AppendExceptionLine(env, er, message, FATAL_ERROR);
@@ -1540,6 +1542,10 @@ static void ReportException(Environment* env,
   }
 
   fflush(stderr);
+
+#if HAVE_INSPECTOR
+  env->inspector_agent()->FatalException(er, message);
+#endif
 }
 
 
@@ -2399,6 +2405,15 @@ NO_RETURN void FatalError(const char* location, const char* message) {
 }
 
 
+FatalTryCatch::~FatalTryCatch() {
+  if (HasCaught()) {
+    HandleScope scope(env_->isolate());
+    ReportException(env_, *this);
+    exit(7);
+  }
+}
+
+
 void FatalException(Isolate* isolate,
                     Local<Value> error,
                     Local<Message> message) {
@@ -2441,9 +2456,6 @@ void FatalException(Isolate* isolate,
   }
 
   if (exit_code) {
-#if HAVE_INSPECTOR
-    env->inspector_agent()->FatalException(error, message);
-#endif
     exit(exit_code);
   }
 }
@@ -2463,25 +2475,6 @@ static void OnMessage(Local<Message> message, Local<Value> error) {
   FatalException(Isolate::GetCurrent(), error, message);
 }
 
-
-void ClearFatalExceptionHandlers(Environment* env) {
-  Local<Object> process = env->process_object();
-  Local<Value> events =
-      process->Get(env->context(), env->events_string()).ToLocalChecked();
-
-  if (events->IsObject()) {
-    events.As<Object>()->Set(
-        env->context(),
-        OneByteString(env->isolate(), "uncaughtException"),
-        Undefined(env->isolate())).FromJust();
-  }
-
-  process->Set(
-      env->context(),
-      env->domain_string(),
-      Undefined(env->isolate())).FromJust();
-}
-
 // Call process.emitWarning(str), fmt is a snprintf() format string
 void ProcessEmitWarning(Environment* env, const char* fmt, ...) {
   char warning[1024];

src/node_internals.h

@@ -277,6 +277,17 @@ void AppendExceptionLine(Environment* env,
 
 NO_RETURN void FatalError(const char* location, const char* message);
 
+// Like a `TryCatch` but exits the process if an exception was caught.
+class FatalTryCatch : public v8::TryCatch {
+ public:
+  explicit FatalTryCatch(Environment* env)
+      : TryCatch(env->isolate()), env_(env) {}
+  ~FatalTryCatch();
+
+ private:
+  Environment* env_;
+};
+
 void ProcessEmitWarning(Environment* env, const char* fmt, ...);
 
 void FillStatsArray(double* fields, const uv_stat_t* s);
@@ -330,11 +341,6 @@ class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
   uint32_t zero_fill_field_ = 1;  // Boolean but exposed as uint32 to JS land.
 };
 
-// Clear any domain and/or uncaughtException handlers to force the error's
-// propagation and shutdown the process. Use this to force the process to exit
-// by clearing all callbacks that could handle the error.
-void ClearFatalExceptionHandlers(Environment* env);
-
 namespace Buffer {
 v8::MaybeLocal<v8::Object> Copy(Environment* env, const char* data, size_t len);
 v8::MaybeLocal<v8::Object> New(Environment* env, size_t size);

src/node_url.cc

@@ -2172,19 +2172,16 @@ const Local<Value> URL::ToObject(Environment* env) const {
   };
   SetArgs(env, argv, &context_);
 
-  TryCatch try_catch(isolate);
-
-  // The SetURLConstructor method must have been called already to
-  // set the constructor function used below. SetURLConstructor is
-  // called automatically when the internal/url.js module is loaded
-  // during the internal/bootstrap_node.js processing.
-  MaybeLocal<Value> ret =
-      env->url_constructor_function()
-          ->Call(env->context(), undef, 9, argv);
-
-  if (ret.IsEmpty()) {
-    ClearFatalExceptionHandlers(env);
-    FatalException(isolate, try_catch);
+  MaybeLocal<Value> ret;
+  {
+    FatalTryCatch try_catch(env);
+
+    // The SetURLConstructor method must have been called already to
+    // set the constructor function used below. SetURLConstructor is
+    // called automatically when the internal/url.js module is loaded
+    // during the internal/bootstrap_node.js processing.
+    ret = env->url_constructor_function()
+        ->Call(env->context(), undef, arraysize(argv), argv);
   }
 
   return ret.ToLocalChecked();