From bfe0424bc203aaa5490926449a0b6bc8786108f3 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Wed, 31 Aug 2022 21:47:10 +0800 Subject: [PATCH] node-api: avoid calling virtual methods in base's dtor Derived classes' fields are already destroyed if the virtual methods are invoked in the base class's destructor. It is not safe to call virtual methods in base's dtor. PR-URL: https://github.com/nodejs/node/pull/44424 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Minwoo Jung --- src/js_native_api_v8.h | 16 +++++++++++----- src/node_api.cc | 4 ++-- src/node_api_internals.h | 3 ++- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 8e9a06ed894362..766398744c5dfb 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -55,9 +55,6 @@ struct napi_env__ { CHECK_EQ(isolate, context->GetIsolate()); napi_clear_last_error(this); } - virtual ~napi_env__() { FinalizeAll(); } - v8::Isolate* const isolate; // Shortcut for context()->GetIsolate() - v8impl::Persistent context_persistent; inline v8::Local context() const { return v8impl::PersistentToLocal::Strong(context_persistent); @@ -65,7 +62,7 @@ struct napi_env__ { inline void Ref() { refs++; } inline void Unref() { - if (--refs == 0) delete this; + if (--refs == 0) DeleteMe(); } virtual bool can_call_into_js() const { return true; } @@ -99,7 +96,7 @@ struct napi_env__ { CallIntoModule([&](napi_env env) { cb(env, data, hint); }); } - void FinalizeAll() { + virtual void DeleteMe() { // First we must finalize those references that have `napi_finalizer` // callbacks. The reason is that addons might store other references which // they delete during their `napi_finalizer` callbacks. If we deleted such @@ -107,8 +104,12 @@ struct napi_env__ { // `napi_finalizer` deleted them subsequently. v8impl::RefTracker::FinalizeAll(&finalizing_reflist); v8impl::RefTracker::FinalizeAll(&reflist); + delete this; } + v8::Isolate* const isolate; // Shortcut for context()->GetIsolate() + v8impl::Persistent context_persistent; + v8impl::Persistent last_exception; // We store references in two different lists, depending on whether they have @@ -121,6 +122,11 @@ struct napi_env__ { int open_callback_scopes = 0; int refs = 1; void* instance_data = nullptr; + + protected: + // Should not be deleted directly. Delete with `napi_env__::DeleteMe()` + // instead. + virtual ~napi_env__() = default; }; // This class is used to keep a napi_env live in a way that diff --git a/src/node_api.cc b/src/node_api.cc index dfe27f43734238..922d6ffac1927b 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -25,9 +25,9 @@ node_napi_env__::node_napi_env__(v8::Local context, CHECK_NOT_NULL(node_env()); } -node_napi_env__::~node_napi_env__() { +void node_napi_env__::DeleteMe() { destructing = true; - FinalizeAll(); + napi_env__::DeleteMe(); } bool node_napi_env__::can_call_into_js() const { diff --git a/src/node_api_internals.h b/src/node_api_internals.h index 6478520fd55da6..de5d9dc0804367 100644 --- a/src/node_api_internals.h +++ b/src/node_api_internals.h @@ -11,7 +11,6 @@ struct node_napi_env__ : public napi_env__ { node_napi_env__(v8::Local context, const std::string& module_filename); - ~node_napi_env__(); bool can_call_into_js() const override; v8::Maybe mark_arraybuffer_as_untransferable( @@ -24,6 +23,8 @@ struct node_napi_env__ : public napi_env__ { template void CallbackIntoModule(T&& call); + void DeleteMe() override; + inline node::Environment* node_env() const { return node::Environment::GetCurrent(context()); }