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.

Author: legendecas

src/aliased_buffer-inl.h

@@ -70,8 +70,8 @@ AliasedBufferBase<NativeT, V8T>::AliasedBufferBase(
       count_(that.count_),
       byte_offset_(that.byte_offset_),
       buffer_(that.buffer_) {
-  DCHECK(is_valid());
   js_array_ = v8::Global<V8T>(that.isolate_, that.GetJSArray());
+  DCHECK(is_valid());
 }
 
 template <typename NativeT, typename V8T>
@@ -126,19 +126,10 @@ void AliasedBufferBase<NativeT, V8T>::Release() {
   js_array_.Reset();
 }
 
-template <typename NativeT, typename V8T>
-inline void AliasedBufferBase<NativeT, V8T>::WeakCallback(
-    const v8::WeakCallbackInfo<AliasedBufferBase<NativeT, V8T>>& data) {
-  AliasedBufferBase<NativeT, V8T>* buffer = data.GetParameter();
-  DCHECK(buffer->is_valid());
-  buffer->cleared_ = true;
-  buffer->js_array_.Reset();
-}
-
 template <typename NativeT, typename V8T>
 inline void AliasedBufferBase<NativeT, V8T>::MakeWeak() {
   DCHECK(is_valid());
-  js_array_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);
+  js_array_.SetWeak();
 }
 
 template <typename NativeT, typename V8T>
@@ -223,7 +214,7 @@ void AliasedBufferBase<NativeT, V8T>::reserve(size_t new_capacity) {
 
 template <typename NativeT, typename V8T>
 inline bool AliasedBufferBase<NativeT, V8T>::is_valid() const {
-  return index_ == nullptr && !cleared_;
+  return index_ == nullptr && !js_array_.IsEmpty();
 }
 
 template <typename NativeT, typename V8T>

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<AliasedBufferBase<NativeT, V8T>>& data);
   v8::Isolate* isolate_ = nullptr;
   size_t count_ = 0;
   size_t byte_offset_ = 0;
   NativeT* buffer_ = nullptr;
   v8::Global<V8T> js_array_;
-  bool cleared_ = false;
 
   // Deserialize data
   const AliasedBufferIndex* index_ = nullptr;

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

@@ -1714,6 +1714,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 +1727,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/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_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

@@ -278,11 +278,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)

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

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;
@@ -31,17 +32,24 @@ 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();
-  delete realm;
+  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; });
 }
 
 ShadowRealm::ShadowRealm(Environment* env)

src/node_url.cc

@@ -41,6 +41,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/known_issues/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);
+  }));
+}