src: make realm binding data store weak

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

@@ -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]