From 02f28792027c302338b77e02b0764c351a8c4098 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Fri, 30 Apr 2021 17:51:11 -0400 Subject: [PATCH 1/6] node-api: fix shutdown crashes Refs: https://github.com/nodejs/node-addon-api/issues/906 Ensure that finalization is not defered during shutdown. The env for the addon is deleted immediately after iterating the list of finalizers to be run. Defering causes crashes as the finalization uses the already deleted env. Signed-off-by: Michael Dawson --- src/js_native_api_v8.h | 7 ++++++- src/node_api.cc | 12 ++++++++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 1a62782c1ad24f..127f84844ef6dd 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -54,7 +54,8 @@ class RefTracker { struct napi_env__ { explicit napi_env__(v8::Local context) : isolate(context->GetIsolate()), - context_persistent(isolate, context) { + context_persistent(isolate, context), + is_env_teardown(false) { CHECK_EQ(isolate, context->GetIsolate()); } virtual ~napi_env__() { @@ -63,11 +64,15 @@ struct napi_env__ { // they delete during their `napi_finalizer` callbacks. If we deleted such // references here first, they would be doubly deleted when the // `napi_finalizer` deleted them subsequently. + is_env_teardown = true; v8impl::RefTracker::FinalizeAll(&finalizing_reflist); v8impl::RefTracker::FinalizeAll(&reflist); } v8::Isolate* const isolate; // Shortcut for context()->GetIsolate() v8impl::Persistent context_persistent; + bool is_env_teardown; + + inline bool isEnvTeardown() { return is_env_teardown; } inline v8::Local context() const { return v8impl::PersistentToLocal::Strong(context_persistent); diff --git a/src/node_api.cc b/src/node_api.cc index 8dbf48d466dfe1..33a1d89378e4d9 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -39,13 +39,21 @@ struct node_napi_env__ : public napi_env__ { void CallFinalizer(napi_finalize cb, void* data, void* hint) override { napi_env env = static_cast(this); - node_env()->SetImmediate([=](node::Environment* node_env) { + if (!env->isEnvTeardown()) { + node_env()->SetImmediate([=](node::Environment* node_env) { + v8::HandleScope handle_scope(env->isolate); + v8::Context::Scope context_scope(env->context()); + env->CallIntoModule([&](napi_env env) { + cb(env, data, hint); + }); + }); + } else { v8::HandleScope handle_scope(env->isolate); v8::Context::Scope context_scope(env->context()); env->CallIntoModule([&](napi_env env) { cb(env, data, hint); }); - }); + } } const char* GetFilename() const { return filename.c_str(); } From d39704b57121ce832255fa1be1017714411a62d6 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Sat, 1 May 2021 17:27:34 -0400 Subject: [PATCH 2/6] squash: add other place Finalization is deferred Signed-off-by: Michael Dawson --- src/node_api.cc | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index 33a1d89378e4d9..a4b193689358b5 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -77,8 +77,22 @@ class BufferFinalizer : private Finalizer { node::Environment* node_env = static_cast(finalizer->_env)->node_env(); - node_env->SetImmediate( - [finalizer = std::move(finalizer)](node::Environment* env) { + if (!finalizer->_env->isEnvTeardown()) { + node_env->SetImmediate( + [finalizer = std::move(finalizer)](node::Environment* env) { + if (finalizer->_finalize_callback == nullptr) return; + + v8::HandleScope handle_scope(finalizer->_env->isolate); + v8::Context::Scope context_scope(finalizer->_env->context()); + + finalizer->_env->CallIntoModule([&](napi_env env) { + finalizer->_finalize_callback( + env, + finalizer->_finalize_data, + finalizer->_finalize_hint); + }); + }); + } else { if (finalizer->_finalize_callback == nullptr) return; v8::HandleScope handle_scope(finalizer->_env->isolate); @@ -89,8 +103,8 @@ class BufferFinalizer : private Finalizer { env, finalizer->_finalize_data, finalizer->_finalize_hint); - }); - }); + }); + } } struct Deleter { From 403391496963b1a40d2ad6d974397c6dc7731bcf Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Sun, 2 May 2021 09:57:29 -0400 Subject: [PATCH 3/6] squash: ref/unref when using SetImmediate --- src/node_api.cc | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index a4b193689358b5..8a26d08ec8a623 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -40,12 +40,16 @@ struct node_napi_env__ : public napi_env__ { void CallFinalizer(napi_finalize cb, void* data, void* hint) override { napi_env env = static_cast(this); if (!env->isEnvTeardown()) { + env->Ref(); node_env()->SetImmediate([=](node::Environment* node_env) { - v8::HandleScope handle_scope(env->isolate); - v8::Context::Scope context_scope(env->context()); - env->CallIntoModule([&](napi_env env) { - cb(env, data, hint); - }); + { + v8::HandleScope handle_scope(env->isolate); + v8::Context::Scope context_scope(env->context()); + env->CallIntoModule([&](napi_env env) { + cb(env, data, hint); + }); + } + env->Unref(); }); } else { v8::HandleScope handle_scope(env->isolate); @@ -78,19 +82,27 @@ class BufferFinalizer : private Finalizer { node::Environment* node_env = static_cast(finalizer->_env)->node_env(); if (!finalizer->_env->isEnvTeardown()) { + finalizer->_env->Ref(); node_env->SetImmediate( [finalizer = std::move(finalizer)](node::Environment* env) { - if (finalizer->_finalize_callback == nullptr) return; - v8::HandleScope handle_scope(finalizer->_env->isolate); - v8::Context::Scope context_scope(finalizer->_env->context()); + if (finalizer->_finalize_callback == nullptr) { + finalizer->_env->Unref(); + return; + } - finalizer->_env->CallIntoModule([&](napi_env env) { - finalizer->_finalize_callback( - env, - finalizer->_finalize_data, - finalizer->_finalize_hint); - }); + { + v8::HandleScope handle_scope(finalizer->_env->isolate); + v8::Context::Scope context_scope(finalizer->_env->context()); + + finalizer->_env->CallIntoModule([&](napi_env env) { + finalizer->_finalize_callback( + env, + finalizer->_finalize_data, + finalizer->_finalize_hint); + }); + } + finalizer->_env->Unref(); }); } else { if (finalizer->_finalize_callback == nullptr) return; From 0eeee2fcf0e087e86e80dbe641640cb7ea8394b7 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Sun, 2 May 2021 10:01:14 -0400 Subject: [PATCH 4/6] squash: fixup spacing --- src/node_api.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index 8a26d08ec8a623..3e6736682f1974 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -48,7 +48,7 @@ struct node_napi_env__ : public napi_env__ { env->CallIntoModule([&](napi_env env) { cb(env, data, hint); }); - } + } env->Unref(); }); } else { @@ -89,10 +89,10 @@ class BufferFinalizer : private Finalizer { if (finalizer->_finalize_callback == nullptr) { finalizer->_env->Unref(); return; - } + } - { - v8::HandleScope handle_scope(finalizer->_env->isolate); + { + v8::HandleScope handle_scope(finalizer->_env->isolate); v8::Context::Scope context_scope(finalizer->_env->context()); finalizer->_env->CallIntoModule([&](napi_env env) { @@ -101,7 +101,7 @@ class BufferFinalizer : private Finalizer { finalizer->_finalize_data, finalizer->_finalize_hint); }); - } + } finalizer->_env->Unref(); }); } else { From f42e1f48671e41158e0e37f5abaf28ab67428892 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Fri, 7 May 2021 16:02:49 -0400 Subject: [PATCH 5/6] squash: refactor - minimal change - Refactor to limit changes needed to address crashses seen. - use exception safe approach to Ref/Unref env Signed-off-by: Michael Dawson --- src/js_native_api_v8.h | 38 +++++++++++++++++++++++++----- src/node_api.cc | 53 ++++++++++-------------------------------- 2 files changed, 44 insertions(+), 47 deletions(-) diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 127f84844ef6dd..30f1d47a43dd93 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -54,8 +54,7 @@ class RefTracker { struct napi_env__ { explicit napi_env__(v8::Local context) : isolate(context->GetIsolate()), - context_persistent(isolate, context), - is_env_teardown(false) { + context_persistent(isolate, context) { CHECK_EQ(isolate, context->GetIsolate()); } virtual ~napi_env__() { @@ -64,15 +63,11 @@ struct napi_env__ { // they delete during their `napi_finalizer` callbacks. If we deleted such // references here first, they would be doubly deleted when the // `napi_finalizer` deleted them subsequently. - is_env_teardown = true; v8impl::RefTracker::FinalizeAll(&finalizing_reflist); v8impl::RefTracker::FinalizeAll(&reflist); } v8::Isolate* const isolate; // Shortcut for context()->GetIsolate() v8impl::Persistent context_persistent; - bool is_env_teardown; - - inline bool isEnvTeardown() { return is_env_teardown; } inline v8::Local context() const { return v8impl::PersistentToLocal::Strong(context_persistent); @@ -127,6 +122,37 @@ struct napi_env__ { void* instance_data = nullptr; }; +// This class is used to keep a napi_env live in a way that +// is exception safe versus calling Ref/Unref directly +class LiveEnv { + public: + explicit LiveEnv(napi_env env) : _env(env) { + _env->Ref(); + } + + explicit LiveEnv(const LiveEnv& other): _env(other.env()) { + _env->Ref(); + } + + LiveEnv(LiveEnv&& other) { + _env = other._env; + other._env = nullptr; + } + + ~LiveEnv() { + if (_env != nullptr) { + _env->Unref(); + } + } + + napi_env env(void) const { + return _env; + } + + private: + napi_env _env; +}; + static inline napi_status napi_clear_last_error(napi_env env) { env->last_error.error_code = napi_ok; diff --git a/src/node_api.cc b/src/node_api.cc index 3e6736682f1974..4287b00375a711 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -38,26 +38,19 @@ struct node_napi_env__ : public napi_env__ { } void CallFinalizer(napi_finalize cb, void* data, void* hint) override { - napi_env env = static_cast(this); - if (!env->isEnvTeardown()) { - env->Ref(); - node_env()->SetImmediate([=](node::Environment* node_env) { - { - v8::HandleScope handle_scope(env->isolate); - v8::Context::Scope context_scope(env->context()); - env->CallIntoModule([&](napi_env env) { - cb(env, data, hint); - }); - } - env->Unref(); - }); - } else { + // we need to keep the env live until the finalizer has been run + // LiveEnv provides an exception safe wrapper to Ref and then + // Unref once the lamba is freed + LiveEnv liveEnv(static_cast(this)); + node_env()->SetImmediate([=, liveEnv = std::move(liveEnv)] + (node::Environment* node_env) { + napi_env env = liveEnv.env(); v8::HandleScope handle_scope(env->isolate); v8::Context::Scope context_scope(env->context()); env->CallIntoModule([&](napi_env env) { cb(env, data, hint); }); - } + }); } const char* GetFilename() const { return filename.c_str(); } @@ -81,30 +74,8 @@ class BufferFinalizer : private Finalizer { node::Environment* node_env = static_cast(finalizer->_env)->node_env(); - if (!finalizer->_env->isEnvTeardown()) { - finalizer->_env->Ref(); - node_env->SetImmediate( - [finalizer = std::move(finalizer)](node::Environment* env) { - - if (finalizer->_finalize_callback == nullptr) { - finalizer->_env->Unref(); - return; - } - - { - v8::HandleScope handle_scope(finalizer->_env->isolate); - v8::Context::Scope context_scope(finalizer->_env->context()); - - finalizer->_env->CallIntoModule([&](napi_env env) { - finalizer->_finalize_callback( - env, - finalizer->_finalize_data, - finalizer->_finalize_hint); - }); - } - finalizer->_env->Unref(); - }); - } else { + node_env->SetImmediate( + [finalizer = std::move(finalizer)](node::Environment* env) { if (finalizer->_finalize_callback == nullptr) return; v8::HandleScope handle_scope(finalizer->_env->isolate); @@ -115,8 +86,8 @@ class BufferFinalizer : private Finalizer { env, finalizer->_finalize_data, finalizer->_finalize_hint); - }); - } + }); + }); } struct Deleter { From 558a765f3dad635f8ec61f63d121848d3cd8264f Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Thu, 13 May 2021 14:20:12 -0400 Subject: [PATCH 6/6] squash: rename to EnvRefHolder Signed-off-by: Michael Dawson --- src/js_native_api_v8.h | 10 +++++----- src/node_api.cc | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 30f1d47a43dd93..f619248a84326a 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -124,22 +124,22 @@ struct napi_env__ { // This class is used to keep a napi_env live in a way that // is exception safe versus calling Ref/Unref directly -class LiveEnv { +class EnvRefHolder { public: - explicit LiveEnv(napi_env env) : _env(env) { + explicit EnvRefHolder(napi_env env) : _env(env) { _env->Ref(); } - explicit LiveEnv(const LiveEnv& other): _env(other.env()) { + explicit EnvRefHolder(const EnvRefHolder& other): _env(other.env()) { _env->Ref(); } - LiveEnv(LiveEnv&& other) { + EnvRefHolder(EnvRefHolder&& other) { _env = other._env; other._env = nullptr; } - ~LiveEnv() { + ~EnvRefHolder() { if (_env != nullptr) { _env->Unref(); } diff --git a/src/node_api.cc b/src/node_api.cc index 4287b00375a711..b3e39d03b8ade8 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -39,9 +39,9 @@ struct node_napi_env__ : public napi_env__ { void CallFinalizer(napi_finalize cb, void* data, void* hint) override { // we need to keep the env live until the finalizer has been run - // LiveEnv provides an exception safe wrapper to Ref and then + // EnvRefHolder provides an exception safe wrapper to Ref and then // Unref once the lamba is freed - LiveEnv liveEnv(static_cast(this)); + EnvRefHolder liveEnv(static_cast(this)); node_env()->SetImmediate([=, liveEnv = std::move(liveEnv)] (node::Environment* node_env) { napi_env env = liveEnv.env();