Skip to content

Commit

Permalink
src: introduce custom smart pointers for BaseObjects
Browse files Browse the repository at this point in the history 
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 <[email protected]>

PR-URL: #30374
Refs: nodejs/quic#165
Reviewed-By: David Carlier <[email protected]>
  • Loading branch information
@addaleax @MylesBorins
addaleax authored and MylesBorins committed Nov 21, 2019
1 parent 1a92c88 commit a2dbadc
Show file tree
Hide file tree
Showing 13 changed files with 536 additions and 33 deletions.
1 change: 1 addition & 0 deletions node.gyp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1100,6 +1100,7 @@
'test/cctest/node_test_fixture.h',
'test/cctest/test_aliased_buffer.cc',
'test/cctest/test_base64.cc',
'test/cctest/test_base_object_ptr.cc',
'test/cctest/test_node_postmortem_metadata.cc',
'test/cctest/test_environment.cc',
'test/cctest/test_linked_binding.cc',
Expand Down
215 changes: 206 additions & 9 deletions src/base_object-inl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,24 +32,33 @@
namespace node {

BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> object)
: persistent_handle_(env->isolate(), object),
env_(env) {
: persistent_handle_(env->isolate(), object), env_(env) {
CHECK_EQ(false, object.IsEmpty());
CHECK_GT(object->InternalFieldCount(), 0);
object->SetAlignedPointerInInternalField(0, static_cast<void*>(this));
env_->AddCleanupHook(DeleteMe, static_cast<void*>(this));
env->AddCleanupHook(DeleteMe, static_cast<void*>(this));
env->modify_base_object_count(1);
}

BaseObject::~BaseObject() {
RemoveCleanupHook();
env()->modify_base_object_count(-1);
env()->RemoveCleanupHook(DeleteMe, static_cast<void*>(this));

if (UNLIKELY(has_pointer_data())) {
PointerData* metadata = pointer_data();
CHECK_EQ(metadata->strong_ptr_count, 0);
metadata->self = nullptr;
if (metadata->weak_ptr_count == 0)
delete metadata;
}

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

{
v8::HandleScope handle_scope(env_->isolate());
v8::HandleScope handle_scope(env()->isolate());
object()->SetAlignedPointerInInternalField(0, nullptr);
}
}
Expand All @@ -58,20 +67,25 @@ void BaseObject::RemoveCleanupHook() {
env_->RemoveCleanupHook(DeleteMe, static_cast<void*>(this));
}

void BaseObject::Detach() {
CHECK_GT(pointer_data()->strong_ptr_count, 0);
pointer_data()->is_detached = true;
}

v8::Global<v8::Object>& BaseObject::persistent() {
return persistent_handle_;
}


v8::Local<v8::Object> BaseObject::object() const {
return PersistentToLocal::Default(env_->isolate(), persistent_handle_);
return PersistentToLocal::Default(env()->isolate(), persistent_handle_);
}

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

DCHECK_EQ(handle->CreationContext()->GetIsolate(), isolate);
DCHECK_EQ(env_->isolate(), isolate);
DCHECK_EQ(env()->isolate(), isolate);

return handle;
}
Expand All @@ -80,7 +94,6 @@ Environment* BaseObject::env() const {
return env_;
}


BaseObject* BaseObject::FromJSObject(v8::Local<v8::Object> obj) {
CHECK_GT(obj->InternalFieldCount(), 0);
return static_cast<BaseObject*>(obj->GetAlignedPointerFromInternalField(0));
Expand All @@ -94,20 +107,34 @@ T* BaseObject::FromJSObject(v8::Local<v8::Object> object) {


void BaseObject::MakeWeak() {
if (has_pointer_data()) {
pointer_data()->wants_weak_jsobj = true;
if (pointer_data()->strong_ptr_count > 0) return;
}

persistent_handle_.SetWeak(
this,
[](const v8::WeakCallbackInfo<BaseObject>& data) {
std::unique_ptr<BaseObject> obj(data.GetParameter());
BaseObject* obj = data.GetParameter();
// Clear the persistent handle so that ~BaseObject() doesn't attempt
// to mess with internal fields, since the JS object may have
// transitioned into an invalid state.
// Refs: https://github.com/nodejs/node/issues/18897
obj->persistent_handle_.Reset();
CHECK_IMPLIES(obj->has_pointer_data(),
obj->pointer_data()->strong_ptr_count == 0);
obj->OnGCCollect();
}, v8::WeakCallbackType::kParameter);
}

void BaseObject::OnGCCollect() {
delete this;
}

void BaseObject::ClearWeak() {
if (has_pointer_data())
pointer_data()->wants_weak_jsobj = false;

persistent_handle_.ClearWeak();
}

Expand Down Expand Up @@ -141,6 +168,176 @@ void BaseObject::InternalFieldSet(v8::Local<v8::String> property,
info.This()->SetInternalField(Field, value);
}

bool BaseObject::has_pointer_data() const {
return pointer_data_ != nullptr;
}

BaseObject::PointerData* BaseObject::pointer_data() {
if (!has_pointer_data()) {
PointerData* metadata = new PointerData();
metadata->wants_weak_jsobj = persistent_handle_.IsWeak();
metadata->self = this;
pointer_data_ = metadata;
}
CHECK(has_pointer_data());
return pointer_data_;
}

void BaseObject::decrease_refcount() {
CHECK(has_pointer_data());
PointerData* metadata = pointer_data();
CHECK_GT(metadata->strong_ptr_count, 0);
unsigned int new_refcount = --metadata->strong_ptr_count;
if (new_refcount == 0) {
if (metadata->is_detached) {
delete this;
} else if (metadata->wants_weak_jsobj && !persistent_handle_.IsEmpty()) {
MakeWeak();
}
}
}

void BaseObject::increase_refcount() {
unsigned int prev_refcount = pointer_data()->strong_ptr_count++;
if (prev_refcount == 0 && !persistent_handle_.IsEmpty())
persistent_handle_.ClearWeak();
}

template <typename T, bool kIsWeak>
BaseObject::PointerData*
BaseObjectPtrImpl<T, kIsWeak>::pointer_data() const {
if (kIsWeak) {
return data_.pointer_data;
} else {
if (get_base_object() == nullptr) return nullptr;
return get_base_object()->pointer_data();
}
}

template <typename T, bool kIsWeak>
BaseObject* BaseObjectPtrImpl<T, kIsWeak>::get_base_object() const {
if (kIsWeak) {
if (pointer_data() == nullptr) return nullptr;
return pointer_data()->self;
} else {
return data_.target;
}
}

template <typename T, bool kIsWeak>
BaseObjectPtrImpl<T, kIsWeak>::~BaseObjectPtrImpl() {
if (get() == nullptr) return;
if (kIsWeak) {
if (--pointer_data()->weak_ptr_count == 0 &&
pointer_data()->self == nullptr) {
delete pointer_data();
}
} else {
get()->decrease_refcount();
}
}

template <typename T, bool kIsWeak>
BaseObjectPtrImpl<T, kIsWeak>::BaseObjectPtrImpl() {
data_.target = nullptr;
}

template <typename T, bool kIsWeak>
BaseObjectPtrImpl<T, kIsWeak>::BaseObjectPtrImpl(T* target)
: BaseObjectPtrImpl() {
if (target == nullptr) return;
if (kIsWeak) {
data_.pointer_data = target->pointer_data();
CHECK_NOT_NULL(pointer_data());
pointer_data()->weak_ptr_count++;
} else {
data_.target = target;
CHECK_NOT_NULL(pointer_data());
get()->increase_refcount();
}
}

template <typename T, bool kIsWeak>
template <typename U, bool kW>
BaseObjectPtrImpl<T, kIsWeak>::BaseObjectPtrImpl(
const BaseObjectPtrImpl<U, kW>& other)
: BaseObjectPtrImpl(other.get()) {}

template <typename T, bool kIsWeak>
BaseObjectPtrImpl<T, kIsWeak>::BaseObjectPtrImpl(const BaseObjectPtrImpl& other)
: BaseObjectPtrImpl(other.get()) {}

template <typename T, bool kIsWeak>
template <typename U, bool kW>
BaseObjectPtrImpl<T, kIsWeak>& BaseObjectPtrImpl<T, kIsWeak>::operator=(
const BaseObjectPtrImpl<U, kW>& other) {
if (other.get() == get()) return *this;
this->~BaseObjectPtrImpl();
return *new (this) BaseObjectPtrImpl(other);
}

template <typename T, bool kIsWeak>
BaseObjectPtrImpl<T, kIsWeak>& BaseObjectPtrImpl<T, kIsWeak>::operator=(
const BaseObjectPtrImpl& other) {
if (other.get() == get()) return *this;
this->~BaseObjectPtrImpl();
return *new (this) BaseObjectPtrImpl(other);
}

template <typename T, bool kIsWeak>
BaseObjectPtrImpl<T, kIsWeak>::BaseObjectPtrImpl(BaseObjectPtrImpl&& other)
: data_(other.data_) {
if (kIsWeak)
other.data_.target = nullptr;
else
other.data_.pointer_data = nullptr;
}

template <typename T, bool kIsWeak>
BaseObjectPtrImpl<T, kIsWeak>& BaseObjectPtrImpl<T, kIsWeak>::operator=(
BaseObjectPtrImpl&& other) {
if (&other == this) return *this;
this->~BaseObjectPtrImpl();
return *new (this) BaseObjectPtrImpl(std::move(other));
}

template <typename T, bool kIsWeak>
void BaseObjectPtrImpl<T, kIsWeak>::reset(T* ptr) {
*this = BaseObjectPtrImpl(ptr);
}

template <typename T, bool kIsWeak>
T* BaseObjectPtrImpl<T, kIsWeak>::get() const {
return static_cast<T*>(get_base_object());
}

template <typename T, bool kIsWeak>
T& BaseObjectPtrImpl<T, kIsWeak>::operator*() const {
return *get();
}

template <typename T, bool kIsWeak>
T* BaseObjectPtrImpl<T, kIsWeak>::operator->() const {
return get();
}

template <typename T, bool kIsWeak>
BaseObjectPtrImpl<T, kIsWeak>::operator bool() const {
return get() != nullptr;
}

template <typename T, typename... Args>
BaseObjectPtr<T> MakeBaseObject(Args&&... args) {
return BaseObjectPtr<T>(new T(std::forward<Args>(args)...));
}

template <typename T, typename... Args>
BaseObjectPtr<T> MakeDetachedBaseObject(Args&&... args) {
BaseObjectPtr<T> target = MakeBaseObject<T>(std::forward<Args>(args)...);
target->Detach();
return target;
}

} // namespace node

#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
Expand Down
Loading

0 comments on commit a2dbadc

Please sign in to comment.