From 5aa1ab35e9a7b90cf432100353253578fee951fb Mon Sep 17 00:00:00 2001 From: legendecas Date: Tue, 9 May 2023 01:29:16 +0800 Subject: [PATCH 1/2] src: remove aliased buffer weak callback An AliasedBuffer can be nested in a BaseObject and their weak callbacks invoke order are not guaranteed. Prevent aliased buffer from being accessed in its weak callback as its validness can be checked with the emptiness of the persistent handle. --- src/aliased_buffer-inl.h | 15 +++------------ src/aliased_buffer.h | 3 --- 2 files changed, 3 insertions(+), 15 deletions(-) diff --git a/src/aliased_buffer-inl.h b/src/aliased_buffer-inl.h index 782a125570d06d..f04fb7c2667016 100644 --- a/src/aliased_buffer-inl.h +++ b/src/aliased_buffer-inl.h @@ -70,8 +70,8 @@ AliasedBufferBase::AliasedBufferBase( count_(that.count_), byte_offset_(that.byte_offset_), buffer_(that.buffer_) { - DCHECK(is_valid()); js_array_ = v8::Global(that.isolate_, that.GetJSArray()); + DCHECK(is_valid()); } template @@ -126,19 +126,10 @@ void AliasedBufferBase::Release() { js_array_.Reset(); } -template -inline void AliasedBufferBase::WeakCallback( - const v8::WeakCallbackInfo>& data) { - AliasedBufferBase* buffer = data.GetParameter(); - DCHECK(buffer->is_valid()); - buffer->cleared_ = true; - buffer->js_array_.Reset(); -} - template inline void AliasedBufferBase::MakeWeak() { DCHECK(is_valid()); - js_array_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter); + js_array_.SetWeak(); } template @@ -223,7 +214,7 @@ void AliasedBufferBase::reserve(size_t new_capacity) { template inline bool AliasedBufferBase::is_valid() const { - return index_ == nullptr && !cleared_; + return index_ == nullptr && !js_array_.IsEmpty(); } template diff --git a/src/aliased_buffer.h b/src/aliased_buffer.h index 4c4f8ac21dfc4a..b847641f8faa15 100644 --- a/src/aliased_buffer.h +++ b/src/aliased_buffer.h @@ -173,14 +173,11 @@ class AliasedBufferBase : public MemoryRetainer { private: inline bool is_valid() const; - static inline void WeakCallback( - const v8::WeakCallbackInfo>& data); v8::Isolate* isolate_ = nullptr; size_t count_ = 0; size_t byte_offset_ = 0; NativeT* buffer_ = nullptr; v8::Global js_array_; - bool cleared_ = false; // Deserialize data const AliasedBufferIndex* index_ = nullptr; From 952cabc46281bc4be7c44c7b625deefe50e405f3 Mon Sep 17 00:00:00 2001 From: legendecas Date: Mon, 3 Apr 2023 00:04:57 +0800 Subject: [PATCH 2/2] src: make realm binding data store weak 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. --- src/base_object-inl.h | 6 ++++ src/base_object.h | 4 +++ src/env.cc | 30 ++++++++++------ src/env.h | 5 +-- src/inspector_js_api.cc | 4 +-- src/node_contextify.cc | 2 +- src/node_realm-inl.h | 9 +++-- src/node_realm.cc | 12 +++++++ src/node_realm.h | 8 ++--- src/node_shadow_realm.cc | 43 +++++++++++++++++------ src/node_shadow_realm.h | 3 +- src/node_url.cc | 1 + test/known_issues/known_issues.status | 3 -- test/known_issues/test-shadow-realm-gc.js | 13 ------- test/parallel/test-shadow-realm-gc.js | 22 ++++++++++++ test/pummel/test-heapdump-shadow-realm.js | 32 +++++++++++++++++ 16 files changed, 148 insertions(+), 49 deletions(-) delete mode 100644 test/known_issues/test-shadow-realm-gc.js create mode 100644 test/parallel/test-shadow-realm-gc.js create mode 100644 test/pummel/test-heapdump-shadow-realm.js 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');