src: make process binding data weak

legendecas · targos · commit 4a1ce451817f · 2023-11-23T10:15:43.000+01:00
Avoid the realm being strongly referenced by the process binding data. PR-URL: #48655 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
diff --git a/lib/internal/process/per_thread.js b/lib/internal/process/per_thread.js
@@ -28,7 +28,6 @@ const {
   StringPrototypeStartsWith,
   Symbol,
   SymbolIterator,
-  Uint32Array,
 } = primordials;
 
 const {
@@ -65,10 +64,10 @@ function refreshHrtimeBuffer() {
   // The 3 entries filled in by the original process.hrtime contains
   // the upper/lower 32 bits of the second part of the value,
   // and the remaining nanoseconds of the value.
-  hrValues = new Uint32Array(binding.hrtimeBuffer);
+  hrValues = binding.hrtimeBuffer;
   // Use a BigUint64Array in the closure because this is actually a bit
   // faster than simply returning a BigInt from C++ in V8 7.1.
-  hrBigintValues = new BigUint64Array(binding.hrtimeBuffer, 0, 1);
+  hrBigintValues = new BigUint64Array(binding.hrtimeBuffer.buffer, 0, 1);
 }
 
 // Create the buffers.
diff --git a/src/node_process.h b/src/node_process.h
@@ -48,16 +48,20 @@ void PatchProcessObject(const v8::FunctionCallbackInfo<v8::Value>& args);
 namespace process {
 class BindingData : public SnapshotableObject {
  public:
+  struct InternalFieldInfo : public node::InternalFieldInfoBase {
+    AliasedBufferIndex hrtime_buffer;
+  };
+
   static void AddMethods(v8::Isolate* isolate,
                          v8::Local<v8::ObjectTemplate> target);
   static void RegisterExternalReferences(ExternalReferenceRegistry* registry);
 
-  using InternalFieldInfo = InternalFieldInfoBase;
-
   SERIALIZABLE_OBJECT_METHODS()
   SET_BINDING_ID(process_binding_data)
 
-  BindingData(Realm* realm, v8::Local<v8::Object> object);
+  BindingData(Realm* realm,
+              v8::Local<v8::Object> object,
+              InternalFieldInfo* info = nullptr);
 
   void MemoryInfo(MemoryTracker* tracker) const override;
   SET_MEMORY_INFO_NAME(BindingData)
@@ -81,10 +85,10 @@ class BindingData : public SnapshotableObject {
   static void SlowBigInt(const v8::FunctionCallbackInfo<v8::Value>& args);
 
  private:
-  static constexpr size_t kBufferSize =
-      std::max(sizeof(uint64_t), sizeof(uint32_t) * 3);
-  v8::Global<v8::ArrayBuffer> array_buffer_;
-  std::shared_ptr<v8::BackingStore> backing_store_;
+  // Buffer length in uint32.
+  static constexpr size_t kHrTimeBufferLength = 3;
+  AliasedUint32Array hrtime_buffer_;
+  InternalFieldInfo* internal_field_info_ = nullptr;
 
   // These need to be static so that we have their addresses available to
   // register as external references in the snapshot at environment creation
diff --git a/src/node_process_methods.cc b/src/node_process_methods.cc
@@ -465,15 +465,29 @@ static void ReallyExit(const FunctionCallbackInfo<Value>& args) {
 
 namespace process {
 
-BindingData::BindingData(Realm* realm, v8::Local<v8::Object> object)
-    : SnapshotableObject(realm, object, type_int) {
+BindingData::BindingData(Realm* realm,
+                         v8::Local<v8::Object> object,
+                         InternalFieldInfo* info)
+    : SnapshotableObject(realm, object, type_int),
+      hrtime_buffer_(realm->isolate(),
+                     kHrTimeBufferLength,
+                     MAYBE_FIELD_PTR(info, hrtime_buffer)) {
   Isolate* isolate = realm->isolate();
   Local<Context> context = realm->context();
-  Local<ArrayBuffer> ab = ArrayBuffer::New(isolate, kBufferSize);
-  array_buffer_.Reset(isolate, ab);
-  object->Set(context, FIXED_ONE_BYTE_STRING(isolate, "hrtimeBuffer"), ab)
-      .ToChecked();
-  backing_store_ = ab->GetBackingStore();
+
+  if (info == nullptr) {
+    object
+        ->Set(context,
+              FIXED_ONE_BYTE_STRING(isolate, "hrtimeBuffer"),
+              hrtime_buffer_.GetJSArray())
+        .ToChecked();
+  } else {
+    hrtime_buffer_.Deserialize(realm->context());
+  }
+
+  // The hrtime buffer is referenced from the binding data js object.
+  // Make the native handle weak to avoid keeping the realm alive.
+  hrtime_buffer_.MakeWeak();
 }
 
 v8::CFunction BindingData::fast_number_(v8::CFunction::Make(FastNumber));
@@ -503,7 +517,7 @@ BindingData* BindingData::FromV8Value(Local<Value> value) {
 }
 
 void BindingData::MemoryInfo(MemoryTracker* tracker) const {
-  tracker->TrackField("array_buffer", array_buffer_);
+  tracker->TrackField("hrtime_buffer", hrtime_buffer_);
 }
 
 // This is the legacy version of hrtime before BigInt was introduced in
@@ -516,20 +530,19 @@ void BindingData::MemoryInfo(MemoryTracker* tracker) const {
 // because there is no Uint64Array in JS.
 // The third entry contains the remaining nanosecond part of the value.
 void BindingData::NumberImpl(BindingData* receiver) {
-  // Make sure we don't accidentally access buffers wiped for snapshot.
-  CHECK(!receiver->array_buffer_.IsEmpty());
   uint64_t t = uv_hrtime();
-  uint32_t* fields = static_cast<uint32_t*>(receiver->backing_store_->Data());
-  fields[0] = (t / NANOS_PER_SEC) >> 32;
-  fields[1] = (t / NANOS_PER_SEC) & 0xffffffff;
-  fields[2] = t % NANOS_PER_SEC;
+  receiver->hrtime_buffer_[0] = (t / NANOS_PER_SEC) >> 32;
+  receiver->hrtime_buffer_[1] = (t / NANOS_PER_SEC) & 0xffffffff;
+  receiver->hrtime_buffer_[2] = t % NANOS_PER_SEC;
 }
 
 void BindingData::BigIntImpl(BindingData* receiver) {
-  // Make sure we don't accidentally access buffers wiped for snapshot.
-  CHECK(!receiver->array_buffer_.IsEmpty());
   uint64_t t = uv_hrtime();
-  uint64_t* fields = static_cast<uint64_t*>(receiver->backing_store_->Data());
+  // The buffer is a Uint32Array, so we need to reinterpret it as a
+  // Uint64Array to write the value. The buffer is valid at this scope so we
+  // can safely cast away the constness.
+  uint64_t* fields = reinterpret_cast<uint64_t*>(
+      const_cast<uint32_t*>(receiver->hrtime_buffer_.GetNativeBuffer()));
   fields[0] = t;
 }
 
@@ -543,18 +556,19 @@ void BindingData::SlowNumber(const v8::FunctionCallbackInfo<v8::Value>& args) {
 
 bool BindingData::PrepareForSerialization(Local<Context> context,
                                           v8::SnapshotCreator* creator) {
-  // It's not worth keeping.
-  // Release it, we will recreate it when the instance is dehydrated.
-  array_buffer_.Reset();
+  DCHECK_NULL(internal_field_info_);
+  internal_field_info_ = InternalFieldInfoBase::New<InternalFieldInfo>(type());
+  internal_field_info_->hrtime_buffer =
+      hrtime_buffer_.Serialize(context, creator);
   // Return true because we need to maintain the reference to the binding from
   // JS land.
   return true;
 }
 
 InternalFieldInfoBase* BindingData::Serialize(int index) {
   DCHECK_IS_SNAPSHOT_SLOT(index);
-  InternalFieldInfo* info =
-      InternalFieldInfoBase::New<InternalFieldInfo>(type());
+  InternalFieldInfo* info = internal_field_info_;
+  internal_field_info_ = nullptr;
   return info;
 }
 
@@ -566,7 +580,9 @@ void BindingData::Deserialize(Local<Context> context,
   v8::HandleScope scope(context->GetIsolate());
   Realm* realm = Realm::GetCurrent(context);
   // Recreate the buffer in the constructor.
-  BindingData* binding = realm->AddBindingData<BindingData>(holder);
+  InternalFieldInfo* casted_info = static_cast<InternalFieldInfo*>(info);
+  BindingData* binding =
+      realm->AddBindingData<BindingData>(holder, casted_info);
   CHECK_NOT_NULL(binding);
 }
 
