Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit f5d4253

Browse filesBrowse files
addaleaxMylesBorins
addaleax
authored and
MylesBorins
committed
src: refactor BaseObject internal field management
- Instead of storing a pointer whose type refers to the specific subclass of `BaseObject`, just store a `BaseObject*` directly. This means in particular that one can cast to classes along the way of the inheritance chain without issues, and that `BaseObject*` no longer needs to be the first superclass in the case of multiple inheritance. In particular, this renders hack-y solutions to this problem (like ddc19be) obsolete and addresses a `TODO` comment of mine. - Move wrapping/unwrapping methods to the `BaseObject` class. We use these almost exclusively for `BaseObject`s, and I hope that this gives a better idea of how (and for what) these are used in our code. - Perform initialization/deinitialization of the internal field in the `BaseObject*` constructor/destructor. This makes the code a bit more obviously correct, avoids explicit calls for this in subclass constructors, and in particular allows us to avoid crash situations when we previously called `ClearWrap()` during GC. This also means that we enforce that the object passed to the `BaseObject` constructor needs to have an internal field. This is the only reason for the test change. - Change the signature of `MakeWeak()` to not require a pointer argument. Previously, this would always have been the same as `this`, and no other value made sense. Also, the parameter was something that I personally found somewhat confusing when becoming familiar with Node’s code. - Add a `TODO` comment that motivates switching to real inheritance for the JS types we expose from the native side. This patch brings us a lot closer to being able to do that. - Some less significant drive-by cleanup. Since we *effectively* already store the `BaseObject*` pointer anyway since ddc19be, I do not think that this is going to have any impact on diagnostic tooling. Fixes: #18897 PR-URL: #20455 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
1 parent c21a52f commit f5d4253
Copy full SHA for f5d4253
Expand file treeCollapse file tree

40 files changed

+147
-244
lines changed
Open diff view settings
Collapse file

‎src/async_wrap.cc‎

Copy file name to clipboardExpand all lines: src/async_wrap.cc
+3-3Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -123,8 +123,8 @@ RetainedObjectInfo* WrapperInfo(uint16_t class_id, Local<Value> wrapper) {
123123
Local<Object> object = wrapper.As<Object>();
124124
CHECK_GT(object->InternalFieldCount(), 0);
125125

126-
AsyncWrap* wrap = Unwrap<AsyncWrap>(object);
127-
if (wrap == nullptr) return nullptr; // ClearWrap() already called.
126+
AsyncWrap* wrap;
127+
ASSIGN_OR_RETURN_UNWRAP(&wrap, object, nullptr);
128128

129129
return new RetainedAsyncInfo(class_id, wrap);
130130
}
@@ -231,7 +231,7 @@ class PromiseWrap : public AsyncWrap {
231231
public:
232232
PromiseWrap(Environment* env, Local<Object> object, bool silent)
233233
: AsyncWrap(env, object, PROVIDER_PROMISE, -1, silent) {
234-
MakeWeak(this);
234+
MakeWeak();
235235
}
236236
size_t self_size() const override { return sizeof(*this); }
237237

Collapse file

‎src/base_object-inl.h‎

Copy file name to clipboardExpand all lines: src/base_object-inl.h
+57-21Lines changed: 57 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -31,54 +31,90 @@
3131

3232
namespace node {
3333

34-
inline BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> handle)
34+
BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> handle)
3535
: persistent_handle_(env->isolate(), handle),
3636
env_(env) {
3737
CHECK_EQ(false, handle.IsEmpty());
38-
// The zero field holds a pointer to the handle. Immediately set it to
39-
// nullptr in case it's accessed by the user before construction is complete.
40-
if (handle->InternalFieldCount() > 0)
41-
handle->SetAlignedPointerInInternalField(0, nullptr);
38+
CHECK_GT(handle->InternalFieldCount(), 0);
39+
handle->SetAlignedPointerInInternalField(0, static_cast<void*>(this));
4240
}
4341

4442

45-
inline Persistent<v8::Object>& BaseObject::persistent() {
43+
BaseObject::~BaseObject() {
44+
if (persistent_handle_.IsEmpty()) {
45+
// This most likely happened because the weak callback below cleared it.
46+
return;
47+
}
48+
49+
{
50+
v8::HandleScope handle_scope(env_->isolate());
51+
object()->SetAlignedPointerInInternalField(0, nullptr);
52+
}
53+
}
54+
55+
56+
Persistent<v8::Object>& BaseObject::persistent() {
4657
return persistent_handle_;
4758
}
4859

4960

50-
inline v8::Local<v8::Object> BaseObject::object() {
61+
v8::Local<v8::Object> BaseObject::object() {
5162
return PersistentToLocal(env_->isolate(), persistent_handle_);
5263
}
5364

5465

55-
inline Environment* BaseObject::env() const {
66+
Environment* BaseObject::env() const {
5667
return env_;
5768
}
5869

5970

60-
template <typename Type>
61-
inline void BaseObject::WeakCallback(
62-
const v8::WeakCallbackInfo<Type>& data) {
63-
delete data.GetParameter();
71+
BaseObject* BaseObject::FromJSObject(v8::Local<v8::Object> obj) {
72+
CHECK_GT(obj->InternalFieldCount(), 0);
73+
return static_cast<BaseObject*>(obj->GetAlignedPointerFromInternalField(0));
6474
}
6575

6676

67-
template <typename Type>
68-
inline void BaseObject::MakeWeak(Type* ptr) {
69-
v8::HandleScope scope(env_->isolate());
70-
v8::Local<v8::Object> handle = object();
71-
CHECK_GT(handle->InternalFieldCount(), 0);
72-
Wrap(handle, ptr);
73-
persistent_handle_.SetWeak<Type>(ptr, WeakCallback<Type>,
74-
v8::WeakCallbackType::kParameter);
77+
template <typename T>
78+
T* BaseObject::FromJSObject(v8::Local<v8::Object> object) {
79+
return static_cast<T*>(FromJSObject(object));
7580
}
7681

7782

78-
inline void BaseObject::ClearWeak() {
83+
void BaseObject::MakeWeak() {
84+
persistent_handle_.SetWeak(
85+
this,
86+
[](const v8::WeakCallbackInfo<BaseObject>& data) {
87+
BaseObject* obj = data.GetParameter();
88+
// Clear the persistent handle so that ~BaseObject() doesn't attempt
89+
// to mess with internal fields, since the JS object may have
90+
// transitioned into an invalid state.
91+
// Refs: https://github.com/nodejs/node/issues/18897
92+
obj->persistent_handle_.Reset();
93+
delete obj;
94+
}, v8::WeakCallbackType::kParameter);
95+
}
96+
97+
98+
void BaseObject::ClearWeak() {
7999
persistent_handle_.ClearWeak();
80100
}
81101

102+
103+
v8::Local<v8::FunctionTemplate>
104+
BaseObject::MakeLazilyInitializedJSTemplate(Environment* env) {
105+
auto constructor = [](const v8::FunctionCallbackInfo<v8::Value>& args) {
106+
#ifdef DEBUG
107+
CHECK(args.IsConstructCall());
108+
CHECK_GT(args.This()->InternalFieldCount(), 0);
109+
#endif
110+
args.This()->SetAlignedPointerInInternalField(0, nullptr);
111+
};
112+
113+
v8::Local<v8::FunctionTemplate> t = env->NewFunctionTemplate(constructor);
114+
t->InstanceTemplate()->SetInternalFieldCount(1);
115+
return t;
116+
}
117+
82118
} // namespace node
83119

84120
#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
Collapse file

‎src/base_object.h‎

Copy file name to clipboardExpand all lines: src/base_object.h
+38-12Lines changed: 38 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -26,15 +26,18 @@
2626

2727
#include "node_persistent.h"
2828
#include "v8.h"
29+
#include <type_traits> // std::remove_reference
2930

3031
namespace node {
3132

3233
class Environment;
3334

3435
class BaseObject {
3536
public:
37+
// Associates this object with `handle`. It uses the 0th internal field for
38+
// that, and in particular aborts if there is no such field.
3639
inline BaseObject(Environment* env, v8::Local<v8::Object> handle);
37-
virtual ~BaseObject() = default;
40+
virtual inline ~BaseObject();
3841

3942
// Returns the wrapped object. Returns an empty handle when
4043
// persistent.IsEmpty() is true.
@@ -44,23 +47,30 @@ class BaseObject {
4447

4548
inline Environment* env() const;
4649

47-
// The handle_ must have an internal field count > 0, and the first
48-
// index is reserved for a pointer to this class. This is an
49-
// implicit requirement, but Node does not have a case where it's
50-
// required that MakeWeak() be called and the internal field not
51-
// be set.
52-
template <typename Type>
53-
inline void MakeWeak(Type* ptr);
50+
// Get a BaseObject* pointer, or subclass pointer, for the JS object that
51+
// was also passed to the `BaseObject()` constructor initially.
52+
// This may return `nullptr` if the C++ object has not been constructed yet,
53+
// e.g. when the JS object used `MakeLazilyInitializedJSTemplate`.
54+
static inline BaseObject* FromJSObject(v8::Local<v8::Object> object);
55+
template <typename T>
56+
static inline T* FromJSObject(v8::Local<v8::Object> object);
5457

58+
// Make the `Persistent` a weak reference and, `delete` this object once
59+
// the JS object has been garbage collected.
60+
inline void MakeWeak();
61+
62+
// Undo `MakeWeak()`, i.e. turn this into a strong reference.
5563
inline void ClearWeak();
5664

65+
// Utility to create a FunctionTemplate with one internal field (used for
66+
// the `BaseObject*` pointer) and a constructor that initializes that field
67+
// to `nullptr`.
68+
static inline v8::Local<v8::FunctionTemplate> MakeLazilyInitializedJSTemplate(
69+
Environment* env);
70+
5771
private:
5872
BaseObject();
5973

60-
template <typename Type>
61-
static inline void WeakCallback(
62-
const v8::WeakCallbackInfo<Type>& data);
63-
6474
// persistent_handle_ needs to be at a fixed offset from the start of the
6575
// class because it is used by src/node_postmortem_metadata.cc to calculate
6676
// offsets and generate debug symbols for BaseObject, which assumes that the
@@ -71,6 +81,22 @@ class BaseObject {
7181
Environment* env_;
7282
};
7383

84+
85+
// Global alias for FromJSObject() to avoid churn.
86+
template <typename T>
87+
inline T* Unwrap(v8::Local<v8::Object> obj) {
88+
return BaseObject::FromJSObject<T>(obj);
89+
}
90+
91+
92+
#define ASSIGN_OR_RETURN_UNWRAP(ptr, obj, ...) \
93+
do { \
94+
*ptr = static_cast<typename std::remove_reference<decltype(*ptr)>::type>( \
95+
BaseObject::FromJSObject(obj)); \
96+
if (*ptr == nullptr) \
97+
return __VA_ARGS__; \
98+
} while (0)
99+
74100
} // namespace node
75101

76102
#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
Collapse file

‎src/cares_wrap.cc‎

Copy file name to clipboardExpand all lines: src/cares_wrap.cc
+4-27Lines changed: 4 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ ChannelWrap::ChannelWrap(Environment* env,
187187
is_servers_default_(true),
188188
library_inited_(false),
189189
active_query_count_(0) {
190-
MakeWeak<ChannelWrap>(this);
190+
MakeWeak();
191191

192192
Setup();
193193
}
@@ -205,7 +205,6 @@ class GetAddrInfoReqWrap : public ReqWrap<uv_getaddrinfo_t> {
205205
GetAddrInfoReqWrap(Environment* env,
206206
Local<Object> req_wrap_obj,
207207
bool verbatim);
208-
~GetAddrInfoReqWrap();
209208

210209
size_t self_size() const override { return sizeof(*this); }
211210
bool verbatim() const { return verbatim_; }
@@ -219,30 +218,19 @@ GetAddrInfoReqWrap::GetAddrInfoReqWrap(Environment* env,
219218
bool verbatim)
220219
: ReqWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_GETADDRINFOREQWRAP)
221220
, verbatim_(verbatim) {
222-
Wrap(req_wrap_obj, this);
223-
}
224-
225-
GetAddrInfoReqWrap::~GetAddrInfoReqWrap() {
226-
ClearWrap(object());
227221
}
228222

229223

230224
class GetNameInfoReqWrap : public ReqWrap<uv_getnameinfo_t> {
231225
public:
232226
GetNameInfoReqWrap(Environment* env, Local<Object> req_wrap_obj);
233-
~GetNameInfoReqWrap();
234227

235228
size_t self_size() const override { return sizeof(*this); }
236229
};
237230

238231
GetNameInfoReqWrap::GetNameInfoReqWrap(Environment* env,
239232
Local<Object> req_wrap_obj)
240233
: ReqWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_GETNAMEINFOREQWRAP) {
241-
Wrap(req_wrap_obj, this);
242-
}
243-
244-
GetNameInfoReqWrap::~GetNameInfoReqWrap() {
245-
ClearWrap(object());
246234
}
247235

248236

@@ -587,8 +575,6 @@ class QueryWrap : public AsyncWrap {
587575
QueryWrap(ChannelWrap* channel, Local<Object> req_wrap_obj)
588576
: AsyncWrap(channel->env(), req_wrap_obj, AsyncWrap::PROVIDER_QUERYWRAP),
589577
channel_(channel) {
590-
Wrap(req_wrap_obj, this);
591-
592578
// Make sure the channel object stays alive during the query lifetime.
593579
req_wrap_obj->Set(env()->context(),
594580
env()->channel_string(),
@@ -597,7 +583,6 @@ class QueryWrap : public AsyncWrap {
597583

598584
~QueryWrap() override {
599585
CHECK_EQ(false, persistent().IsEmpty());
600-
ClearWrap(object());
601586
}
602587

603588
// Subclasses should implement the appropriate Send method.
@@ -2143,32 +2128,24 @@ void Initialize(Local<Object> target,
21432128
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "AI_V4MAPPED"),
21442129
Integer::New(env->isolate(), AI_V4MAPPED));
21452130

2146-
auto is_construct_call_callback =
2147-
[](const FunctionCallbackInfo<Value>& args) {
2148-
CHECK(args.IsConstructCall());
2149-
ClearWrap(args.This());
2150-
};
21512131
Local<FunctionTemplate> aiw =
2152-
FunctionTemplate::New(env->isolate(), is_construct_call_callback);
2153-
aiw->InstanceTemplate()->SetInternalFieldCount(1);
2132+
BaseObject::MakeLazilyInitializedJSTemplate(env);
21542133
AsyncWrap::AddWrapMethods(env, aiw);
21552134
Local<String> addrInfoWrapString =
21562135
FIXED_ONE_BYTE_STRING(env->isolate(), "GetAddrInfoReqWrap");
21572136
aiw->SetClassName(addrInfoWrapString);
21582137
target->Set(addrInfoWrapString, aiw->GetFunction());
21592138

21602139
Local<FunctionTemplate> niw =
2161-
FunctionTemplate::New(env->isolate(), is_construct_call_callback);
2162-
niw->InstanceTemplate()->SetInternalFieldCount(1);
2140+
BaseObject::MakeLazilyInitializedJSTemplate(env);
21632141
AsyncWrap::AddWrapMethods(env, niw);
21642142
Local<String> nameInfoWrapString =
21652143
FIXED_ONE_BYTE_STRING(env->isolate(), "GetNameInfoReqWrap");
21662144
niw->SetClassName(nameInfoWrapString);
21672145
target->Set(nameInfoWrapString, niw->GetFunction());
21682146

21692147
Local<FunctionTemplate> qrw =
2170-
FunctionTemplate::New(env->isolate(), is_construct_call_callback);
2171-
qrw->InstanceTemplate()->SetInternalFieldCount(1);
2148+
BaseObject::MakeLazilyInitializedJSTemplate(env);
21722149
AsyncWrap::AddWrapMethods(env, qrw);
21732150
Local<String> queryWrapString =
21742151
FIXED_ONE_BYTE_STRING(env->isolate(), "QueryReqWrap");
Collapse file

‎src/connect_wrap.cc‎

Copy file name to clipboardExpand all lines: src/connect_wrap.cc
-6Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,6 @@ using v8::Object;
1313
ConnectWrap::ConnectWrap(Environment* env,
1414
Local<Object> req_wrap_obj,
1515
AsyncWrap::ProviderType provider) : ReqWrap(env, req_wrap_obj, provider) {
16-
Wrap(req_wrap_obj, this);
17-
}
18-
19-
20-
ConnectWrap::~ConnectWrap() {
21-
ClearWrap(object());
2216
}
2317

2418
} // namespace node
Collapse file

‎src/connect_wrap.h‎

Copy file name to clipboardExpand all lines: src/connect_wrap.h
-1Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ class ConnectWrap : public ReqWrap<uv_connect_t> {
1515
ConnectWrap(Environment* env,
1616
v8::Local<v8::Object> req_wrap_obj,
1717
AsyncWrap::ProviderType provider);
18-
~ConnectWrap();
1918

2019
size_t self_size() const override { return sizeof(*this); }
2120
};
Collapse file

‎src/connection_wrap.h‎

Copy file name to clipboardExpand all lines: src/connection_wrap.h
-1Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ class ConnectionWrap : public LibuvStreamWrap {
2929
UVType handle_;
3030
};
3131

32-
3332
} // namespace node
3433

3534
#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
Collapse file

‎src/fs_event_wrap.cc‎

Copy file name to clipboardExpand all lines: src/fs_event_wrap.cc
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ void FSEventWrap::New(const FunctionCallbackInfo<Value>& args) {
131131
void FSEventWrap::Start(const FunctionCallbackInfo<Value>& args) {
132132
Environment* env = Environment::GetCurrent(args);
133133

134-
FSEventWrap* wrap = Unwrap<FSEventWrap>(args.Holder());
134+
FSEventWrap* wrap = Unwrap<FSEventWrap>(args.This());
135135
CHECK_NE(wrap, nullptr);
136136
CHECK(!wrap->initialized_);
137137

Collapse file

‎src/handle_wrap.cc‎

Copy file name to clipboardExpand all lines: src/handle_wrap.cc
-2Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,6 @@ HandleWrap::HandleWrap(Environment* env,
9393
handle_(handle) {
9494
handle_->data = this;
9595
HandleScope scope(env->isolate());
96-
Wrap(object, this);
9796
env->handle_wrap_queue()->PushBack(this);
9897
}
9998

@@ -114,7 +113,6 @@ void HandleWrap::OnClose(uv_handle_t* handle) {
114113
if (have_close_callback)
115114
wrap->MakeCallback(env->onclose_string(), 0, nullptr);
116115

117-
ClearWrap(wrap->object());
118116
delete wrap;
119117
}
120118

Collapse file

‎src/inspector_js_api.cc‎

Copy file name to clipboardExpand all lines: src/inspector_js_api.cc
-4Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,6 @@ class JSBindingsConnection : public AsyncWrap {
6464
Local<Function> callback)
6565
: AsyncWrap(env, wrap, PROVIDER_INSPECTORJSBINDING),
6666
callback_(env->isolate(), callback) {
67-
Wrap(wrap, this);
6867
Agent* inspector = env->inspector_agent();
6968
session_ = inspector->Connect(std::unique_ptr<JSBindingsSessionDelegate>(
7069
new JSBindingsSessionDelegate(env, this)));
@@ -83,9 +82,6 @@ class JSBindingsConnection : public AsyncWrap {
8382

8483
void Disconnect() {
8584
session_.reset();
86-
if (!persistent().IsEmpty()) {
87-
ClearWrap(object());
88-
}
8985
delete this;
9086
}
9187

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.