Skip to content

Commit f38987e

Browse files
legendecasjuanarbol
authored andcommitted
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]>
1 parent c7713f1 commit f38987e

File tree

3 files changed

+15
-8
lines changed

3 files changed

+15
-8
lines changed

src/js_native_api_v8.h

+11-5
Original file line numberDiff line numberDiff line change
@@ -55,17 +55,14 @@ struct napi_env__ {
5555
CHECK_EQ(isolate, context->GetIsolate());
5656
napi_clear_last_error(this);
5757
}
58-
virtual ~napi_env__() { FinalizeAll(); }
59-
v8::Isolate* const isolate; // Shortcut for context()->GetIsolate()
60-
v8impl::Persistent<v8::Context> context_persistent;
6158

6259
inline v8::Local<v8::Context> context() const {
6360
return v8impl::PersistentToLocal::Strong(context_persistent);
6461
}
6562

6663
inline void Ref() { refs++; }
6764
inline void Unref() {
68-
if (--refs == 0) delete this;
65+
if (--refs == 0) DeleteMe();
6966
}
7067

7168
virtual bool can_call_into_js() const { return true; }
@@ -99,16 +96,20 @@ struct napi_env__ {
9996
CallIntoModule([&](napi_env env) { cb(env, data, hint); });
10097
}
10198

102-
void FinalizeAll() {
99+
virtual void DeleteMe() {
103100
// First we must finalize those references that have `napi_finalizer`
104101
// callbacks. The reason is that addons might store other references which
105102
// they delete during their `napi_finalizer` callbacks. If we deleted such
106103
// references here first, they would be doubly deleted when the
107104
// `napi_finalizer` deleted them subsequently.
108105
v8impl::RefTracker::FinalizeAll(&finalizing_reflist);
109106
v8impl::RefTracker::FinalizeAll(&reflist);
107+
delete this;
110108
}
111109

110+
v8::Isolate* const isolate; // Shortcut for context()->GetIsolate()
111+
v8impl::Persistent<v8::Context> context_persistent;
112+
112113
v8impl::Persistent<v8::Value> last_exception;
113114

114115
// We store references in two different lists, depending on whether they have
@@ -121,6 +122,11 @@ struct napi_env__ {
121122
int open_callback_scopes = 0;
122123
int refs = 1;
123124
void* instance_data = nullptr;
125+
126+
protected:
127+
// Should not be deleted directly. Delete with `napi_env__::DeleteMe()`
128+
// instead.
129+
virtual ~napi_env__() = default;
124130
};
125131

126132
// This class is used to keep a napi_env live in a way that

src/node_api.cc

+2-2
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,9 @@ node_napi_env__::node_napi_env__(v8::Local<v8::Context> context,
2525
CHECK_NOT_NULL(node_env());
2626
}
2727

28-
node_napi_env__::~node_napi_env__() {
28+
void node_napi_env__::DeleteMe() {
2929
destructing = true;
30-
FinalizeAll();
30+
napi_env__::DeleteMe();
3131
}
3232

3333
bool node_napi_env__::can_call_into_js() const {

src/node_api_internals.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
struct node_napi_env__ : public napi_env__ {
1212
node_napi_env__(v8::Local<v8::Context> context,
1313
const std::string& module_filename);
14-
~node_napi_env__();
1514

1615
bool can_call_into_js() const override;
1716
v8::Maybe<bool> mark_arraybuffer_as_untransferable(
@@ -24,6 +23,8 @@ struct node_napi_env__ : public napi_env__ {
2423
template <bool enforceUncaughtExceptionPolicy, typename T>
2524
void CallbackIntoModule(T&& call);
2625

26+
void DeleteMe() override;
27+
2728
inline node::Environment* node_env() const {
2829
return node::Environment::GetCurrent(context());
2930
}

0 commit comments

Comments
 (0)