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 ff8d3952cf20a3..f5d00ba45d63f3 100644 --- a/src/env.cc +++ b/src/env.cc @@ -595,6 +595,20 @@ void Environment::AssignToContext(Local context, TrackContext(context); } +void Environment::UnassignToContext(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, @@ -820,7 +834,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); } @@ -890,9 +903,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(); @@ -915,10 +928,6 @@ Environment::~Environment() { addon.Close(); } } - - for (auto realm : shadow_realms_) { - realm->OnEnvironmentDestruct(); - } } void Environment::InitializeLibuv() { @@ -1714,6 +1723,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); @@ -1724,8 +1736,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 5359436be31e76..4e91e303f51bac 100644 --- a/src/env.h +++ b/src/env.h @@ -645,8 +645,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 UnassignToContext(v8::Local context); void TrackShadowRealm(shadow_realm::ShadowRealm* realm); void UntrackShadowRealm(shadow_realm::ShadowRealm* realm); @@ -998,6 +997,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 7f9f71ba74223a..f237b608641635 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()->UnassignToContext(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..11d2476dbed385 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_->UnassignToContext(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..e213414791cd10 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_->UnassignToContext(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 078e155fc49374..b56a07c7091f20 100644 --- a/src/node_url.cc +++ b/src/node_url.cc @@ -41,6 +41,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..8a191b8639aa36 --- /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); + })); +}