From f084e06de7b03750a80925a7aa1aefe3aed47504 Mon Sep 17 00:00:00 2001 From: Gus Caplan Date: Fri, 30 Nov 2018 08:13:45 -0600 Subject: [PATCH] src: use custom TryCatch subclass PR-URL: https://github.com/nodejs/node/pull/24751 Reviewed-By: Joyee Cheung --- src/async_wrap.cc | 9 +++-- src/env.cc | 9 +++-- src/js_stream.cc | 14 ++++--- src/module_wrap.cc | 9 +++-- src/node.cc | 8 ++-- src/node_contextify.cc | 11 +++--- src/node_contextify.h | 5 ++- src/node_errors.cc | 90 ++++++++++++++++++++++-------------------- src/node_errors.h | 31 ++++++++++++++- src/node_url.cc | 5 ++- 10 files changed, 116 insertions(+), 75 deletions(-) diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 6a354c0aec898c..2c9aa3cb884220 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -58,6 +58,7 @@ using v8::WeakCallbackInfo; using v8::WeakCallbackType; using AsyncHooks = node::Environment::AsyncHooks; +using TryCatchScope = node::errors::TryCatchScope; namespace node { @@ -91,7 +92,7 @@ struct AsyncWrapObject : public AsyncWrap { static void DestroyAsyncIdsCallback(Environment* env, void* data) { Local fn = env->async_hooks_destroy_function(); - FatalTryCatch try_catch(env); + TryCatchScope try_catch(env, TryCatchScope::CatchMode::kFatal); do { std::vector destroy_async_id_list; @@ -127,7 +128,7 @@ void Emit(Environment* env, double async_id, AsyncHooks::Fields type, HandleScope handle_scope(env->isolate()); Local async_id_value = Number::New(env->isolate(), async_id); - FatalTryCatch try_catch(env); + TryCatchScope try_catch(env, TryCatchScope::CatchMode::kFatal); USE(fn->Call(env->context(), Undefined(env->isolate()), 1, &async_id_value)); } @@ -673,7 +674,7 @@ void AsyncWrap::EmitAsyncInit(Environment* env, object, }; - FatalTryCatch try_catch(env); + TryCatchScope try_catch(env, TryCatchScope::CatchMode::kFatal); USE(init_fn->Call(env->context(), object, arraysize(argv), argv)); } @@ -776,7 +777,7 @@ Local AsyncWrap::GetOwner(Environment* env, Local obj) { EscapableHandleScope handle_scope(env->isolate()); CHECK(!obj.IsEmpty()); - TryCatch ignore_exceptions(env->isolate()); + TryCatchScope ignore_exceptions(env); while (true) { Local owner; if (!obj->Get(env->context(), diff --git a/src/env.cc b/src/env.cc index 5731bad9932603..bdb3c1ea247e21 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1,6 +1,7 @@ #include "async_wrap.h" #include "node_buffer.h" #include "node_context_data.h" +#include "node_errors.h" #include "node_file.h" #include "node_internals.h" #include "node_native_module.h" @@ -15,6 +16,7 @@ namespace node { +using errors::TryCatchScope; using v8::Context; using v8::EmbedderGraph; using v8::External; @@ -36,7 +38,6 @@ using v8::StackTrace; using v8::String; using v8::Symbol; using v8::TracingController; -using v8::TryCatch; using v8::Undefined; using v8::Value; using worker::Worker; @@ -156,7 +157,7 @@ void Environment::TrackingTraceStateObserver::UpdateTraceCategoryState() { Local cb = env_->trace_category_state_function(); if (cb.IsEmpty()) return; - TryCatch try_catch(isolate); + TryCatchScope try_catch(env_); try_catch.SetVerbose(true); cb->Call(env_->context(), Undefined(isolate), 0, nullptr).ToLocalChecked(); @@ -577,7 +578,7 @@ void Environment::RunAndClearNativeImmediates() { std::vector list; native_immediate_callbacks_.swap(list); auto drain_list = [&]() { - TryCatch try_catch(isolate()); + TryCatchScope try_catch(this); for (auto it = list.begin(); it != list.end(); ++it) { #ifdef DEBUG v8::SealHandleScope seal_handle_scope(isolate()); @@ -642,7 +643,7 @@ void Environment::RunTimers(uv_timer_t* handle) { // impossible for us to end up in an infinite loop due to how the JS-side // is structured. do { - TryCatch try_catch(env->isolate()); + TryCatchScope try_catch(env); try_catch.SetVerbose(true); ret = cb->Call(env->context(), process, 1, &arg); } while (ret.IsEmpty() && env->can_call_into_js()); diff --git a/src/js_stream.cc b/src/js_stream.cc index 2d07b11b636ef9..31e15ec308326b 100644 --- a/src/js_stream.cc +++ b/src/js_stream.cc @@ -3,12 +3,15 @@ #include "async_wrap.h" #include "env-inl.h" #include "node_buffer.h" +#include "node_errors.h" #include "node_internals.h" #include "stream_base-inl.h" #include "v8.h" namespace node { +using errors::TryCatchScope; + using v8::Array; using v8::Context; using v8::FunctionCallbackInfo; @@ -18,7 +21,6 @@ using v8::Int32; using v8::Local; using v8::Object; using v8::String; -using v8::TryCatch; using v8::Value; @@ -42,7 +44,7 @@ bool JSStream::IsAlive() { bool JSStream::IsClosing() { HandleScope scope(env()->isolate()); Context::Scope context_scope(env()->context()); - TryCatch try_catch(env()->isolate()); + TryCatchScope try_catch(env()); Local value; if (!MakeCallback(env()->isclosing_string(), 0, nullptr).ToLocal(&value)) { if (!try_catch.HasTerminated()) @@ -56,7 +58,7 @@ bool JSStream::IsClosing() { int JSStream::ReadStart() { HandleScope scope(env()->isolate()); Context::Scope context_scope(env()->context()); - TryCatch try_catch(env()->isolate()); + TryCatchScope try_catch(env()); Local value; int value_int = UV_EPROTO; if (!MakeCallback(env()->onreadstart_string(), 0, nullptr).ToLocal(&value) || @@ -71,7 +73,7 @@ int JSStream::ReadStart() { int JSStream::ReadStop() { HandleScope scope(env()->isolate()); Context::Scope context_scope(env()->context()); - TryCatch try_catch(env()->isolate()); + TryCatchScope try_catch(env()); Local value; int value_int = UV_EPROTO; if (!MakeCallback(env()->onreadstop_string(), 0, nullptr).ToLocal(&value) || @@ -91,7 +93,7 @@ int JSStream::DoShutdown(ShutdownWrap* req_wrap) { req_wrap->object() }; - TryCatch try_catch(env()->isolate()); + TryCatchScope try_catch(env()); Local value; int value_int = UV_EPROTO; if (!MakeCallback(env()->onshutdown_string(), @@ -126,7 +128,7 @@ int JSStream::DoWrite(WriteWrap* w, bufs_arr }; - TryCatch try_catch(env()->isolate()); + TryCatchScope try_catch(env()); Local value; int value_int = UV_EPROTO; if (!MakeCallback(env()->onwrite_string(), diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 5443b2e07dc856..9a3c1cb2e2c26e 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -14,6 +14,8 @@ namespace node { namespace loader { +using errors::TryCatchScope; + using node::contextify::ContextifyContext; using node::url::URL; using node::url::URL_FLAGS_FAILED; @@ -40,7 +42,6 @@ using v8::Promise; using v8::ScriptCompiler; using v8::ScriptOrigin; using v8::String; -using v8::TryCatch; using v8::Undefined; using v8::Value; @@ -135,7 +136,7 @@ void ModuleWrap::New(const FunctionCallbackInfo& args) { } Environment::ShouldNotAbortOnUncaughtScope no_abort_scope(env); - TryCatch try_catch(isolate); + TryCatchScope try_catch(env); Local module; Local host_defined_options = @@ -244,7 +245,7 @@ void ModuleWrap::Instantiate(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&obj, args.This()); Local context = obj->context_.Get(isolate); Local module = obj->module_.Get(isolate); - TryCatch try_catch(isolate); + TryCatchScope try_catch(env); Maybe ok = module->InstantiateModule(context, ResolveCallback); // clear resolve cache on instantiate @@ -279,7 +280,7 @@ void ModuleWrap::Evaluate(const FunctionCallbackInfo& args) { bool break_on_sigint = args[1]->IsTrue(); Environment::ShouldNotAbortOnUncaughtScope no_abort_scope(env); - TryCatch try_catch(isolate); + TryCatchScope try_catch(env); bool timed_out = false; bool received_signal = false; diff --git a/src/node.cc b/src/node.cc index 01690a1e6c94da..975373baa6c56d 100644 --- a/src/node.cc +++ b/src/node.cc @@ -116,6 +116,7 @@ typedef int mode_t; namespace node { +using errors::TryCatchScope; using native_module::NativeModuleLoader; using options_parser::kAllowedInEnvironment; using options_parser::kDisallowedInEnvironment; @@ -154,7 +155,6 @@ using v8::SealHandleScope; using v8::SideEffectType; using v8::String; using v8::TracingController; -using v8::TryCatch; using v8::Undefined; using v8::V8; using v8::Value; @@ -746,7 +746,7 @@ static MaybeLocal ExecuteString(Environment* env, Local source, Local filename) { EscapableHandleScope scope(env->isolate()); - TryCatch try_catch(env->isolate()); + TryCatchScope try_catch(env); // try_catch must be nonverbose to disable FatalException() handler, // we will handle exceptions ourself. @@ -1341,7 +1341,7 @@ static MaybeLocal GetBootstrapper( Local script_name) { EscapableHandleScope scope(env->isolate()); - TryCatch try_catch(env->isolate()); + TryCatchScope try_catch(env); // Disable verbose mode to stop FatalException() handler from trying // to handle the exception. Errors this early in the start-up phase @@ -1386,7 +1386,7 @@ static bool ExecuteBootstrapper(Environment* env, Local bootstrapper, void LoadEnvironment(Environment* env) { HandleScope handle_scope(env->isolate()); - TryCatch try_catch(env->isolate()); + TryCatchScope try_catch(env); // Disable verbose mode to stop FatalException() handler from trying // to handle the exception. Errors this early in the start-up phase // are not safe to ignore. diff --git a/src/node_contextify.cc b/src/node_contextify.cc index bc08e31a065306..f12d5e7d66724c 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -31,6 +31,8 @@ namespace node { namespace contextify { +using errors::TryCatchScope; + using v8::Array; using v8::ArrayBuffer; using v8::ArrayBufferView; @@ -63,7 +65,6 @@ using v8::ScriptCompiler; using v8::ScriptOrigin; using v8::String; using v8::Symbol; -using v8::TryCatch; using v8::Uint32; using v8::UnboundScript; using v8::Value; @@ -245,7 +246,7 @@ void ContextifyContext::MakeContext(const FunctionCallbackInfo& args) { CHECK(args[4]->IsBoolean()); options.allow_code_gen_wasm = args[4].As(); - TryCatch try_catch(env->isolate()); + TryCatchScope try_catch(env); ContextifyContext* context = new ContextifyContext(env, sandbox, options); if (try_catch.HasCaught()) { @@ -705,7 +706,7 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { if (source.GetCachedData() != nullptr) compile_options = ScriptCompiler::kConsumeCodeCache; - TryCatch try_catch(isolate); + TryCatchScope try_catch(env); Environment::ShouldNotAbortOnUncaughtScope no_abort_scope(env); Context::Scope scope(parsing_context); @@ -863,7 +864,7 @@ bool ContextifyScript::EvalMachine(Environment* env, "Script methods can only be called on script instances."); return false; } - TryCatch try_catch(env->isolate()); + TryCatchScope try_catch(env); ContextifyScript* wrapped_script; ASSIGN_OR_RETURN_UNWRAP(&wrapped_script, args.Holder(), false); Local unbound_script = @@ -1012,7 +1013,7 @@ void ContextifyContext::CompileFunction( options = ScriptCompiler::kConsumeCodeCache; } - TryCatch try_catch(isolate); + TryCatchScope try_catch(env); Context::Scope scope(parsing_context); // Read context extensions from buffer diff --git a/src/node_contextify.h b/src/node_contextify.h index 68ba2707f8d53b..9884a627410316 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -3,9 +3,10 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS -#include "node_internals.h" -#include "node_context_data.h" #include "base_object-inl.h" +#include "node_context_data.h" +#include "node_errors.h" +#include "node_internals.h" namespace node { namespace contextify { diff --git a/src/node_errors.cc b/src/node_errors.cc index d78608bf7bf24c..b7f7f59e70ef62 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -3,6 +3,7 @@ #include "node_internals.h" namespace node { + using v8::Context; using v8::Exception; using v8::Function; @@ -20,7 +21,6 @@ using v8::Number; using v8::Object; using v8::ScriptOrigin; using v8::String; -using v8::TryCatch; using v8::Undefined; using v8::Value; @@ -258,45 +258,10 @@ void ReportException(Environment* env, #endif } -void ReportException(Environment* env, const TryCatch& try_catch) { +void ReportException(Environment* env, const v8::TryCatch& try_catch) { ReportException(env, try_catch.Exception(), try_catch.Message()); } -void DecorateErrorStack(Environment* env, const TryCatch& try_catch) { - Local exception = try_catch.Exception(); - - if (!exception->IsObject()) return; - - Local err_obj = exception.As(); - - if (IsExceptionDecorated(env, err_obj)) return; - - AppendExceptionLine(env, exception, try_catch.Message(), CONTEXTIFY_ERROR); - Local stack = err_obj->Get(env->context(), - env->stack_string()).ToLocalChecked(); - MaybeLocal maybe_value = - err_obj->GetPrivate(env->context(), env->arrow_message_private_symbol()); - - Local arrow; - if (!(maybe_value.ToLocal(&arrow) && arrow->IsString())) { - return; - } - - if (stack.IsEmpty() || !stack->IsString()) { - return; - } - - Local decorated_stack = String::Concat( - env->isolate(), - String::Concat(env->isolate(), - arrow.As(), - FIXED_ONE_BYTE_STRING(env->isolate(), "\n")), - stack.As()); - err_obj->Set(env->context(), env->stack_string(), decorated_stack).FromJust(); - err_obj->SetPrivate( - env->context(), env->decorated_private_symbol(), True(env->isolate())); -} - void PrintErrorString(const char* format, ...) { va_list ap; va_start(ap, format); @@ -347,14 +312,54 @@ void OnFatalError(const char* location, const char* message) { ABORT(); } -FatalTryCatch::~FatalTryCatch() { - if (HasCaught()) { +namespace errors { + +TryCatchScope::~TryCatchScope() { + if (HasCaught() && mode_ == CatchMode::kFatal) { HandleScope scope(env_->isolate()); - ReportException(env_, *this); + ReportException(env_, Exception(), Message()); exit(7); } } +} // namespace errors + +void DecorateErrorStack(Environment* env, + const errors::TryCatchScope& try_catch) { + Local exception = try_catch.Exception(); + + if (!exception->IsObject()) return; + + Local err_obj = exception.As(); + + if (IsExceptionDecorated(env, err_obj)) return; + + AppendExceptionLine(env, exception, try_catch.Message(), CONTEXTIFY_ERROR); + Local stack = + err_obj->Get(env->context(), env->stack_string()).ToLocalChecked(); + MaybeLocal maybe_value = + err_obj->GetPrivate(env->context(), env->arrow_message_private_symbol()); + + Local arrow; + if (!(maybe_value.ToLocal(&arrow) && arrow->IsString())) { + return; + } + + if (stack.IsEmpty() || !stack->IsString()) { + return; + } + + Local decorated_stack = String::Concat( + env->isolate(), + String::Concat(env->isolate(), + arrow.As(), + FIXED_ONE_BYTE_STRING(env->isolate(), "\n")), + stack.As()); + err_obj->Set(env->context(), env->stack_string(), decorated_stack).FromJust(); + err_obj->SetPrivate( + env->context(), env->decorated_private_symbol(), True(env->isolate())); +} + void FatalException(Isolate* isolate, Local error, Local message) { @@ -374,7 +379,7 @@ void FatalException(Isolate* isolate, ReportException(env, error, message); exit(6); } else { - TryCatch fatal_try_catch(isolate); + errors::TryCatchScope fatal_try_catch(env); // Do not call FatalException when _fatalException handler throws fatal_try_catch.SetVerbose(false); @@ -389,6 +394,7 @@ void FatalException(Isolate* isolate, // The fatal exception function threw, so we must exit ReportException(env, fatal_try_catch); exit(7); + } else if (caught.ToLocalChecked()->IsFalse()) { ReportException(env, error, message); @@ -405,7 +411,7 @@ void FatalException(Isolate* isolate, } } -void FatalException(Isolate* isolate, const TryCatch& try_catch) { +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 diff --git a/src/node_errors.h b/src/node_errors.h index 44dc1d8094f254..7638ff4251f95e 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -14,8 +14,6 @@ namespace node { -void DecorateErrorStack(Environment* env, const v8::TryCatch& try_catch); - enum ErrorHandlingMode { CONTEXTIFY_ERROR, FATAL_ERROR, MODULE_ERROR }; void AppendExceptionLine(Environment* env, v8::Local er, @@ -167,6 +165,35 @@ inline v8::Local ERR_STRING_TOO_LONG(v8::Isolate* isolate) { prefix " must be a string"); \ } while (0) +namespace errors { + +class TryCatchScope : public v8::TryCatch { + public: + enum class CatchMode { kNormal, kFatal }; + + explicit TryCatchScope(Environment* env, CatchMode mode = CatchMode::kNormal) + : v8::TryCatch(env->isolate()), env_(env), mode_(mode) {} + ~TryCatchScope(); + + // Since the dtor is not virtual we need to make sure no one creates + // object of it in the free store that might be held by polymorphic pointers. + void* operator new(std::size_t count) = delete; + void* operator new[](std::size_t count) = delete; + TryCatchScope(TryCatchScope&) = delete; + TryCatchScope(TryCatchScope&&) = delete; + TryCatchScope operator=(TryCatchScope&) = delete; + TryCatchScope operator=(TryCatchScope&&) = delete; + + private: + Environment* env_; + CatchMode mode_; +}; + +} // namespace errors + +void DecorateErrorStack(Environment* env, + const errors::TryCatchScope& try_catch); + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/node_url.cc b/src/node_url.cc index b9fda6cf43e521..d5dba61ae4cc84 100644 --- a/src/node_url.cc +++ b/src/node_url.cc @@ -11,6 +11,8 @@ namespace node { +using errors::TryCatchScope; + using v8::Array; using v8::Context; using v8::Function; @@ -25,7 +27,6 @@ using v8::NewStringType; using v8::Null; using v8::Object; using v8::String; -using v8::TryCatch; using v8::Undefined; using v8::Value; @@ -2406,7 +2407,7 @@ const Local URL::ToObject(Environment* env) const { MaybeLocal ret; { - FatalTryCatch try_catch(env); + TryCatchScope try_catch(env, TryCatchScope::CatchMode::kFatal); // The SetURLConstructor method must have been called already to // set the constructor function used below. SetURLConstructor is