From 7b992cc229a6a3f6bfa5641fb30d8af35953d038 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Tue, 27 Sep 2022 16:39:53 +0800 Subject: [PATCH] src: create BaseObject with node::Realm BaseObject is a wrapper around JS objects. These objects should be created in a node::Realm and destroyed when their associated realm is cleaning up. PR-URL: https://github.com/nodejs/node/pull/44348 Refs: https://github.com/nodejs/node/issues/42528 Reviewed-By: Joyee Cheung --- node.gyp | 1 + src/api/embed_helpers.cc | 2 +- src/base_object-inl.h | 9 +- src/base_object.cc | 164 ++++++++++++++++++++++ src/base_object.h | 10 +- src/env-inl.h | 27 +--- src/env.cc | 209 ++-------------------------- src/env.h | 20 +-- src/node_realm-inl.h | 30 ++++ src/node_realm.cc | 74 +++++++++- src/node_realm.h | 30 +++- src/node_snapshotable.cc | 26 ++-- src/node_snapshotable.h | 6 +- test/cctest/test_base_object_ptr.cc | 46 +++--- test/pummel/test-heapdump-env.js | 15 +- 15 files changed, 381 insertions(+), 288 deletions(-) create mode 100644 src/base_object.cc diff --git a/node.gyp b/node.gyp index 949e2599556368..24c0497f4a8479 100644 --- a/node.gyp +++ b/node.gyp @@ -477,6 +477,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', diff --git a/src/api/embed_helpers.cc b/src/api/embed_helpers.cc index bd0459f20b1b3e..13e0f826cf1e75 100644 --- a/src/api/embed_helpers.cc +++ b/src/api/embed_helpers.cc @@ -68,7 +68,7 @@ Maybe SpinEventLoop(Environment* env) { env->set_snapshot_serialize_callback(Local()); env->PrintInfoForSnapshotIfDebug(); - env->VerifyNoStrongBaseObjects(); + env->ForEachRealm([](Realm* realm) { realm->VerifyNoStrongBaseObjects(); }); return EmitProcessExit(env); } diff --git a/src/base_object-inl.h b/src/base_object-inl.h index c5560f66bffb04..498ab9afb07666 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -32,6 +32,9 @@ namespace node { +BaseObject::BaseObject(Environment* env, v8::Local object) + : BaseObject(env->principal_realm(), object) {} + // static v8::Local BaseObject::GetConstructorTemplate( Environment* env) { @@ -63,7 +66,11 @@ v8::Local 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 value) { diff --git a/src/base_object.cc b/src/base_object.cc new file mode 100644 index 00000000000000..cdf1285bef06fa --- /dev/null +++ b/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) + : 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(this)); + realm->AddCleanupHook(DeleteMe, static_cast(this)); + realm->modify_base_object_count(1); +} + +BaseObject::~BaseObject() { + realm()->modify_base_object_count(-1); + realm()->RemoveCleanupHook(DeleteMe, static_cast(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(realm()->isolate()); + 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& 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& args) { + DCHECK(args.IsConstructCall()); + CHECK_GE(args.This()->InternalFieldCount(), BaseObject::kInternalFieldCount); + args.This()->SetAlignedPointerInInternalField(BaseObject::kEmbedderType, + &kNodeEmbedderId); + args.This()->SetAlignedPointerInInternalField(BaseObject::kSlot, nullptr); +} + +Local BaseObject::MakeLazilyInitializedJSTemplate( + Environment* env) { + Local 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(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 BaseObject::WrappedObject() const { + return object(); +} + +bool BaseObject::IsRootNode() const { + return !persistent_handle_.IsWeak(); +} + +Local BaseObject::GetConstructorTemplate( + IsolateData* isolate_data) { + Local 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 diff --git a/src/base_object.h b/src/base_object.h index 2d1dffed361599..25a380cb746c01 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -32,6 +32,7 @@ namespace node { class Environment; class IsolateData; +class Realm; template class BaseObjectPtrImpl; @@ -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 object); + // This is the designated constructor. + BaseObject(Realm* realm, v8::Local object); + // Convenient constructor for constructing BaseObject in the principal realm. + inline BaseObject(Environment* env, v8::Local object); ~BaseObject() override; BaseObject() = delete; @@ -63,6 +67,7 @@ class BaseObject : public MemoryRetainer { inline v8::Global& 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. @@ -93,6 +98,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 MakeLazilyInitializedJSTemplate( Environment* env); @@ -215,7 +221,7 @@ class BaseObject : public MemoryRetainer { void decrease_refcount(); void increase_refcount(); - Environment* env_; + Realm* realm_; PointerData* pointer_data_ = nullptr; }; diff --git a/src/env-inl.h b/src/env-inl.h index e04c0c2cf179e3..fbc4fbb27b065d 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -745,6 +745,12 @@ inline IsolateData* Environment::isolate_data() const { return isolate_data_; } +template +inline void Environment::ForEachRealm(T&& iterator) const { + // TODO(legendecas): iterate over more realms bound to the environment. + iterator(principal_realm()); +} + inline void Environment::ThrowError(const char* errmsg) { ThrowError(v8::Exception::Error, errmsg); } @@ -789,27 +795,6 @@ void Environment::RemoveCleanupHook(CleanupQueue::Callback fn, void* arg) { cleanup_queue_.Remove(fn, arg); } -template -void Environment::ForEachBaseObject(T&& iterator) { - cleanup_queue_.ForEachBaseObject(std::forward(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 str) { CHECK(!main_utf16_); main_utf16_ = std::move(str); diff --git a/src/env.cc b/src/env.cc index 43cce236ebf95a..4a6d3ffaeacb4d 100644 --- a/src/env.cc +++ b/src/env.cc @@ -37,7 +37,6 @@ using v8::Context; using v8::EmbedderGraph; using v8::EscapableHandleScope; using v8::Function; -using v8::FunctionCallbackInfo; using v8::FunctionTemplate; using v8::HandleScope; using v8::HeapSpaceStatistics; @@ -58,8 +57,6 @@ using v8::TracingController; using v8::TryCatch; using v8::Undefined; using v8::Value; -using v8::WeakCallbackInfo; -using v8::WeakCallbackType; using worker::Worker; int const ContextEmbedderTag::kNodeContextTag = 0x6e6f64; @@ -840,8 +837,6 @@ Environment::~Environment() { addon.Close(); } } - - CHECK_EQ(base_object_count_, 0); } void Environment::InitializeLibuv() { @@ -1002,11 +997,16 @@ void Environment::RunCleanup() { started_cleanup_ = true; TRACE_EVENT0(TRACING_CATEGORY_NODE1(environment), "RunCleanup"); bindings_.clear(); + // Only BaseObject's cleanups are registered as per-realm cleanup hooks now. + // Defer the BaseObject cleanup after handles are cleaned up. CleanupHandles(); - while (!cleanup_queue_.empty() || native_immediates_.size() > 0 || + while (!cleanup_queue_.empty() || principal_realm_->HasCleanupHooks() || + native_immediates_.size() > 0 || native_immediates_threadsafe_.size() > 0 || native_immediates_interrupts_.size() > 0) { + // TODO(legendecas): cleanup handles in per-realm cleanup hooks as well. + principal_realm_->RunCleanup(); cleanup_queue_.Drain(); CleanupHandles(); } @@ -1565,8 +1565,8 @@ void Environment::RemoveUnmanagedFd(int fd) { void Environment::PrintInfoForSnapshotIfDebug() { if (enabled_debug_list()->enabled(DebugCategory::MKSNAPSHOT)) { - fprintf(stderr, "BaseObjects at the exit of the Environment:\n"); - PrintAllBaseObjects(); + fprintf(stderr, "At the exit of the Environment:\n"); + principal_realm()->PrintInfoForSnapshot(); fprintf(stderr, "\nNative modules without cache:\n"); for (const auto& s : builtins_without_cache) { fprintf(stderr, "%s\n", s.c_str()); @@ -1582,45 +1582,6 @@ void Environment::PrintInfoForSnapshotIfDebug() { } } -void Environment::PrintAllBaseObjects() { - size_t i = 0; - std::cout << "BaseObjects\n"; - ForEachBaseObject([&](BaseObject* obj) { - std::cout << "#" << i++ << " " << obj << ": " << - obj->MemoryInfoName() << "\n"; - }); -} - -void Environment::VerifyNoStrongBaseObjects() { - // When a process exits cleanly, i.e. because the event loop ends up without - // things to wait for, the Node.js objects that are left on the heap should - // be: - // - // 1. weak, i.e. ready for garbage collection once no longer referenced, or - // 2. detached, i.e. scheduled for destruction once no longer referenced, or - // 3. an unrefed libuv handle, i.e. does not keep the event loop alive, or - // 4. an inactive libuv handle (essentially the same here) - // - // There are a few exceptions to this rule, but generally, if there are - // C++-backed Node.js objects on the heap that do not fall into the above - // categories, we may be looking at a potential memory leak. Most likely, - // the cause is a missing MakeWeak() call on the corresponding object. - // - // In order to avoid this kind of problem, we check the list of BaseObjects - // for these criteria. Currently, we only do so when explicitly instructed to - // or when in debug mode (where --verify-base-objects is always-on). - - if (!options()->verify_base_objects) return; - - ForEachBaseObject([](BaseObject* obj) { - if (obj->IsNotIndicativeOfMemoryLeakAtExit()) return; - fprintf(stderr, "Found bad BaseObject during clean exit: %s\n", - obj->MemoryInfoName().c_str()); - fflush(stderr); - ABORT(); - }); -} - EnvSerializeInfo Environment::Serialize(SnapshotCreator* creator) { EnvSerializeInfo info; Local ctx = context(); @@ -1639,10 +1600,6 @@ EnvSerializeInfo Environment::Serialize(SnapshotCreator* creator) { info.should_abort_on_uncaught_toggle = should_abort_on_uncaught_toggle_.Serialize(ctx, creator); - // Do this after other creator->AddData() calls so that Snapshotable objects - // can use 0 to indicate that a SnapshotIndex is invalid. - SerializeSnapshotableObjects(this, creator, &info); - info.principal_realm = principal_realm_->Serialize(creator); return info; } @@ -1716,6 +1673,7 @@ void Environment::BuildEmbedderGraph(Isolate* isolate, void* data) { MemoryTracker tracker(isolate, graph); Environment* env = static_cast(data); + // Start traversing embedder objects from the root Environment object. tracker.Track(env); } @@ -1890,153 +1848,4 @@ void Environment::MemoryInfo(MemoryTracker* tracker) const { void Environment::RunWeakRefCleanup() { isolate()->ClearKeptObjects(); } - -// Not really any better place than env.cc at this moment. -BaseObject::BaseObject(Environment* env, Local object) - : persistent_handle_(env->isolate(), object), env_(env) { - CHECK_EQ(false, object.IsEmpty()); - CHECK_GE(object->InternalFieldCount(), BaseObject::kInternalFieldCount); - object->SetAlignedPointerInInternalField(BaseObject::kEmbedderType, - &kNodeEmbedderId); - object->SetAlignedPointerInInternalField(BaseObject::kSlot, - static_cast(this)); - env->AddCleanupHook(DeleteMe, static_cast(this)); - env->modify_base_object_count(1); -} - -BaseObject::~BaseObject() { - env()->modify_base_object_count(-1); - env()->RemoveCleanupHook(DeleteMe, static_cast(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()); - 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& 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& args) { - DCHECK(args.IsConstructCall()); - CHECK_GE(args.This()->InternalFieldCount(), BaseObject::kInternalFieldCount); - args.This()->SetAlignedPointerInInternalField(BaseObject::kEmbedderType, - &kNodeEmbedderId); - args.This()->SetAlignedPointerInInternalField(BaseObject::kSlot, nullptr); -} - -Local BaseObject::MakeLazilyInitializedJSTemplate( - Environment* env) { - Local 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(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 BaseObject::WrappedObject() const { - return object(); -} - -bool BaseObject::IsRootNode() const { - return !persistent_handle_.IsWeak(); -} - -Local BaseObject::GetConstructorTemplate( - IsolateData* isolate_data) { - Local 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 diff --git a/src/env.h b/src/env.h index d344443c67b702..e484092bc2d0ed 100644 --- a/src/env.h +++ b/src/env.h @@ -508,7 +508,6 @@ struct DeserializeRequest { }; struct EnvSerializeInfo { - std::vector native_objects; std::vector builtins; AsyncHooks::SerializeInfo async_hooks; TickInfo::SerializeInfo tick_info; @@ -600,8 +599,6 @@ class Environment : public MemoryRetainer { void DeserializeProperties(const EnvSerializeInfo* info); void PrintInfoForSnapshotIfDebug(); - void PrintAllBaseObjects(); - void VerifyNoStrongBaseObjects(); void EnqueueDeserializeRequest(DeserializeRequestCallback cb, v8::Local holder, int index, @@ -966,19 +963,6 @@ class Environment : public MemoryRetainer { inline std::shared_ptr options(); inline std::shared_ptr> inspector_host_port(); - // The BaseObject count is a debugging helper that makes sure that there are - // no memory leaks caused by BaseObjects staying alive longer than expected - // (in particular, no circular BaseObjectPtr references). - inline void modify_base_object_count(int64_t delta); - inline int64_t base_object_count() const; - - // Base object count created in bootstrap of the principal realm. - // This adjusts the return value of base_object_created_after_bootstrap() so - // that tests that check the count do not have to account for internally - // created BaseObjects. - inline void set_base_object_created_by_bootstrap(int64_t count); - inline int64_t base_object_created_after_bootstrap() const; - inline int32_t stack_trace_limit() const { return 10; } #if HAVE_INSPECTOR @@ -1031,7 +1015,7 @@ class Environment : public MemoryRetainer { void RemoveUnmanagedFd(int fd); template - void ForEachBaseObject(T&& iterator); + void ForEachRealm(T&& iterator) const; inline void set_heap_snapshot_near_heap_limit(uint32_t limit); inline bool is_in_heapsnapshot_heap_limit_callback() const; @@ -1178,8 +1162,6 @@ class Environment : public MemoryRetainer { CleanupQueue cleanup_queue_; bool started_cleanup_ = false; - int64_t base_object_count_ = 0; - int64_t base_object_created_by_bootstrap_ = 0; std::atomic_bool is_stopping_ { false }; std::unordered_set unmanaged_fds_; diff --git a/src/node_realm-inl.h b/src/node_realm-inl.h index c50ba2ebcbc0ee..743071315fcceb 100644 --- a/src/node_realm-inl.h +++ b/src/node_realm-inl.h @@ -3,6 +3,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#include "cleanup_queue-inl.h" #include "node_realm.h" namespace node { @@ -41,6 +42,23 @@ inline bool Realm::has_run_bootstrapping_code() const { return has_run_bootstrapping_code_; } +template +void Realm::ForEachBaseObject(T&& iterator) const { + cleanup_queue_.ForEachBaseObject(std::forward(iterator)); +} + +void Realm::modify_base_object_count(int64_t delta) { + base_object_count_ += delta; +} + +int64_t Realm::base_object_created_after_bootstrap() const { + return base_object_count_ - base_object_created_by_bootstrap_; +} + +int64_t Realm::base_object_count() const { + return base_object_count_; +} + #define V(PropertyName, TypeName) \ inline v8::Local Realm::PropertyName() const { \ return PersistentToLocal::Strong(PropertyName##_); \ @@ -55,6 +73,18 @@ v8::Local Realm::context() const { return PersistentToLocal::Strong(context_); } +void Realm::AddCleanupHook(CleanupQueue::Callback fn, void* arg) { + cleanup_queue_.Add(fn, arg); +} + +void Realm::RemoveCleanupHook(CleanupQueue::Callback fn, void* arg) { + cleanup_queue_.Remove(fn, arg); +} + +bool Realm::HasCleanupHooks() const { + return !cleanup_queue_.empty(); +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/node_realm.cc b/src/node_realm.cc index dd045e43fdd533..0a19c89f53a7f3 100644 --- a/src/node_realm.cc +++ b/src/node_realm.cc @@ -35,6 +35,10 @@ Realm::Realm(Environment* env, } } +Realm::~Realm() { + CHECK_EQ(base_object_count_, 0); +} + void Realm::MemoryInfo(MemoryTracker* tracker) const { #define V(PropertyName, TypeName) \ tracker->TrackField(#PropertyName, PropertyName()); @@ -42,6 +46,13 @@ void Realm::MemoryInfo(MemoryTracker* tracker) const { #undef V tracker->TrackField("env", env_); + tracker->TrackField("cleanup_queue", cleanup_queue_); + + ForEachBaseObject([&](BaseObject* obj) { + if (obj->IsDoneInitializing()) { + tracker->Track(obj); + } + }); } void Realm::CreateProperties() { @@ -105,6 +116,10 @@ RealmSerializeInfo Realm::Serialize(SnapshotCreator* creator) { PER_REALM_STRONG_PERSISTENT_VALUES(V) #undef V + // Do this after other creator->AddData() calls so that Snapshotable objects + // can use 0 to indicate that a SnapshotIndex is invalid. + SerializeSnapshotableObjects(this, creator, &info); + info.context = creator->AddData(ctx, ctx); return info; } @@ -289,8 +304,6 @@ MaybeLocal Realm::RunBootstrapping() { } void Realm::DoneBootstrapping() { - has_run_bootstrapping_code_ = true; - // Make sure that no request or handle is created during bootstrap - // if necessary those should be done in pre-execution. // Usually, doing so would trigger the checks present in the ReqWrap and @@ -301,8 +314,61 @@ void Realm::DoneBootstrapping() { CHECK(env_->req_wrap_queue()->IsEmpty()); CHECK(env_->handle_wrap_queue()->IsEmpty()); - // TODO(legendecas): track base object count by realms. - env_->set_base_object_created_by_bootstrap(env_->base_object_count()); + has_run_bootstrapping_code_ = true; + + // This adjusts the return value of base_object_created_after_bootstrap() so + // that tests that check the count do not have to account for internally + // created BaseObjects. + base_object_created_by_bootstrap_ = base_object_count_; +} + +void Realm::RunCleanup() { + TRACE_EVENT0(TRACING_CATEGORY_NODE1(realm), "RunCleanup"); + + cleanup_queue_.Drain(); +} + +void Realm::PrintInfoForSnapshot() { + fprintf(stderr, "Realm = %p\n", this); + fprintf(stderr, "BaseObjects of the Realm:\n"); + size_t i = 0; + ForEachBaseObject([&](BaseObject* obj) { + std::cout << "#" << i++ << " " << obj << ": " << obj->MemoryInfoName() + << "\n"; + }); + fprintf(stderr, "End of the Realm.\n"); +} + +void Realm::VerifyNoStrongBaseObjects() { + // When a process exits cleanly, i.e. because the event loop ends up without + // things to wait for, the Node.js objects that are left on the heap should + // be: + // + // 1. weak, i.e. ready for garbage collection once no longer referenced, or + // 2. detached, i.e. scheduled for destruction once no longer referenced, or + // 3. an unrefed libuv handle, i.e. does not keep the event loop alive, or + // 4. an inactive libuv handle (essentially the same here) + // + // There are a few exceptions to this rule, but generally, if there are + // C++-backed Node.js objects on the heap that do not fall into the above + // categories, we may be looking at a potential memory leak. Most likely, + // the cause is a missing MakeWeak() call on the corresponding object. + // + // In order to avoid this kind of problem, we check the list of BaseObjects + // for these criteria. Currently, we only do so when explicitly instructed to + // or when in debug mode (where --verify-base-objects is always-on). + + // TODO(legendecas): introduce per-realm options. + if (!env()->options()->verify_base_objects) return; + + ForEachBaseObject([](BaseObject* obj) { + if (obj->IsNotIndicativeOfMemoryLeakAtExit()) return; + fprintf(stderr, + "Found bad BaseObject during clean exit: %s\n", + obj->MemoryInfoName().c_str()); + fflush(stderr); + ABORT(); + }); } } // namespace node diff --git a/src/node_realm.h b/src/node_realm.h index afaaa66c9bdd2e..9ece8cb38958c3 100644 --- a/src/node_realm.h +++ b/src/node_realm.h @@ -4,6 +4,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #include +#include "cleanup_queue.h" #include "env_properties.h" #include "memory_tracker.h" #include "node_snapshotable.h" @@ -12,6 +13,7 @@ namespace node { struct RealmSerializeInfo { std::vector persistent_values; + std::vector native_objects; SnapshotIndex context; friend std::ostream& operator<<(std::ostream& o, const RealmSerializeInfo& i); @@ -45,7 +47,7 @@ class Realm : public MemoryRetainer { Realm(Environment* env, v8::Local context, const RealmSerializeInfo* realm_info); - ~Realm() = default; + ~Realm(); Realm(const Realm&) = delete; Realm& operator=(const Realm&) = delete; @@ -66,11 +68,32 @@ class Realm : public MemoryRetainer { v8::MaybeLocal BootstrapNode(); v8::MaybeLocal RunBootstrapping(); + inline void AddCleanupHook(CleanupQueue::Callback cb, void* arg); + inline void RemoveCleanupHook(CleanupQueue::Callback cb, void* arg); + inline bool HasCleanupHooks() const; + void RunCleanup(); + + template + void ForEachBaseObject(T&& iterator) const; + + void PrintInfoForSnapshot(); + void VerifyNoStrongBaseObjects(); + + inline IsolateData* isolate_data() const; inline Environment* env() const; inline v8::Isolate* isolate() const; inline v8::Local context() const; inline bool has_run_bootstrapping_code() const; + // The BaseObject count is a debugging helper that makes sure that there are + // no memory leaks caused by BaseObjects staying alive longer than expected + // (in particular, no circular BaseObjectPtr references). + inline void modify_base_object_count(int64_t delta); + inline int64_t base_object_count() const; + + // Base object count created after the bootstrap of the realm. + inline int64_t base_object_created_after_bootstrap() const; + #define V(PropertyName, TypeName) \ inline v8::Local PropertyName() const; \ inline void set_##PropertyName(v8::Local value); @@ -88,6 +111,11 @@ class Realm : public MemoryRetainer { v8::Global context_; bool has_run_bootstrapping_code_ = false; + int64_t base_object_count_ = 0; + int64_t base_object_created_by_bootstrap_ = 0; + + CleanupQueue cleanup_queue_; + #define V(PropertyName, TypeName) v8::Global PropertyName##_; PER_REALM_STRONG_PERSISTENT_VALUES(V) #undef V diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index 859417d9fb5fd7..cb42a464c2ac0a 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -102,6 +102,9 @@ std::ostream& operator<<(std::ostream& output, const RealmSerializeInfo& i) { << "// -- persistent_values begins --\n" << i.persistent_values << ",\n" << "// -- persistent_values ends --\n" + << "// -- native_objects begins --\n" + << i.native_objects << ",\n" + << "// -- native_objects ends --\n" << i.context << ", // context\n" << "}"; return output; @@ -109,9 +112,6 @@ std::ostream& operator<<(std::ostream& output, const RealmSerializeInfo& i) { std::ostream& operator<<(std::ostream& output, const EnvSerializeInfo& i) { output << "{\n" - << "// -- native_objects begins --\n" - << i.native_objects << ",\n" - << "// -- native_objects ends --\n" << "// -- builtins begins --\n" << i.builtins << ",\n" << "// -- builtins ends --\n" @@ -705,6 +705,7 @@ RealmSerializeInfo FileReader::Read() { per_process::Debug(DebugCategory::MKSNAPSHOT, "Read()\n"); RealmSerializeInfo result; result.persistent_values = ReadVector(); + result.native_objects = ReadVector(); result.context = Read(); return result; } @@ -718,6 +719,7 @@ size_t FileWriter::Write(const RealmSerializeInfo& data) { // Use += here to ensure order of evaluation. size_t written_total = WriteVector(data.persistent_values); + written_total += WriteVector(data.native_objects); written_total += Write(data.context); Debug("Write() wrote %d bytes\n", written_total); @@ -728,7 +730,6 @@ template <> EnvSerializeInfo FileReader::Read() { per_process::Debug(DebugCategory::MKSNAPSHOT, "Read()\n"); EnvSerializeInfo result; - result.native_objects = ReadVector(); result.builtins = ReadVector(); result.async_hooks = Read(); result.tick_info = Read(); @@ -750,8 +751,7 @@ size_t FileWriter::Write(const EnvSerializeInfo& data) { } // Use += here to ensure order of evaluation. - size_t written_total = WriteVector(data.native_objects); - written_total += WriteVector(data.builtins); + size_t written_total = WriteVector(data.builtins); written_total += Write(data.async_hooks); written_total += Write(data.tick_info); written_total += Write(data.immediate_info); @@ -1194,7 +1194,7 @@ int SnapshotBuilder::Generate(SnapshotData* out, } if (per_process::enabled_debug_list.enabled(DebugCategory::MKSNAPSHOT)) { - env->PrintAllBaseObjects(); + env->ForEachRealm([](Realm* realm) { realm->PrintInfoForSnapshot(); }); printf("Environment = %p\n", env); } @@ -1400,11 +1400,13 @@ StartupData SerializeNodeContextInternalFields(Local holder, static_cast(info->length)}; } -void SerializeSnapshotableObjects(Environment* env, +void SerializeSnapshotableObjects(Realm* realm, SnapshotCreator* creator, - EnvSerializeInfo* info) { + RealmSerializeInfo* info) { + HandleScope scope(realm->isolate()); + Local context = realm->context(); uint32_t i = 0; - env->ForEachBaseObject([&](BaseObject* obj) { + realm->ForEachBaseObject([&](BaseObject* obj) { // If there are any BaseObjects that are not snapshotable left // during context serialization, V8 would crash due to unregistered // global handles and print detailed information about them. @@ -1422,8 +1424,8 @@ void SerializeSnapshotableObjects(Environment* env, *(ptr->object()), type_name); - if (ptr->PrepareForSerialization(env->context(), creator)) { - SnapshotIndex index = creator->AddData(env->context(), obj->object()); + if (ptr->PrepareForSerialization(context, creator)) { + SnapshotIndex index = creator->AddData(context, obj->object()); per_process::Debug(DebugCategory::MKSNAPSHOT, "Serialized with index=%d\n", static_cast(index)); diff --git a/src/node_snapshotable.h b/src/node_snapshotable.h index 6ff8c14898850f..39a2a71f45bd49 100644 --- a/src/node_snapshotable.h +++ b/src/node_snapshotable.h @@ -10,7 +10,7 @@ namespace node { class Environment; -struct EnvSerializeInfo; +struct RealmSerializeInfo; struct SnapshotData; class ExternalReferenceRegistry; @@ -131,9 +131,9 @@ void DeserializeNodeInternalFields(v8::Local holder, int index, v8::StartupData payload, void* env); -void SerializeSnapshotableObjects(Environment* env, +void SerializeSnapshotableObjects(Realm* realm, v8::SnapshotCreator* creator, - EnvSerializeInfo* info); + RealmSerializeInfo* info); } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/test/cctest/test_base_object_ptr.cc b/test/cctest/test_base_object_ptr.cc index 52a1e2aff79dde..c25a267a446096 100644 --- a/test/cctest/test_base_object_ptr.cc +++ b/test/cctest/test_base_object_ptr.cc @@ -1,6 +1,7 @@ +#include "base_object-inl.h" #include "gtest/gtest.h" #include "node.h" -#include "base_object-inl.h" +#include "node_realm-inl.h" #include "node_test_fixture.h" using node::BaseObject; @@ -9,6 +10,7 @@ using node::BaseObjectWeakPtr; using node::Environment; using node::MakeBaseObject; using node::MakeDetachedBaseObject; +using node::Realm; using v8::HandleScope; using v8::Isolate; using v8::Local; @@ -46,13 +48,14 @@ TEST_F(BaseObjectPtrTest, ScopedDetached) { const Argv argv; Env env_{handle_scope, argv}; Environment* env = *env_; + Realm* realm = env->principal_realm(); - EXPECT_EQ(env->base_object_created_after_bootstrap(), 0); + EXPECT_EQ(realm->base_object_created_after_bootstrap(), 0); { BaseObjectPtr ptr = DummyBaseObject::NewDetached(env); - EXPECT_EQ(env->base_object_created_after_bootstrap(), 1); + EXPECT_EQ(realm->base_object_created_after_bootstrap(), 1); } - EXPECT_EQ(env->base_object_created_after_bootstrap(), 0); + EXPECT_EQ(realm->base_object_created_after_bootstrap(), 0); } TEST_F(BaseObjectPtrTest, ScopedDetachedWithWeak) { @@ -60,17 +63,18 @@ TEST_F(BaseObjectPtrTest, ScopedDetachedWithWeak) { const Argv argv; Env env_{handle_scope, argv}; Environment* env = *env_; + Realm* realm = env->principal_realm(); BaseObjectWeakPtr weak_ptr; - EXPECT_EQ(env->base_object_created_after_bootstrap(), 0); + EXPECT_EQ(realm->base_object_created_after_bootstrap(), 0); { BaseObjectPtr ptr = DummyBaseObject::NewDetached(env); weak_ptr = ptr; - EXPECT_EQ(env->base_object_created_after_bootstrap(), 1); + EXPECT_EQ(realm->base_object_created_after_bootstrap(), 1); } EXPECT_EQ(weak_ptr.get(), nullptr); - EXPECT_EQ(env->base_object_created_after_bootstrap(), 0); + EXPECT_EQ(realm->base_object_created_after_bootstrap(), 0); } TEST_F(BaseObjectPtrTest, Undetached) { @@ -78,16 +82,17 @@ TEST_F(BaseObjectPtrTest, Undetached) { const Argv argv; Env env_{handle_scope, argv}; Environment* env = *env_; + Realm* realm = env->principal_realm(); node::AddEnvironmentCleanupHook( isolate_, [](void* arg) { - EXPECT_EQ(static_cast(arg)->base_object_count(), 0); + EXPECT_EQ(static_cast(arg)->base_object_count(), 0); }, - env); + realm); BaseObjectPtr ptr = DummyBaseObject::New(env); - EXPECT_EQ(env->base_object_created_after_bootstrap(), 1); + EXPECT_EQ(realm->base_object_created_after_bootstrap(), 1); } TEST_F(BaseObjectPtrTest, GCWeak) { @@ -95,6 +100,7 @@ TEST_F(BaseObjectPtrTest, GCWeak) { const Argv argv; Env env_{handle_scope, argv}; Environment* env = *env_; + Realm* realm = env->principal_realm(); BaseObjectWeakPtr weak_ptr; @@ -104,21 +110,21 @@ TEST_F(BaseObjectPtrTest, GCWeak) { weak_ptr = ptr; ptr->MakeWeak(); - EXPECT_EQ(env->base_object_created_after_bootstrap(), 1); + EXPECT_EQ(realm->base_object_created_after_bootstrap(), 1); EXPECT_EQ(weak_ptr.get(), ptr.get()); EXPECT_EQ(weak_ptr->persistent().IsWeak(), false); ptr.reset(); } - EXPECT_EQ(env->base_object_created_after_bootstrap(), 1); + EXPECT_EQ(realm->base_object_created_after_bootstrap(), 1); EXPECT_NE(weak_ptr.get(), nullptr); EXPECT_EQ(weak_ptr->persistent().IsWeak(), true); v8::V8::SetFlagsFromString("--expose-gc"); isolate_->RequestGarbageCollectionForTesting(Isolate::kFullGarbageCollection); - EXPECT_EQ(env->base_object_created_after_bootstrap(), 0); + EXPECT_EQ(realm->base_object_created_after_bootstrap(), 0); EXPECT_EQ(weak_ptr.get(), nullptr); } @@ -127,9 +133,10 @@ TEST_F(BaseObjectPtrTest, Moveable) { const Argv argv; Env env_{handle_scope, argv}; Environment* env = *env_; + Realm* realm = env->principal_realm(); BaseObjectPtr ptr = DummyBaseObject::NewDetached(env); - EXPECT_EQ(env->base_object_created_after_bootstrap(), 1); + EXPECT_EQ(realm->base_object_created_after_bootstrap(), 1); BaseObjectWeakPtr weak_ptr { ptr }; EXPECT_EQ(weak_ptr.get(), ptr.get()); @@ -140,12 +147,12 @@ TEST_F(BaseObjectPtrTest, Moveable) { BaseObjectWeakPtr weak_ptr2 = std::move(weak_ptr); EXPECT_EQ(weak_ptr2.get(), ptr2.get()); EXPECT_EQ(weak_ptr.get(), nullptr); - EXPECT_EQ(env->base_object_created_after_bootstrap(), 1); + EXPECT_EQ(realm->base_object_created_after_bootstrap(), 1); ptr2.reset(); EXPECT_EQ(weak_ptr2.get(), nullptr); - EXPECT_EQ(env->base_object_created_after_bootstrap(), 0); + EXPECT_EQ(realm->base_object_created_after_bootstrap(), 0); } TEST_F(BaseObjectPtrTest, NestedClasses) { @@ -165,18 +172,19 @@ TEST_F(BaseObjectPtrTest, NestedClasses) { const Argv argv; Env env_{handle_scope, argv}; Environment* env = *env_; + Realm* realm = env->principal_realm(); node::AddEnvironmentCleanupHook( isolate_, [](void* arg) { - EXPECT_EQ(static_cast(arg)->base_object_count(), 0); + EXPECT_EQ(static_cast(arg)->base_object_count(), 0); }, - env); + realm); ObjectWithPtr* obj = new ObjectWithPtr(env, DummyBaseObject::MakeJSObject(env)); obj->ptr1 = DummyBaseObject::NewDetached(env); obj->ptr2 = DummyBaseObject::New(env); - EXPECT_EQ(env->base_object_created_after_bootstrap(), 3); + EXPECT_EQ(realm->base_object_created_after_bootstrap(), 3); } diff --git a/test/pummel/test-heapdump-env.js b/test/pummel/test-heapdump-env.js index c33e815092fc4d..2e9f75cbfff3ba 100644 --- a/test/pummel/test-heapdump-env.js +++ b/test/pummel/test-heapdump-env.js @@ -21,11 +21,16 @@ validateSnapshotNodes('Node / Environment', [{ ] }]); -validateSnapshotNodes('Node / CleanupQueue', [{ - children: [ - { node_name: 'Node / ContextifyScript' }, - ] -}]); +validateSnapshotNodes('Node / CleanupQueue', [ + // The first one is the cleanup_queue of the Environment. + {}, + // The second one is the cleanup_queue of the principal realm. + { + children: [ + { node_name: 'Node / ContextifyScript' }, + ] + }, +]); validateSnapshotNodes('Node / Realm', [{ children: [