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 <apapirovski@mac.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
nodejs · Dec 12, 2017 · 6a7a59a · 6a7a59a
1 parent 6e3a8be
commit 6a7a59a
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 78 deletions.
diff --git a/src/async_wrap.cc b/src/async_wrap.cc
@@ -144,7 +144,7 @@ static void DestroyAsyncIdsCallback(uv_timer_t* handle) {
   Context::Scope context_scope(env->context());
   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;
@@ -157,11 +157,8 @@ static void DestroyAsyncIdsCallback(uv_timer_t* handle) {
       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());
 }
@@ -175,14 +172,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>());
 }
 
 
@@ -209,14 +201,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>());
 }
 
 
@@ -245,14 +232,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 {
@@ -753,14 +735,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>());
 }
 
 

diff --git a/src/node.cc b/src/node.cc
@@ -1818,6 +1818,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);
@@ -1887,6 +1889,10 @@ static void ReportException(Environment* env,
   }
 
   fflush(stderr);
+
+#if HAVE_INSPECTOR
+  env->inspector_agent()->FatalException(er, message);
+#endif
 }
 
 
@@ -2746,6 +2752,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) {
@@ -2788,9 +2803,6 @@ void FatalException(Isolate* isolate,
   }
 
   if (exit_code) {
-#if HAVE_INSPECTOR
-    env->inspector_agent()->FatalException(error, message);
-#endif
     exit(exit_code);
   }
 }
@@ -2810,25 +2822,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
@@ -270,6 +270,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);
@@ -323,11 +334,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,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();