diff --git a/src/base_object-inl.h b/src/base_object-inl.h index feaeab306acefb..99a438a4963717 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -289,6 +289,12 @@ template BaseObjectPtr MakeBaseObject(Args&&... args) { return BaseObjectPtr(new T(std::forward(args)...)); } +template +BaseObjectWeakPtr MakeWeakBaseObject(Args&&... args) { + T* target = new T(std::forward(args)...); + target->MakeWeak(); + return BaseObjectWeakPtr(target); +} template BaseObjectPtr MakeDetachedBaseObject(Args&&... args) { diff --git a/src/base_object.h b/src/base_object.h index 96f661c41284dc..cfd6af673cec97 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -302,6 +302,10 @@ using BaseObjectWeakPtr = BaseObjectPtrImpl; template inline BaseObjectPtr MakeBaseObject(Args&&... args); // Create a BaseObject instance and return a pointer to it. +// This variant makes the object a weak GC root by default. +template +inline BaseObjectWeakPtr MakeWeakBaseObject(Args&&... args); +// Create a BaseObject instance and return a pointer to it. // This variant detaches the object by default, meaning that the caller fully // owns it, and once the last BaseObjectPtr to it is destroyed, the object // itself is also destroyed. diff --git a/src/env.cc b/src/env.cc index 61e334bb984a57..0401e5db86fb99 100644 --- a/src/env.cc +++ b/src/env.cc @@ -594,6 +594,20 @@ void Environment::AssignToContext(Local context, TrackContext(context); } +void Environment::UnassignFromContext(Local context) { + if (!context.IsEmpty()) { + context->SetAlignedPointerInEmbedderData(ContextEmbedderIndex::kEnvironment, + nullptr); + context->SetAlignedPointerInEmbedderData(ContextEmbedderIndex::kRealm, + nullptr); + context->SetAlignedPointerInEmbedderData( + ContextEmbedderIndex::kBindingDataStoreIndex, nullptr); + context->SetAlignedPointerInEmbedderData( + ContextEmbedderIndex::kContextifyContext, nullptr); + } + UntrackContext(context); +} + void Environment::TryLoadAddon( const char* filename, int flags, @@ -819,7 +833,6 @@ void Environment::InitializeMainContext(Local context, const EnvSerializeInfo* env_info) { principal_realm_ = std::make_unique( this, context, MAYBE_FIELD_PTR(env_info, principal_realm)); - AssignToContext(context, principal_realm_.get(), ContextInfo("")); if (env_info != nullptr) { DeserializeProperties(env_info); } @@ -889,9 +902,9 @@ Environment::~Environment() { inspector_agent_.reset(); #endif - ctx->SetAlignedPointerInEmbedderData(ContextEmbedderIndex::kEnvironment, - nullptr); - ctx->SetAlignedPointerInEmbedderData(ContextEmbedderIndex::kRealm, nullptr); + // Sub-realms should have been cleared with Environment's cleanup. + DCHECK_EQ(shadow_realms_.size(), 0); + principal_realm_.reset(); if (trace_state_observer_) { tracing::AgentWriterHandle* writer = GetTracingAgentWriter(); @@ -914,10 +927,6 @@ Environment::~Environment() { addon.Close(); } } - - for (auto realm : shadow_realms_) { - realm->OnEnvironmentDestruct(); - } } void Environment::InitializeLibuv() { @@ -1713,6 +1722,9 @@ void Environment::DeserializeProperties(const EnvSerializeInfo* info) { std::cerr << *info << "\n"; } + // Deserialize the realm's properties before running the deserialize + // requests as the requests may need to access the realm's properties. + principal_realm_->DeserializeProperties(&info->principal_realm); RunDeserializeRequests(); async_hooks_.Deserialize(ctx); @@ -1723,8 +1735,6 @@ void Environment::DeserializeProperties(const EnvSerializeInfo* info) { exit_info_.Deserialize(ctx); stream_base_state_.Deserialize(ctx); should_abort_on_uncaught_toggle_.Deserialize(ctx); - - principal_realm_->DeserializeProperties(&info->principal_realm); } uint64_t GuessMemoryAvailableToTheProcess() { diff --git a/src/env.h b/src/env.h index b31cd12dfe2ec3..dc9ba3baeb2b4b 100644 --- a/src/env.h +++ b/src/env.h @@ -649,8 +649,7 @@ class Environment : public MemoryRetainer { void AssignToContext(v8::Local context, Realm* realm, const ContextInfo& info); - void TrackContext(v8::Local context); - void UntrackContext(v8::Local context); + void UnassignFromContext(v8::Local context); void TrackShadowRealm(shadow_realm::ShadowRealm* realm); void UntrackShadowRealm(shadow_realm::ShadowRealm* realm); @@ -1002,6 +1001,8 @@ class Environment : public MemoryRetainer { private: inline void ThrowError(v8::Local (*fun)(v8::Local), const char* errmsg); + void TrackContext(v8::Local context); + void UntrackContext(v8::Local context); std::list loaded_addons_; v8::Isolate* const isolate_; diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index b62bdea9c5e5e1..5c66757afd1a7a 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -172,12 +172,12 @@ static bool InspectorEnabled(Environment* env) { } void SetConsoleExtensionInstaller(const FunctionCallbackInfo& info) { - auto env = Environment::GetCurrent(info); + Realm* realm = Realm::GetCurrent(info); CHECK_EQ(info.Length(), 1); CHECK(info[0]->IsFunction()); - env->set_inspector_console_extension_installer(info[0].As()); + realm->set_inspector_console_extension_installer(info[0].As()); } void CallAndPauseOnStart(const FunctionCallbackInfo& args) { diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 6713f17f65314f..f8bd2d9b7cd71d 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -163,7 +163,7 @@ ContextifyContext::~ContextifyContext() { Isolate* isolate = env()->isolate(); HandleScope scope(isolate); - env()->UntrackContext(PersistentToLocal::Weak(isolate, context_)); + env()->UnassignFromContext(PersistentToLocal::Weak(isolate, context_)); context_.Reset(); } diff --git a/src/node_realm-inl.h b/src/node_realm-inl.h index c6a85d24d1767c..748ecfe262afa2 100644 --- a/src/node_realm-inl.h +++ b/src/node_realm-inl.h @@ -86,8 +86,13 @@ inline T* Realm::AddBindingData(v8::Local context, Args&&... args) { DCHECK_EQ(GetCurrent(context), this); // This won't compile if T is not a BaseObject subclass. - BaseObjectPtr item = - MakeDetachedBaseObject(this, target, std::forward(args)...); + static_assert(std::is_base_of_v); + // The binding data must be weak so that it won't keep the realm reachable + // from strong GC roots indefinitely. The wrapper object of binding data + // should be referenced from JavaScript, thus the binding data should be + // reachable throughout the lifetime of the realm. + BaseObjectWeakPtr item = + MakeWeakBaseObject(this, target, std::forward(args)...); DCHECK_EQ(context->GetAlignedPointerFromEmbedderData( ContextEmbedderIndex::kBindingDataStoreIndex), &binding_data_store_); diff --git a/src/node_realm.cc b/src/node_realm.cc index 80b2f9d2784611..7101b23e347c7c 100644 --- a/src/node_realm.cc +++ b/src/node_realm.cc @@ -21,6 +21,7 @@ using v8::Value; Realm::Realm(Environment* env, v8::Local context, Kind kind) : env_(env), isolate_(context->GetIsolate()), kind_(kind) { context_.Reset(isolate_, context); + env->AssignToContext(context, this, ContextInfo("")); } Realm::~Realm() { @@ -278,11 +279,15 @@ v8::Local Realm::context() const { return PersistentToLocal::Strong(context_); } +// Per-realm strong value accessors. The per-realm values should avoid being +// accessed across realms. #define V(PropertyName, TypeName) \ v8::Local PrincipalRealm::PropertyName() const { \ return PersistentToLocal::Strong(PropertyName##_); \ } \ void PrincipalRealm::set_##PropertyName(v8::Local value) { \ + DCHECK_IMPLIES(!value.IsEmpty(), \ + isolate()->GetCurrentContext() == context()); \ PropertyName##_.Reset(isolate(), value); \ } PER_REALM_STRONG_PERSISTENT_VALUES(V) @@ -300,6 +305,13 @@ PrincipalRealm::PrincipalRealm(Environment* env, } } +PrincipalRealm::~PrincipalRealm() { + DCHECK(!context_.IsEmpty()); + + HandleScope handle_scope(isolate()); + env_->UnassignFromContext(context()); +} + MaybeLocal PrincipalRealm::BootstrapRealm() { HandleScope scope(isolate_); diff --git a/src/node_realm.h b/src/node_realm.h index a588b02abf3d8b..6cca9d5041d3fb 100644 --- a/src/node_realm.h +++ b/src/node_realm.h @@ -21,9 +21,9 @@ struct RealmSerializeInfo { friend std::ostream& operator<<(std::ostream& o, const RealmSerializeInfo& i); }; -using BindingDataStore = std::array, - static_cast( - BindingDataType::kBindingDataTypeCount)>; +using BindingDataStore = + std::array, + static_cast(BindingDataType::kBindingDataTypeCount)>; /** * node::Realm is a container for a set of JavaScript objects and functions @@ -162,7 +162,7 @@ class PrincipalRealm : public Realm { PrincipalRealm(Environment* env, v8::Local context, const RealmSerializeInfo* realm_info); - ~PrincipalRealm() = default; + ~PrincipalRealm(); SET_MEMORY_INFO_NAME(PrincipalRealm) SET_SELF_SIZE(PrincipalRealm) diff --git a/src/node_shadow_realm.cc b/src/node_shadow_realm.cc index e9fcf4f98ab1de..1cf0a57617b8b2 100644 --- a/src/node_shadow_realm.cc +++ b/src/node_shadow_realm.cc @@ -5,6 +5,7 @@ namespace node { namespace shadow_realm { using v8::Context; +using v8::EscapableHandleScope; using v8::HandleScope; using v8::Local; using v8::MaybeLocal; @@ -15,7 +16,6 @@ using TryCatchScope = node::errors::TryCatchScope; // static ShadowRealm* ShadowRealm::New(Environment* env) { ShadowRealm* realm = new ShadowRealm(env); - env->AssignToContext(realm->context(), realm, ContextInfo("")); // We do not expect the realm bootstrapping to throw any // exceptions. If it does, exit the current Node.js instance. @@ -31,9 +31,10 @@ ShadowRealm* ShadowRealm::New(Environment* env) { MaybeLocal HostCreateShadowRealmContextCallback( Local initiator_context) { Environment* env = Environment::GetCurrent(initiator_context); + EscapableHandleScope scope(env->isolate()); ShadowRealm* realm = ShadowRealm::New(env); if (realm != nullptr) { - return realm->context(); + return scope.Escape(realm->context()); } return MaybeLocal(); } @@ -41,29 +42,51 @@ MaybeLocal HostCreateShadowRealmContextCallback( // static void ShadowRealm::WeakCallback(const v8::WeakCallbackInfo& data) { ShadowRealm* realm = data.GetParameter(); + realm->context_.Reset(); + + // Yield to pending weak callbacks before deleting the realm. + // This is necessary to avoid cleaning up base objects before their scheduled + // weak callbacks are invoked, which can lead to accessing to v8 apis during + // the first pass of the weak callback. + realm->env()->SetImmediate([realm](Environment* env) { delete realm; }); + // Remove the cleanup hook to avoid deleting the realm again. + realm->env()->RemoveCleanupHook(DeleteMe, realm); +} + +// static +void ShadowRealm::DeleteMe(void* data) { + ShadowRealm* realm = static_cast(data); + // Clear the context handle to avoid invoking the weak callback again. + // Also, the context internal slots are cleared and the context is no longer + // reference to the realm. delete realm; } ShadowRealm::ShadowRealm(Environment* env) : Realm(env, NewContext(env->isolate()), kShadowRealm) { - env->TrackShadowRealm(this); context_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter); CreateProperties(); + + env->TrackShadowRealm(this); + env->AddCleanupHook(DeleteMe, this); } ShadowRealm::~ShadowRealm() { while (HasCleanupHooks()) { RunCleanup(); } - if (env_ != nullptr) { - env_->UntrackShadowRealm(this); + + env_->UntrackShadowRealm(this); + + if (context_.IsEmpty()) { + // This most likely happened because the weak callback cleared it. + return; } -} -void ShadowRealm::OnEnvironmentDestruct() { - CHECK_NOT_NULL(env_); - env_ = nullptr; // This means that the shadow realm has out-lived the - // environment. + { + HandleScope handle_scope(isolate()); + env_->UnassignFromContext(context()); + } } v8::Local ShadowRealm::context() const { diff --git a/src/node_shadow_realm.h b/src/node_shadow_realm.h index 58d6581938522c..c2a6f2c8cb8d05 100644 --- a/src/node_shadow_realm.h +++ b/src/node_shadow_realm.h @@ -24,13 +24,12 @@ class ShadowRealm : public Realm { PER_REALM_STRONG_PERSISTENT_VALUES(V) #undef V - void OnEnvironmentDestruct(); - protected: v8::MaybeLocal BootstrapRealm() override; private: static void WeakCallback(const v8::WeakCallbackInfo& data); + static void DeleteMe(void* data); explicit ShadowRealm(Environment* env); ~ShadowRealm(); diff --git a/src/node_url.cc b/src/node_url.cc index d1ae2e9b502755..fceb3943901a3b 100644 --- a/src/node_url.cc +++ b/src/node_url.cc @@ -40,6 +40,7 @@ BindingData::BindingData(Realm* realm, v8::Local object) FIXED_ONE_BYTE_STRING(realm->isolate(), "urlComponents"), url_components_buffer_.GetJSArray()) .Check(); + url_components_buffer_.MakeWeak(); } bool BindingData::PrepareForSerialization(v8::Local context, diff --git a/test/known_issues/known_issues.status b/test/known_issues/known_issues.status index 12a6a9f070aab6..af3e24a1bbd959 100644 --- a/test/known_issues/known_issues.status +++ b/test/known_issues/known_issues.status @@ -11,9 +11,6 @@ prefix known_issues # foreseeable future. The test itself is flaky and skipped. It # serves as a demonstration of the issue only. test-vm-timeout-escape-queuemicrotask: SKIP -# Skipping it because it crashes out of OOM instead of exiting. -# https://github.com/nodejs/node/issues/47353 -test-shadow-realm-gc: SKIP [$system==win32] diff --git a/test/known_issues/test-shadow-realm-gc.js b/test/known_issues/test-shadow-realm-gc.js deleted file mode 100644 index cf15324e5cec06..00000000000000 --- a/test/known_issues/test-shadow-realm-gc.js +++ /dev/null @@ -1,13 +0,0 @@ -// Flags: --experimental-shadow-realm --max-old-space-size=20 -'use strict'; - -/** - * Verifying ShadowRealm instances can be correctly garbage collected. - */ - -require('../common'); - -for (let i = 0; i < 1000; i++) { - const realm = new ShadowRealm(); - realm.evaluate('new TextEncoder(); 1;'); -} diff --git a/test/parallel/test-shadow-realm-gc.js b/test/parallel/test-shadow-realm-gc.js new file mode 100644 index 00000000000000..83f793fd89222f --- /dev/null +++ b/test/parallel/test-shadow-realm-gc.js @@ -0,0 +1,22 @@ +// Flags: --experimental-shadow-realm --max-old-space-size=20 +'use strict'; + +/** + * Verifying ShadowRealm instances can be correctly garbage collected. + */ + +const common = require('../common'); +const assert = require('assert'); +const { isMainThread, Worker } = require('worker_threads'); + +for (let i = 0; i < 100; i++) { + const realm = new ShadowRealm(); + realm.evaluate('new TextEncoder(); 1;'); +} + +if (isMainThread) { + const worker = new Worker(__filename); + worker.on('exit', common.mustCall((code) => { + assert.strictEqual(code, 0); + })); +} diff --git a/test/pummel/test-heapdump-shadow-realm.js b/test/pummel/test-heapdump-shadow-realm.js new file mode 100644 index 00000000000000..76bd425e1c8674 --- /dev/null +++ b/test/pummel/test-heapdump-shadow-realm.js @@ -0,0 +1,32 @@ +// Flags: --experimental-shadow-realm --expose-internals +'use strict'; +require('../common'); +const { validateSnapshotNodes } = require('../common/heap'); + +validateSnapshotNodes('Node / ShadowRealm', []); +const realm = new ShadowRealm(); +{ + // Create a bunch of un-referenced ShadowRealms to make sure the heap + // snapshot can handle it. + for (let i = 0; i < 100; i++) { + const realm = new ShadowRealm(); + realm.evaluate('undefined'); + } +} +validateSnapshotNodes('Node / Environment', [ + { + children: [ + { node_name: 'Node / shadow_realms', edge_name: 'shadow_realms' }, + ], + }, +]); +validateSnapshotNodes('Node / shadow_realms', [ + { + children: [ + { node_name: 'Node / ShadowRealm' }, + ], + }, +]); + +// Keep the realm alive. +realm.evaluate('undefined');