FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: refactor `BaseObject` internal field management · nodejs/node@f5d4253 · GitHub

/ node Public

Commit f5d4253

Browse files
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

40 files changed

Lines changed: 147 additions & 244 deletions

‎src/async_wrap.cc‎

Lines 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

‎src/base_object-inl.h‎

Lines 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

‎src/base_object.h‎

Lines 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

‎src/cares_wrap.cc‎

Lines 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");

‎src/connect_wrap.cc‎

Lines 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

‎src/connect_wrap.h‎

Lines 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
};

‎src/connection_wrap.h‎

Lines 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

‎src/fs_event_wrap.cc‎

Lines 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

‎src/handle_wrap.cc‎

Lines 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

‎src/inspector_js_api.cc‎

Lines 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)
Back | FazBrowse Home | New Git URL