From 5250947a539780c33212dbaf4b7bf2c99c862272 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 6 Apr 2023 02:25:09 +0200 Subject: [PATCH] src: track ShadowRealm native objects correctly in the heap snapshot Previously the native ShadowRealm objects were never actually tracked in the heap snapshot - the tracking starts with the Environment, we don't call the tracking methods unless it's reachable natively from the Environment. This patch adds a set in the Environment for tracking shadow realms in the heap snapshot. Drive-by: remove redundant MemoryInfo() overrides from the derived classes of Realm and remove the tracking of `env` from `Realm::MemoryInfo()` because the realms don't hold the Environment alive (even for the principal realm, it's the other way around). PR-URL: https://github.com/nodejs/node/pull/47389 Reviewed-By: Chengzhong Wu Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig --- src/env.cc | 14 ++++++++++++++ src/env.h | 6 ++++++ src/node_realm.cc | 5 ----- src/node_realm.h | 1 - src/node_shadow_realm.cc | 10 ++++++++-- src/node_shadow_realm.h | 3 ++- 6 files changed, 30 insertions(+), 9 deletions(-) diff --git a/src/env.cc b/src/env.cc index 8ebbcfb2525239..571a8ed5ce257f 100644 --- a/src/env.cc +++ b/src/env.cc @@ -11,6 +11,7 @@ #include "node_internals.h" #include "node_options-inl.h" #include "node_process-inl.h" +#include "node_shadow_realm.h" #include "node_v8_platform-inl.h" #include "node_worker.h" #include "req_wrap-inl.h" @@ -226,6 +227,14 @@ void Environment::UntrackContext(Local context) { } } +void Environment::TrackShadowRealm(shadow_realm::ShadowRealm* realm) { + shadow_realms_.insert(realm); +} + +void Environment::UntrackShadowRealm(shadow_realm::ShadowRealm* realm) { + shadow_realms_.erase(realm); +} + AsyncHooks::DefaultTriggerAsyncIdScope::DefaultTriggerAsyncIdScope( Environment* env, double default_trigger_async_id) : async_hooks_(env->async_hooks()) { @@ -901,6 +910,10 @@ Environment::~Environment() { addon.Close(); } } + + for (auto realm : shadow_realms_) { + realm->OnEnvironmentDestruct(); + } } void Environment::InitializeLibuv() { @@ -1896,6 +1909,7 @@ void Environment::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackField("timeout_info", timeout_info_); tracker->TrackField("tick_info", tick_info_); tracker->TrackField("principal_realm", principal_realm_); + tracker->TrackField("shadow_realms", shadow_realms_); // FIXME(joyeecheung): track other fields in Environment. // Currently MemoryTracker is unable to track these diff --git a/src/env.h b/src/env.h index 8b3596a6c5b358..a50e2a83e332cf 100644 --- a/src/env.h +++ b/src/env.h @@ -64,6 +64,9 @@ namespace node { +namespace shadow_realm { +class ShadowRealm; +} namespace contextify { class ContextifyScript; class CompiledFnEntry; @@ -643,6 +646,8 @@ class Environment : public MemoryRetainer { const ContextInfo& info); void TrackContext(v8::Local context); void UntrackContext(v8::Local context); + void TrackShadowRealm(shadow_realm::ShadowRealm* realm); + void UntrackShadowRealm(shadow_realm::ShadowRealm* realm); void StartProfilerIdleNotifier(); @@ -1020,6 +1025,7 @@ class Environment : public MemoryRetainer { size_t async_callback_scope_depth_ = 0; std::vector destroy_async_id_list_; + std::unordered_set shadow_realms_; #if HAVE_INSPECTOR std::unique_ptr coverage_connection_; diff --git a/src/node_realm.cc b/src/node_realm.cc index 3313c683e7e424..80b2f9d2784611 100644 --- a/src/node_realm.cc +++ b/src/node_realm.cc @@ -33,7 +33,6 @@ void Realm::MemoryInfo(MemoryTracker* tracker) const { PER_REALM_STRONG_PERSISTENT_VALUES(V) #undef V - tracker->TrackField("env", env_); tracker->TrackField("cleanup_queue", cleanup_queue_); tracker->TrackField("builtins_with_cache", builtins_with_cache); tracker->TrackField("builtins_without_cache", builtins_without_cache); @@ -301,10 +300,6 @@ PrincipalRealm::PrincipalRealm(Environment* env, } } -void PrincipalRealm::MemoryInfo(MemoryTracker* tracker) const { - Realm::MemoryInfo(tracker); -} - MaybeLocal PrincipalRealm::BootstrapRealm() { HandleScope scope(isolate_); diff --git a/src/node_realm.h b/src/node_realm.h index 4bff386baff227..d5b721002efb32 100644 --- a/src/node_realm.h +++ b/src/node_realm.h @@ -165,7 +165,6 @@ class PrincipalRealm : public Realm { SET_MEMORY_INFO_NAME(PrincipalRealm) SET_SELF_SIZE(PrincipalRealm) - void MemoryInfo(MemoryTracker* tracker) const override; #define V(PropertyName, TypeName) \ v8::Local PropertyName() const override; \ diff --git a/src/node_shadow_realm.cc b/src/node_shadow_realm.cc index bf39509d537757..e9fcf4f98ab1de 100644 --- a/src/node_shadow_realm.cc +++ b/src/node_shadow_realm.cc @@ -46,6 +46,7 @@ void ShadowRealm::WeakCallback(const v8::WeakCallbackInfo& data) { ShadowRealm::ShadowRealm(Environment* env) : Realm(env, NewContext(env->isolate()), kShadowRealm) { + env->TrackShadowRealm(this); context_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter); CreateProperties(); } @@ -54,10 +55,15 @@ ShadowRealm::~ShadowRealm() { while (HasCleanupHooks()) { RunCleanup(); } + if (env_ != nullptr) { + env_->UntrackShadowRealm(this); + } } -void ShadowRealm::MemoryInfo(MemoryTracker* tracker) const { - Realm::MemoryInfo(tracker); +void ShadowRealm::OnEnvironmentDestruct() { + CHECK_NOT_NULL(env_); + env_ = nullptr; // This means that the shadow realm has out-lived the + // environment. } v8::Local ShadowRealm::context() const { diff --git a/src/node_shadow_realm.h b/src/node_shadow_realm.h index cc76cbacdc4842..58d6581938522c 100644 --- a/src/node_shadow_realm.h +++ b/src/node_shadow_realm.h @@ -15,7 +15,6 @@ class ShadowRealm : public Realm { SET_MEMORY_INFO_NAME(ShadowRealm) SET_SELF_SIZE(ShadowRealm) - void MemoryInfo(MemoryTracker* tracker) const override; v8::Local context() const override; @@ -25,6 +24,8 @@ class ShadowRealm : public Realm { PER_REALM_STRONG_PERSISTENT_VALUES(V) #undef V + void OnEnvironmentDestruct(); + protected: v8::MaybeLocal BootstrapRealm() override;