Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

node-api: avoid calling virtual methods in base's dtor #44424

Merged
merged 1 commit into from
Aug 31, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions src/js_native_api_v8.h
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
3 changes: 2 additions & 1 deletion src/node_api_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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());
}
Expand Down