Skip to content

Commit afa5499

Browse files
addaleaxMylesBorins
addaleax
authored and
MylesBorins
committed
src: introduce custom smart pointers for BaseObjects
Referring to `BaseObject` instances using standard C++ smart pointers can interfere with BaseObject’s own cleanup mechanisms (explicit delete, delete-on-GC and delete-on-cleanup). Introducing custom smart pointers allows referring to `BaseObject`s safely while keeping those mechanisms intact. Refs: nodejs/quic#141 Refs: nodejs/quic#149 Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: nodejs#30374 Refs: nodejs/quic#165 Reviewed-By: David Carlier <devnexen@gmail.com>
1 parent dca3d29 commit afa5499

13 files changed

+535
-33
lines changed

‎node.gyp

+1
Original file line numberDiff line numberDiff line change
@@ -1117,6 +1117,7 @@
11171117
'test/cctest/node_test_fixture.h',
11181118
'test/cctest/test_aliased_buffer.cc',
11191119
'test/cctest/test_base64.cc',
1120+
'test/cctest/test_base_object_ptr.cc',
11201121
'test/cctest/test_node_postmortem_metadata.cc',
11211122
'test/cctest/test_environment.cc',
11221123
'test/cctest/test_linked_binding.cc',

‎src/base_object-inl.h

+206-9
Original file line numberDiff line numberDiff line change
@@ -40,24 +40,33 @@
4040
namespace node {
4141

4242
BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> object)
43-
: persistent_handle_(env->isolate(), object),
44-
env_(env) {
43+
: persistent_handle_(env->isolate(), object), env_(env) {
4544
CHECK_EQ(false, object.IsEmpty());
4645
CHECK_GT(object->InternalFieldCount(), 0);
4746
object->SetAlignedPointerInInternalField(0, static_cast<void*>(this));
48-
env_->AddCleanupHook(DeleteMe, static_cast<void*>(this));
47+
env->AddCleanupHook(DeleteMe, static_cast<void*>(this));
48+
env->modify_base_object_count(1);
4949
}
5050

5151
BaseObject::~BaseObject() {
52-
RemoveCleanupHook();
52+
env()->modify_base_object_count(-1);
53+
env()->RemoveCleanupHook(DeleteMe, static_cast<void*>(this));
54+
55+
if (UNLIKELY(has_pointer_data())) {
56+
PointerData* metadata = pointer_data();
57+
CHECK_EQ(metadata->strong_ptr_count, 0);
58+
metadata->self = nullptr;
59+
if (metadata->weak_ptr_count == 0)
60+
delete metadata;
61+
}
5362

5463
if (persistent_handle_.IsEmpty()) {
5564
// This most likely happened because the weak callback below cleared it.
5665
return;
5766
}
5867

5968
{
60-
v8::HandleScope handle_scope(env_->isolate());
69+
v8::HandleScope handle_scope(env()->isolate());
6170
object()->SetAlignedPointerInInternalField(0, nullptr);
6271
}
6372
}
@@ -66,20 +75,25 @@ void BaseObject::RemoveCleanupHook() {
6675
env_->RemoveCleanupHook(DeleteMe, static_cast<void*>(this));
6776
}
6877

78+
void BaseObject::Detach() {
79+
CHECK_GT(pointer_data()->strong_ptr_count, 0);
80+
pointer_data()->is_detached = true;
81+
}
82+
6983
v8::Global<v8::Object>& BaseObject::persistent() {
7084
return persistent_handle_;
7185
}
7286

7387

7488
v8::Local<v8::Object> BaseObject::object() const {
75-
return PersistentToLocal::Default(env_->isolate(), persistent_handle_);
89+
return PersistentToLocal::Default(env()->isolate(), persistent_handle_);
7690
}
7791

7892
v8::Local<v8::Object> BaseObject::object(v8::Isolate* isolate) const {
7993
v8::Local<v8::Object> handle = object();
8094

8195
DCHECK_EQ(handle->CreationContext()->GetIsolate(), isolate);
82-
DCHECK_EQ(env_->isolate(), isolate);
96+
DCHECK_EQ(env()->isolate(), isolate);
8397

8498
return handle;
8599
}
@@ -88,7 +102,6 @@ Environment* BaseObject::env() const {
88102
return env_;
89103
}
90104

91-
92105
BaseObject* BaseObject::FromJSObject(v8::Local<v8::Object> obj) {
93106
CHECK_GT(obj->InternalFieldCount(), 0);
94107
return static_cast<BaseObject*>(obj->GetAlignedPointerFromInternalField(0));
@@ -102,20 +115,34 @@ T* BaseObject::FromJSObject(v8::Local<v8::Object> object) {
102115

103116

104117
void BaseObject::MakeWeak() {
118+
if (has_pointer_data()) {
119+
pointer_data()->wants_weak_jsobj = true;
120+
if (pointer_data()->strong_ptr_count > 0) return;
121+
}
122+
105123
persistent_handle_.SetWeak(
106124
this,
107125
[](const v8::WeakCallbackInfo<BaseObject>& data) {
108-
std::unique_ptr<BaseObject> obj(data.GetParameter());
126+
BaseObject* obj = data.GetParameter();
109127
// Clear the persistent handle so that ~BaseObject() doesn't attempt
110128
// to mess with internal fields, since the JS object may have
111129
// transitioned into an invalid state.
112130
// Refs: https://github.com/nodejs/node/issues/18897
113131
obj->persistent_handle_.Reset();
132+
CHECK_IMPLIES(obj->has_pointer_data(),
133+
obj->pointer_data()->strong_ptr_count == 0);
134+
obj->OnGCCollect();
114135
}, v8::WeakCallbackType::kParameter);
115136
}
116137

138+
void BaseObject::OnGCCollect() {
139+
delete this;
140+
}
117141

118142
void BaseObject::ClearWeak() {
143+
if (has_pointer_data())
144+
pointer_data()->wants_weak_jsobj = false;
145+
119146
persistent_handle_.ClearWeak();
120147
}
121148

@@ -149,6 +176,176 @@ void BaseObject::InternalFieldSet(v8::Local<v8::String> property,
149176
info.This()->SetInternalField(Field, value);
150177
}
151178

179+
bool BaseObject::has_pointer_data() const {
180+
return pointer_data_ != nullptr;
181+
}
182+
183+
BaseObject::PointerData* BaseObject::pointer_data() {
184+
if (!has_pointer_data()) {
185+
PointerData* metadata = new PointerData();
186+
metadata->wants_weak_jsobj = persistent_handle_.IsWeak();
187+
metadata->self = this;
188+
pointer_data_ = metadata;
189+
}
190+
CHECK(has_pointer_data());
191+
return pointer_data_;
192+
}
193+
194+
void BaseObject::decrease_refcount() {
195+
CHECK(has_pointer_data());
196+
PointerData* metadata = pointer_data();
197+
CHECK_GT(metadata->strong_ptr_count, 0);
198+
unsigned int new_refcount = --metadata->strong_ptr_count;
199+
if (new_refcount == 0) {
200+
if (metadata->is_detached) {
201+
delete this;
202+
} else if (metadata->wants_weak_jsobj && !persistent_handle_.IsEmpty()) {
203+
MakeWeak();
204+
}
205+
}
206+
}
207+
208+
void BaseObject::increase_refcount() {
209+
unsigned int prev_refcount = pointer_data()->strong_ptr_count++;
210+
if (prev_refcount == 0 && !persistent_handle_.IsEmpty())
211+
persistent_handle_.ClearWeak();
212+
}
213+
214+
template <typename T, bool kIsWeak>
215+
BaseObject::PointerData*
216+
BaseObjectPtrImpl<T, kIsWeak>::pointer_data() const {
217+
if (kIsWeak) {
218+
return data_.pointer_data;
219+
} else {
220+
if (get_base_object() == nullptr) return nullptr;
221+
return get_base_object()->pointer_data();
222+
}
223+
}
224+
225+
template <typename T, bool kIsWeak>
226+
BaseObject* BaseObjectPtrImpl<T, kIsWeak>::get_base_object() const {
227+
if (kIsWeak) {
228+
if (pointer_data() == nullptr) return nullptr;
229+
return pointer_data()->self;
230+
} else {
231+
return data_.target;
232+
}
233+
}
234+
235+
template <typename T, bool kIsWeak>
236+
BaseObjectPtrImpl<T, kIsWeak>::~BaseObjectPtrImpl() {
237+
if (get() == nullptr) return;
238+
if (kIsWeak) {
239+
if (--pointer_data()->weak_ptr_count == 0 &&
240+
pointer_data()->self == nullptr) {
241+
delete pointer_data();
242+
}
243+
} else {
244+
get()->decrease_refcount();
245+
}
246+
}
247+
248+
template <typename T, bool kIsWeak>
249+
BaseObjectPtrImpl<T, kIsWeak>::BaseObjectPtrImpl() {
250+
data_.target = nullptr;
251+
}
252+
253+
template <typename T, bool kIsWeak>
254+
BaseObjectPtrImpl<T, kIsWeak>::BaseObjectPtrImpl(T* target)
255+
: BaseObjectPtrImpl() {
256+
if (target == nullptr) return;
257+
if (kIsWeak) {
258+
data_.pointer_data = target->pointer_data();
259+
CHECK_NOT_NULL(pointer_data());
260+
pointer_data()->weak_ptr_count++;
261+
} else {
262+
data_.target = target;
263+
CHECK_NOT_NULL(pointer_data());
264+
get()->increase_refcount();
265+
}
266+
}
267+
268+
template <typename T, bool kIsWeak>
269+
template <typename U, bool kW>
270+
BaseObjectPtrImpl<T, kIsWeak>::BaseObjectPtrImpl(
271+
const BaseObjectPtrImpl<U, kW>& other)
272+
: BaseObjectPtrImpl(other.get()) {}
273+
274+
template <typename T, bool kIsWeak>
275+
BaseObjectPtrImpl<T, kIsWeak>::BaseObjectPtrImpl(const BaseObjectPtrImpl& other)
276+
: BaseObjectPtrImpl(other.get()) {}
277+
278+
template <typename T, bool kIsWeak>
279+
template <typename U, bool kW>
280+
BaseObjectPtrImpl<T, kIsWeak>& BaseObjectPtrImpl<T, kIsWeak>::operator=(
281+
const BaseObjectPtrImpl<U, kW>& other) {
282+
if (other.get() == get()) return *this;
283+
this->~BaseObjectPtrImpl();
284+
return *new (this) BaseObjectPtrImpl(other);
285+
}
286+
287+
template <typename T, bool kIsWeak>
288+
BaseObjectPtrImpl<T, kIsWeak>& BaseObjectPtrImpl<T, kIsWeak>::operator=(
289+
const BaseObjectPtrImpl& other) {
290+
if (other.get() == get()) return *this;
291+
this->~BaseObjectPtrImpl();
292+
return *new (this) BaseObjectPtrImpl(other);
293+
}
294+
295+
template <typename T, bool kIsWeak>
296+
BaseObjectPtrImpl<T, kIsWeak>::BaseObjectPtrImpl(BaseObjectPtrImpl&& other)
297+
: data_(other.data_) {
298+
if (kIsWeak)
299+
other.data_.target = nullptr;
300+
else
301+
other.data_.pointer_data = nullptr;
302+
}
303+
304+
template <typename T, bool kIsWeak>
305+
BaseObjectPtrImpl<T, kIsWeak>& BaseObjectPtrImpl<T, kIsWeak>::operator=(
306+
BaseObjectPtrImpl&& other) {
307+
if (&other == this) return *this;
308+
this->~BaseObjectPtrImpl();
309+
return *new (this) BaseObjectPtrImpl(std::move(other));
310+
}
311+
312+
template <typename T, bool kIsWeak>
313+
void BaseObjectPtrImpl<T, kIsWeak>::reset(T* ptr) {
314+
*this = BaseObjectPtrImpl(ptr);
315+
}
316+
317+
template <typename T, bool kIsWeak>
318+
T* BaseObjectPtrImpl<T, kIsWeak>::get() const {
319+
return static_cast<T*>(get_base_object());
320+
}
321+
322+
template <typename T, bool kIsWeak>
323+
T& BaseObjectPtrImpl<T, kIsWeak>::operator*() const {
324+
return *get();
325+
}
326+
327+
template <typename T, bool kIsWeak>
328+
T* BaseObjectPtrImpl<T, kIsWeak>::operator->() const {
329+
return get();
330+
}
331+
332+
template <typename T, bool kIsWeak>
333+
BaseObjectPtrImpl<T, kIsWeak>::operator bool() const {
334+
return get() != nullptr;
335+
}
336+
337+
template <typename T, typename... Args>
338+
BaseObjectPtr<T> MakeBaseObject(Args&&... args) {
339+
return BaseObjectPtr<T>(new T(std::forward<Args>(args)...));
340+
}
341+
342+
template <typename T, typename... Args>
343+
BaseObjectPtr<T> MakeDetachedBaseObject(Args&&... args) {
344+
BaseObjectPtr<T> target = MakeBaseObject<T>(std::forward<Args>(args)...);
345+
target->Detach();
346+
return target;
347+
}
348+
152349
} // namespace node
153350

154351
#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

0 commit comments

Comments
 (0)