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. Refs: nodejs#17159 Refs: nodejs#17324
addaleax · Nov 27, 2017 · 703f474 · 703f474
1 parent bb44626
commit 703f474
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 71 deletions.
diff --git a/src/async_wrap.cc b/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)
+      .ToLocalChecked();
 }
 
 
@@ -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)
+      .ToLocalChecked();
 }
 
 
@@ -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)
+      .ToLocalChecked();
 }
 
 class PromiseWrap : public AsyncWrap {
@@ -748,14 +730,8 @@ 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).ToLocalChecked();
 }
 
 

diff --git a/src/node.cc b/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];

diff --git a/src/node_internals.h b/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 it crashes the process if it did catch an exception.
+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);

diff --git a/src/node_url.cc b/src/node_url.cc
@@ -2172,7 +2172,7 @@ const Local<Value> URL::ToObject(Environment* env) const {
   };
   SetArgs(env, argv, &context_);
 
-  TryCatch try_catch(isolate);
+  FatalTryCatch try_catch(env);
 
   // The SetURLConstructor method must have been called already to
   // set the constructor function used below. SetURLConstructor is
@@ -2182,11 +2182,6 @@ const Local<Value> URL::ToObject(Environment* env) const {
       env->url_constructor_function()
           ->Call(env->context(), undef, 9, argv);
 
-  if (ret.IsEmpty()) {
-    ClearFatalExceptionHandlers(env);
-    FatalException(isolate, try_catch);
-  }
-
   return ret.ToLocalChecked();
 }