From 6a7a59a8c1bd6c2f96b21c84b3149ce193ceb706 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 27 Nov 2017 01:31:24 +0100 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/17333 Refs: https://github.com/nodejs/node/pull/17159 Refs: https://github.com/nodejs/node/pull/17324 Reviewed-By: Anatoli Papirovski Reviewed-By: Timothy Gu Reviewed-By: Daniel Bevenius Reviewed-By: Andreas Madsen Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Colin Ihrig --- src/async_wrap.cc | 53 +++++++++++++------------------------------- src/node.cc | 37 +++++++++++++------------------ src/node_internals.h | 16 ++++++++----- src/node_url.cc | 23 +++++++++---------- 4 files changed, 51 insertions(+), 78 deletions(-) diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 99d14fb2933cda..b355841c3419d0 100644 --- 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 fn = env->async_hooks_destroy_function(); - TryCatch try_catch(env->isolate()); + FatalTryCatch try_catch(env); do { std::vector destroy_async_id_list; @@ -157,11 +157,8 @@ static void DestroyAsyncIdsCallback(uv_timer_t* handle) { MaybeLocal 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 async_id_value = Number::New(env->isolate(), async_id); Local fn = env->async_hooks_promise_resolve_function(); - TryCatch try_catch(env->isolate()); - MaybeLocal 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()); } @@ -209,14 +201,9 @@ void AsyncWrap::EmitBefore(Environment* env, double async_id) { Local async_id_value = Number::New(env->isolate(), async_id); Local fn = env->async_hooks_before_function(); - TryCatch try_catch(env->isolate()); - MaybeLocal 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()); } @@ -245,14 +232,9 @@ void AsyncWrap::EmitAfter(Environment* env, double async_id) { // end of _fatalException(). Local async_id_value = Number::New(env->isolate(), async_id); Local fn = env->async_hooks_after_function(); - TryCatch try_catch(env->isolate()); - MaybeLocal 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()); } class PromiseWrap : public AsyncWrap { @@ -753,14 +735,9 @@ void AsyncWrap::EmitAsyncInit(Environment* env, object, }; - TryCatch try_catch(env->isolate()); - MaybeLocal 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()); } diff --git a/src/node.cc b/src/node.cc index 5d9024bf5dca9a..d5631a5b061011 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1818,6 +1818,8 @@ void AppendExceptionLine(Environment* env, static void ReportException(Environment* env, Local er, Local 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 error, Local 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, Local error) { FatalException(Isolate::GetCurrent(), error, message); } - -void ClearFatalExceptionHandlers(Environment* env) { - Local process = env->process_object(); - Local events = - process->Get(env->context(), env->events_string()).ToLocalChecked(); - - if (events->IsObject()) { - events.As()->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 index d7fbcc992f6a0f..ed64b0fdbe183f 100644 --- 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 Copy(Environment* env, const char* data, size_t len); v8::MaybeLocal New(Environment* env, size_t size); diff --git a/src/node_url.cc b/src/node_url.cc index 67c6986da876c8..c9c8ccd579744e 100644 --- a/src/node_url.cc +++ b/src/node_url.cc @@ -2172,19 +2172,16 @@ const Local 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 ret = - env->url_constructor_function() - ->Call(env->context(), undef, 9, argv); - - if (ret.IsEmpty()) { - ClearFatalExceptionHandlers(env); - FatalException(isolate, try_catch); + MaybeLocal 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();