diff --git a/src/env.cc b/src/env.cc index f6aeb75933bd59..6e264f7d7bbd16 100644 --- a/src/env.cc +++ b/src/env.cc @@ -259,13 +259,9 @@ 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); -} +// TrackShadowRealm/UntrackShadowRealm removed - they created strong references +// that prevented Shadow Realms from being garbage collected. +// Fixes: https://github.com/nodejs/node/issues/47353 AsyncHooks::DefaultTriggerAsyncIdScope::DefaultTriggerAsyncIdScope( Environment* env, double default_trigger_async_id) @@ -1044,8 +1040,8 @@ Environment::~Environment() { inspector_agent_.reset(); #endif - // Sub-realms should have been cleared with Environment's cleanup. - DCHECK_EQ(shadow_realms_.size(), 0); + // Shadow realms are now managed via weak references and cleanup hooks, + // not tracked in a set. Fixes: https://github.com/nodejs/node/issues/47353 principal_realm_.reset(); if (trace_state_observer_) { @@ -2232,7 +2228,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_); + // shadow_realms_ removed - no longer tracking to avoid strong references // 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 6098541a9c70a1..c8e9cf3377bcb7 100644 --- a/src/env.h +++ b/src/env.h @@ -691,8 +691,9 @@ class Environment final : public MemoryRetainer { Realm* realm, const ContextInfo& info); void UnassignFromContext(v8::Local context); - void TrackShadowRealm(shadow_realm::ShadowRealm* realm); - void UntrackShadowRealm(shadow_realm::ShadowRealm* realm); + // TrackShadowRealm/UntrackShadowRealm removed - they created strong + // references that prevented Shadow Realms from being garbage collected. + // Fixes: https://github.com/nodejs/node/issues/47353 void StartProfilerIdleNotifier(); @@ -1117,7 +1118,9 @@ class Environment final : public MemoryRetainer { size_t async_callback_scope_depth_ = 0; std::vector destroy_async_id_list_; - std::unordered_set shadow_realms_; + // shadow_realms_ member removed - tracking shadow realms in a set created + // strong C++ references that prevented garbage collection. + // Fixes: https://github.com/nodejs/node/issues/47353 #if HAVE_INSPECTOR std::unique_ptr coverage_connection_; diff --git a/src/node_shadow_realm.cc b/src/node_shadow_realm.cc index 1273555b50c80b..7674374e19f65a 100644 --- a/src/node_shadow_realm.cc +++ b/src/node_shadow_realm.cc @@ -79,7 +79,10 @@ ShadowRealm::ShadowRealm(Environment* env) context_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter); CreateProperties(); - env->TrackShadowRealm(this); + // Don't track shadow realms in environment's set - that creates a strong + // reference preventing GC. The cleanup hook and weak callback are sufficient + // for proper lifecycle management. + // Fixes: https://github.com/nodejs/node/issues/47353 env->AddCleanupHook(DeleteMe, this); } @@ -88,7 +91,8 @@ ShadowRealm::~ShadowRealm() { RunCleanup(); } - env_->UntrackShadowRealm(this); + // Removed UntrackShadowRealm() call - we no longer track shadow realms + // in environment's set to avoid creating strong references that prevent GC. if (context_.IsEmpty()) { // This most likely happened because the weak callback cleared it. diff --git a/test/pummel/test-heapdump-shadow-realm.js b/test/pummel/test-heapdump-shadow-realm.js index 66c32986b4c8cd..3abe85d03abb7b 100644 --- a/test/pummel/test-heapdump-shadow-realm.js +++ b/test/pummel/test-heapdump-shadow-realm.js @@ -30,10 +30,12 @@ function createRealms() { } function validateHeap() { - validateByRetainingPath('Node / Environment', [ - { node_name: 'Node / shadow_realms', edge_name: 'shadow_realms' }, - { node_name: 'Node / ShadowRealm' }, - ]); + // Validate that ShadowRealm appears in the heap snapshot. + // We don't validate a specific retaining path since ShadowRealms are now + // managed via weak references and cleanup hooks rather than a strong + // shadow_realms_ set (which was removed to fix the memory leak). + const nodes = validateByRetainingPath('Node / ShadowRealm', []); + assert(nodes.length > 0, 'Expected at least one ShadowRealm in heap snapshot'); } createRealms();