From 981bbcd9256379d15d5d2fafd60e78f749579729 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 11 Feb 2016 13:57:26 -0700 Subject: [PATCH] src: remove TryCatch in MakeCallback After attempting to use ReThrow() and Reset() there were cases where firing the domain's error handlers was not happening. Or in some cases reentering MakeCallback would still cause the domain enter callback to abort (because the error had not been Reset yet). In order for the script to properly stop execution when a subsequent call to MakeCallback throws it must not be located within a TryCatch. Ref: https://github.com/nodejs/node/pull/7048 PR-URL: https://github.com/nodejs/node/pull/4507 Reviewed-By: Fedor Indutny --- src/async-wrap.cc | 28 +++++++++++----------------- src/env.cc | 13 ++++--------- src/node.cc | 27 ++++++++++++--------------- 3 files changed, 27 insertions(+), 41 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 789e357c732773..a7a9822238329a 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -196,33 +196,28 @@ Local AsyncWrap::MakeCallback(const Local cb, } } - TryCatch try_catch(env()->isolate()); - try_catch.SetVerbose(true); - if (has_domain) { Local enter_v = domain->Get(env()->enter_string()); if (enter_v->IsFunction()) { - enter_v.As()->Call(domain, 0, nullptr); - if (try_catch.HasCaught()) - return Undefined(env()->isolate()); + if (enter_v.As()->Call(domain, 0, nullptr).IsEmpty()) { + FatalError("node::AsyncWrap::MakeCallback", + "domain enter callback threw, please report this"); + } } } if (ran_init_callback() && !pre_fn.IsEmpty()) { - pre_fn->Call(context, 0, nullptr); - if (try_catch.HasCaught()) + if (pre_fn->Call(context, 0, nullptr).IsEmpty()) FatalError("node::AsyncWrap::MakeCallback", "pre hook threw"); } Local ret = cb->Call(context, argc, argv); if (ran_init_callback() && !post_fn.IsEmpty()) { - post_fn->Call(context, 0, nullptr); - if (try_catch.HasCaught()) + if (post_fn->Call(context, 0, nullptr).IsEmpty()) FatalError("node::AsyncWrap::MakeCallback", "post hook threw"); } - // If the return value is empty then the callback threw. if (ret.IsEmpty()) { return Undefined(env()->isolate()); } @@ -230,9 +225,10 @@ Local AsyncWrap::MakeCallback(const Local cb, if (has_domain) { Local exit_v = domain->Get(env()->exit_string()); if (exit_v->IsFunction()) { - exit_v.As()->Call(domain, 0, nullptr); - if (try_catch.HasCaught()) - return Undefined(env()->isolate()); + if (exit_v.As()->Call(domain, 0, nullptr).IsEmpty()) { + FatalError("node::AsyncWrap::MakeCallback", + "domain exit callback threw, please report this"); + } } } @@ -251,9 +247,7 @@ Local AsyncWrap::MakeCallback(const Local cb, return ret; } - env()->tick_callback_function()->Call(process, 0, nullptr); - - if (try_catch.HasCaught()) { + if (env()->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) { return Undefined(env()->isolate()); } diff --git a/src/env.cc b/src/env.cc index a9b108c47a49be..144a37f2e3cb67 100644 --- a/src/env.cc +++ b/src/env.cc @@ -11,6 +11,7 @@ using v8::Message; using v8::StackFrame; using v8::StackTrace; using v8::TryCatch; +using v8::Value; void Environment::PrintSyncTrace() const { if (!trace_sync_io_) @@ -73,16 +74,10 @@ bool Environment::KickNextTick(Environment::AsyncCallbackScope* scope) { return true; } - // process nextTicks after call - TryCatch try_catch(isolate()); - try_catch.SetVerbose(true); - tick_callback_function()->Call(process_object(), 0, nullptr); + Local ret = + tick_callback_function()->Call(process_object(), 0, nullptr); - if (try_catch.HasCaught()) { - return false; - } - - return true; + return !ret.IsEmpty(); } } // namespace node diff --git a/src/node.cc b/src/node.cc index fc39b8399a1ffa..5b4b2f8c8affa0 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1154,33 +1154,28 @@ Local MakeCallback(Environment* env, } } - TryCatch try_catch(env->isolate()); - try_catch.SetVerbose(true); - if (has_domain) { Local enter_v = domain->Get(env->enter_string()); if (enter_v->IsFunction()) { - enter_v.As()->Call(domain, 0, nullptr); - if (try_catch.HasCaught()) - return Undefined(env->isolate()); + if (enter_v.As()->Call(domain, 0, nullptr).IsEmpty()) { + FatalError("node::MakeCallback", + "domain enter callback threw, please report this"); + } } } if (ran_init_callback && !pre_fn.IsEmpty()) { - pre_fn->Call(object, 0, nullptr); - if (try_catch.HasCaught()) + if (pre_fn->Call(object, 0, nullptr).IsEmpty()) FatalError("node::MakeCallback", "pre hook threw"); } Local ret = callback->Call(recv, argc, argv); if (ran_init_callback && !post_fn.IsEmpty()) { - post_fn->Call(object, 0, nullptr); - if (try_catch.HasCaught()) + if (post_fn->Call(object, 0, nullptr).IsEmpty()) FatalError("node::MakeCallback", "post hook threw"); } - // If the return value is empty then the callback threw. if (ret.IsEmpty()) { return Undefined(env->isolate()); } @@ -1188,14 +1183,16 @@ Local MakeCallback(Environment* env, if (has_domain) { Local exit_v = domain->Get(env->exit_string()); if (exit_v->IsFunction()) { - exit_v.As()->Call(domain, 0, nullptr); - if (try_catch.HasCaught()) - return Undefined(env->isolate()); + if (exit_v.As()->Call(domain, 0, nullptr).IsEmpty()) { + FatalError("node::MakeCallback", + "domain exit callback threw, please report this"); + } } } - if (!env->KickNextTick(&callback_scope)) + if (!env->KickNextTick(&callback_scope)) { return Undefined(env->isolate()); + } return ret; }