Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: create BaseObject with node::Realm #44348

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions node.gyp
Expand Up @@ -465,6 +465,7 @@
'src/api/hooks.cc',
'src/api/utils.cc',
'src/async_wrap.cc',
'src/base_object.cc',
'src/cares_wrap.cc',
'src/cleanup_queue.cc',
'src/connect_wrap.cc',
Expand Down
2 changes: 1 addition & 1 deletion src/api/embed_helpers.cc
Expand Up @@ -68,7 +68,7 @@ Maybe<int> SpinEventLoop(Environment* env) {
env->set_snapshot_serialize_callback(Local<Function>());

env->PrintInfoForSnapshotIfDebug();
env->VerifyNoStrongBaseObjects();
env->principal_realm()->VerifyNoStrongBaseObjects();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it's a good time now to introduce a list of realms in the env and do a env->ForEachRealm() in places like this, so we don't have to update everything again later other than adding more realms to the list..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, thank you for the suggestion, it makes sense to me. Updated.

return EmitProcessExit(env);
}

Expand Down
9 changes: 8 additions & 1 deletion src/base_object-inl.h
Expand Up @@ -32,6 +32,9 @@

namespace node {

BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> object)
: BaseObject(env->principal_realm(), object) {}

// static
v8::Local<v8::FunctionTemplate> BaseObject::GetConstructorTemplate(
Environment* env) {
Expand Down Expand Up @@ -63,7 +66,11 @@ v8::Local<v8::Object> BaseObject::object(v8::Isolate* isolate) const {
}

Environment* BaseObject::env() const {
return env_;
return realm_->env();
}

Realm* BaseObject::realm() const {
return realm_;
}

BaseObject* BaseObject::FromJSObject(v8::Local<v8::Value> value) {
Expand Down
164 changes: 164 additions & 0 deletions src/base_object.cc
@@ -0,0 +1,164 @@
#include "base_object.h"
#include "env-inl.h"
#include "node_realm-inl.h"

namespace node {

using v8::FunctionCallbackInfo;
using v8::FunctionTemplate;
using v8::HandleScope;
using v8::Local;
using v8::Object;
using v8::Value;
using v8::WeakCallbackInfo;
using v8::WeakCallbackType;

BaseObject::BaseObject(Realm* realm, Local<Object> object)
: persistent_handle_(realm->isolate(), object), realm_(realm) {
CHECK_EQ(false, object.IsEmpty());
CHECK_GE(object->InternalFieldCount(), BaseObject::kInternalFieldCount);
object->SetAlignedPointerInInternalField(BaseObject::kEmbedderType,
&kNodeEmbedderId);
object->SetAlignedPointerInInternalField(BaseObject::kSlot,
static_cast<void*>(this));
realm->AddCleanupHook(DeleteMe, static_cast<void*>(this));
realm->modify_base_object_count(1);
}

BaseObject::~BaseObject() {
realm()->modify_base_object_count(-1);
realm()->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;
}

{
HandleScope handle_scope(env()->isolate());
legendecas marked this conversation as resolved.
Show resolved Hide resolved
object()->SetAlignedPointerInInternalField(BaseObject::kSlot, nullptr);
}
}

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 WeakCallbackInfo<BaseObject>& data) {
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();
},
WeakCallbackType::kParameter);
}

// This just has to be different from the Chromium ones:
// https://source.chromium.org/chromium/chromium/src/+/main:gin/public/gin_embedders.h;l=18-23;drc=5a758a97032f0b656c3c36a3497560762495501a
// Otherwise, when Node is loaded in an isolate which uses cppgc, cppgc will
// misinterpret the data stored in the embedder fields and try to garbage
// collect them.
uint16_t kNodeEmbedderId = 0x90de;

void BaseObject::LazilyInitializedJSTemplateConstructor(
const FunctionCallbackInfo<Value>& args) {
DCHECK(args.IsConstructCall());
CHECK_GE(args.This()->InternalFieldCount(), BaseObject::kInternalFieldCount);
args.This()->SetAlignedPointerInInternalField(BaseObject::kEmbedderType,
&kNodeEmbedderId);
args.This()->SetAlignedPointerInInternalField(BaseObject::kSlot, nullptr);
}

Local<FunctionTemplate> BaseObject::MakeLazilyInitializedJSTemplate(
Environment* env) {
Local<FunctionTemplate> t = NewFunctionTemplate(
env->isolate(), LazilyInitializedJSTemplateConstructor);
t->Inherit(BaseObject::GetConstructorTemplate(env));
t->InstanceTemplate()->SetInternalFieldCount(BaseObject::kInternalFieldCount);
return t;
}

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) {
OnGCCollect();
} 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();
}

void BaseObject::DeleteMe(void* data) {
BaseObject* self = static_cast<BaseObject*>(data);
if (self->has_pointer_data() && self->pointer_data()->strong_ptr_count > 0) {
return self->Detach();
}
delete self;
}

bool BaseObject::IsDoneInitializing() const {
return true;
}

Local<Object> BaseObject::WrappedObject() const {
return object();
}

bool BaseObject::IsRootNode() const {
return !persistent_handle_.IsWeak();
}

Local<FunctionTemplate> BaseObject::GetConstructorTemplate(
IsolateData* isolate_data) {
Local<FunctionTemplate> tmpl = isolate_data->base_object_ctor_template();
if (tmpl.IsEmpty()) {
tmpl = NewFunctionTemplate(isolate_data->isolate(), nullptr);
tmpl->SetClassName(
FIXED_ONE_BYTE_STRING(isolate_data->isolate(), "BaseObject"));
isolate_data->set_base_object_ctor_template(tmpl);
}
return tmpl;
}

bool BaseObject::IsNotIndicativeOfMemoryLeakAtExit() const {
return IsWeakOrDetached();
}

} // namespace node
10 changes: 8 additions & 2 deletions src/base_object.h
Expand Up @@ -32,6 +32,7 @@ namespace node {

class Environment;
class IsolateData;
class Realm;
template <typename T, bool kIsWeak>
class BaseObjectPtrImpl;

Expand All @@ -47,7 +48,10 @@ class BaseObject : public MemoryRetainer {

// Associates this object with `object`. It uses the 1st internal field for
// that, and in particular aborts if there is no such field.
BaseObject(Environment* env, v8::Local<v8::Object> object);
// This is the designated constructor.
BaseObject(Realm* realm, v8::Local<v8::Object> object);
// Convenient constructor for constructing BaseObject in the principal realm.
inline BaseObject(Environment* env, v8::Local<v8::Object> object);
~BaseObject() override;

BaseObject() = delete;
Expand All @@ -63,6 +67,7 @@ class BaseObject : public MemoryRetainer {
inline v8::Global<v8::Object>& persistent();

inline Environment* env() const;
inline Realm* realm() const;

// Get a BaseObject* pointer, or subclass pointer, for the JS object that
// was also passed to the `BaseObject()` constructor initially.
Expand Down Expand Up @@ -91,6 +96,7 @@ class BaseObject : public MemoryRetainer {
// Utility to create a FunctionTemplate with one internal field (used for
// the `BaseObject*` pointer) and a constructor that initializes that field
// to `nullptr`.
// TODO(legendecas): Disentangle template with env.
static v8::Local<v8::FunctionTemplate> MakeLazilyInitializedJSTemplate(
Environment* env);

Expand Down Expand Up @@ -213,7 +219,7 @@ class BaseObject : public MemoryRetainer {
void decrease_refcount();
void increase_refcount();

Environment* env_;
Realm* realm_;
PointerData* pointer_data_ = nullptr;
};

Expand Down
21 changes: 0 additions & 21 deletions src/env-inl.h
Expand Up @@ -789,27 +789,6 @@ void Environment::RemoveCleanupHook(CleanupQueue::Callback fn, void* arg) {
cleanup_queue_.Remove(fn, arg);
}

template <typename T>
void Environment::ForEachBaseObject(T&& iterator) {
cleanup_queue_.ForEachBaseObject(std::forward<T>(iterator));
}

void Environment::modify_base_object_count(int64_t delta) {
base_object_count_ += delta;
}

int64_t Environment::base_object_count() const {
return base_object_count_;
}

inline void Environment::set_base_object_created_by_bootstrap(int64_t count) {
base_object_created_by_bootstrap_ = base_object_count_;
}

int64_t Environment::base_object_created_after_bootstrap() const {
return base_object_count_ - base_object_created_by_bootstrap_;
}

void Environment::set_main_utf16(std::unique_ptr<v8::String::Value> str) {
CHECK(!main_utf16_);
main_utf16_ = std::move(str);
Expand Down