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: #44424 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
nodejs · Sep 7, 2022 · ef08cbd · ef08cbd
1 parent fd611cc
commit ef08cbd
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 8 deletions.
diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h
@@ -55,17 +55,14 @@ 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<v8::Context> context_persistent;
 
   inline v8::Local<v8::Context> context() const {
     return v8impl::PersistentToLocal::Strong(context_persistent);
   }
 
   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,16 +96,20 @@ 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
     // references here first, they would be doubly deleted when the
     // `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<v8::Context> context_persistent;
+
   v8impl::Persistent<v8::Value> 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
@@ -25,9 +25,9 @@ node_napi_env__::node_napi_env__(v8::Local<v8::Context> 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
@@ -11,7 +11,6 @@
 struct node_napi_env__ : public napi_env__ {
   node_napi_env__(v8::Local<v8::Context> context,
                   const std::string& module_filename);
-  ~node_napi_env__();
 
   bool can_call_into_js() const override;
   v8::Maybe<bool> mark_arraybuffer_as_untransferable(
@@ -24,6 +23,8 @@ struct node_napi_env__ : public napi_env__ {
   template <bool enforceUncaughtExceptionPolicy, typename T>
   void CallbackIntoModule(T&& call);
 
+  void DeleteMe() override;
+
   inline node::Environment* node_env() const {
     return node::Environment::GetCurrent(context());
   }