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.

PR-URL: nodejs#47688
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

src/base_object-inl.h

@@ -289,6 +289,12 @@ template <typename T, typename... Args>
 BaseObjectPtr<T> MakeBaseObject(Args&&... args) {
   return BaseObjectPtr<T>(new T(std::forward<Args>(args)...));
 }
+template <typename T, typename... Args>
+BaseObjectWeakPtr<T> MakeWeakBaseObject(Args&&... args) {
+  T* target = new T(std::forward<Args>(args)...);
+  target->MakeWeak();
+  return BaseObjectWeakPtr<T>(target);
+}
 
 template <typename T, typename... Args>
 BaseObjectPtr<T> MakeDetachedBaseObject(Args&&... args) {

src/base_object.h

@@ -302,6 +302,10 @@ using BaseObjectWeakPtr = BaseObjectPtrImpl<T, true>;
 template <typename T, typename... Args>
 inline BaseObjectPtr<T> 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 <typename T, typename... Args>
+inline BaseObjectWeakPtr<T> 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.

src/env.cc

@@ -594,6 +594,20 @@ void Environment::AssignToContext(Local<v8::Context> context,
   TrackContext(context);
 }
 
+void Environment::UnassignFromContext(Local<v8::Context> 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> context,
                                         const EnvSerializeInfo* env_info) {
   principal_realm_ = std::make_unique<PrincipalRealm>(
       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() {

src/env.h

@@ -649,8 +649,7 @@ class Environment : public MemoryRetainer {
   void AssignToContext(v8::Local<v8::Context> context,
                        Realm* realm,
                        const ContextInfo& info);
-  void TrackContext(v8::Local<v8::Context> context);
-  void UntrackContext(v8::Local<v8::Context> context);
+  void UnassignFromContext(v8::Local<v8::Context> 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<v8::Value> (*fun)(v8::Local<v8::String>),
                          const char* errmsg);
+  void TrackContext(v8::Local<v8::Context> context);
+  void UntrackContext(v8::Local<v8::Context> context);
 
   std::list<binding::DLib> loaded_addons_;
   v8::Isolate* const isolate_;

src/inspector_js_api.cc

@@ -172,12 +172,12 @@ static bool InspectorEnabled(Environment* env) {
 }
 
 void SetConsoleExtensionInstaller(const FunctionCallbackInfo<Value>& 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<Function>());
+  realm->set_inspector_console_extension_installer(info[0].As<Function>());
 }
 
 void CallAndPauseOnStart(const FunctionCallbackInfo<v8::Value>& args) {

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();
 }
 

src/node_realm-inl.h

@@ -86,8 +86,13 @@ inline T* Realm::AddBindingData(v8::Local<v8::Context> context,
                                 Args&&... args) {
   DCHECK_EQ(GetCurrent(context), this);
   // This won't compile if T is not a BaseObject subclass.
-  BaseObjectPtr<T> item =
-      MakeDetachedBaseObject<T>(this, target, std::forward<Args>(args)...);
+  static_assert(std::is_base_of_v<BaseObject, T>);
+  // 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<T> item =
+      MakeWeakBaseObject<T>(this, target, std::forward<Args>(args)...);
   DCHECK_EQ(context->GetAlignedPointerFromEmbedderData(
                 ContextEmbedderIndex::kBindingDataStoreIndex),
             &binding_data_store_);

src/node_realm.cc

@@ -21,6 +21,7 @@ using v8::Value;
 Realm::Realm(Environment* env, v8::Local<v8::Context> 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<v8::Context> 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<TypeName> PrincipalRealm::PropertyName() const {                   \
     return PersistentToLocal::Strong(PropertyName##_);                         \
   }                                                                            \
   void PrincipalRealm::set_##PropertyName(v8::Local<TypeName> 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<Value> PrincipalRealm::BootstrapRealm() {
   HandleScope scope(isolate_);
 

src/node_realm.h

@@ -21,9 +21,9 @@ struct RealmSerializeInfo {
   friend std::ostream& operator<<(std::ostream& o, const RealmSerializeInfo& i);
 };
 
-using BindingDataStore = std::array<BaseObjectPtr<BaseObject>,
-                     static_cast<size_t>(
-                         BindingDataType::kBindingDataTypeCount)>;
+using BindingDataStore =
+    std::array<BaseObjectWeakPtr<BaseObject>,
+               static_cast<size_t>(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<v8::Context> context,
                  const RealmSerializeInfo* realm_info);
-  ~PrincipalRealm() = default;
+  ~PrincipalRealm();
 
   SET_MEMORY_INFO_NAME(PrincipalRealm)
   SET_SELF_SIZE(PrincipalRealm)

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,39 +31,62 @@ ShadowRealm* ShadowRealm::New(Environment* env) {
 MaybeLocal<Context> HostCreateShadowRealmContextCallback(
     Local<Context> 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<Context>();
 }
 
 // static
 void ShadowRealm::WeakCallback(const v8::WeakCallbackInfo<ShadowRealm>& 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<ShadowRealm*>(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<v8::Context> ShadowRealm::context() const {

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<v8::Value> BootstrapRealm() override;
 
  private:
   static void WeakCallback(const v8::WeakCallbackInfo<ShadowRealm>& data);
+  static void DeleteMe(void* data);
 
   explicit ShadowRealm(Environment* env);
   ~ShadowRealm();

src/node_url.cc

@@ -40,6 +40,7 @@ BindingData::BindingData(Realm* realm, v8::Local<v8::Object> object)
             FIXED_ONE_BYTE_STRING(realm->isolate(), "urlComponents"),
             url_components_buffer_.GetJSArray())
       .Check();
+  url_components_buffer_.MakeWeak();
 }
 
 bool BindingData::PrepareForSerialization(v8::Local<v8::Context> context,

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]
 

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);
+  }));
+}