node-api: avoid calling virtual methods in base's dtor · nodejs/node@f38987e · GitHub

nodejs / node Public

• Notifications You must be signed in to change notification settings
• Fork 36.6k
• Star 120k

• Code
• Issues 635
• Pull requests 755
• Actions
• Projects
• Security and quality
• Insights

Additional navigation options

Commit f38987e

Browse filesBrowse the repository at this point in the historyBrowse files

legendecas

authored and

juanarbol

committed

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

1 parent c7713f1 commit f38987e
Copy full SHA for f38987e

3 files changed

+15-8Lines changed: 15 additions & 8 deletions

File tree

Expand file treeCollapse file tree

Open diff view settings

Filter options

• src
  • js_native_api_v8.h
  • node_api.cc
  • node_api_internals.h

Expand file treeCollapse file tree

Open diff view settings

Collapse file

‎src/js_native_api_v8.h‎

Copy file name to clipboardExpand all lines: src/js_native_api_v8.h

+11-5Lines changed: 11 additions & 5 deletions

Original file line number	Diff line number	Diff line change
@@ -55,17 +55,14 @@ struct napi_env__ {
55	55	CHECK_EQ(isolate, context->GetIsolate());
56	56	napi_clear_last_error(this);
57	57	}
58		- virtual ~napi_env__() { FinalizeAll(); }
59		- v8::Isolate* const isolate; // Shortcut for context()->GetIsolate()
60		- v8impl::Persistent<v8::Context> context_persistent;
61	58
62	59	inline v8::Local<v8::Context> context() const {
63	60	return v8impl::PersistentToLocal::Strong(context_persistent);
64	61	}
65	62
66	63	inline void Ref() { refs++; }
67	64	inline void Unref() {
68		- if (--refs == 0) delete this;
	65	+ if (--refs == 0) DeleteMe();
69	66	}
70	67
71	68	virtual bool can_call_into_js() const { return true; }
@@ -99,16 +96,20 @@ struct napi_env__ {
99	96	CallIntoModule([&](napi_env env) { cb(env, data, hint); });
100	97	}
101	98
102		- void FinalizeAll() {
	99	+ virtual void DeleteMe() {
103	100	// First we must finalize those references that have `napi_finalizer`
104	101	// callbacks. The reason is that addons might store other references which
105	102	// they delete during their `napi_finalizer` callbacks. If we deleted such
106	103	// references here first, they would be doubly deleted when the
107	104	// `napi_finalizer` deleted them subsequently.
108	105	v8impl::RefTracker::FinalizeAll(&finalizing_reflist);
109	106	v8impl::RefTracker::FinalizeAll(&reflist);
	107	+ delete this;
110	108	}
111	109
	110	+ v8::Isolate* const isolate; // Shortcut for context()->GetIsolate()
	111	+ v8impl::Persistent<v8::Context> context_persistent;
	112	+
112	113	v8impl::Persistent<v8::Value> last_exception;
113	114
114	115	// We store references in two different lists, depending on whether they have
@@ -121,6 +122,11 @@ struct napi_env__ {
121	122	int open_callback_scopes = 0;
122	123	int refs = 1;
123	124	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;
124	130	};
125	131
126	132	// This class is used to keep a napi_env live in a way that

Collapse file

‎src/node_api.cc‎

Copy file name to clipboardExpand all lines: src/node_api.cc

+2-2Lines changed: 2 additions & 2 deletions

Original file line number	Diff line number	Diff line change
@@ -25,9 +25,9 @@ node_napi_env__::node_napi_env__(v8::Local<v8::Context> context,
25	25	CHECK_NOT_NULL(node_env());
26	26	}
27	27
28		- node_napi_env__::~node_napi_env__() {
	28	+ void node_napi_env__::DeleteMe() {
29	29	destructing = true;
30		- FinalizeAll();
	30	+ napi_env__::DeleteMe();
31	31	}
32	32
33	33	bool node_napi_env__::can_call_into_js() const {

Collapse file

‎src/node_api_internals.h‎

Copy file name to clipboardExpand all lines: src/node_api_internals.h

+2-1Lines changed: 2 additions & 1 deletion

Original file line number	Diff line number	Diff line change
@@ -11,7 +11,6 @@
11	11	struct node_napi_env__ : public napi_env__ {
12	12	node_napi_env__(v8::Local<v8::Context> context,
13	13	const std::string& module_filename);
14		- ~node_napi_env__();
15	14
16	15	bool can_call_into_js() const override;
17	16	v8::Maybe<bool> mark_arraybuffer_as_untransferable(
@@ -24,6 +23,8 @@ struct node_napi_env__ : public napi_env__ {
24	23	template <bool enforceUncaughtExceptionPolicy, typename T>
25	24	void CallbackIntoModule(T&& call);
26	25
	26	+ void DeleteMe() override;
	27	+
27	28	inline node::Environment* node_env() const {
28	29	return node::Environment::GetCurrent(context());
29	30	}

0 commit comments

Comments

0 (0)