src: iterate over base objects to prepare for snapshot

joyeecheung · Fyko · commit 9dc07af1c6b8 · 2022-09-15T16:56:07.000-06:00
Instead of iterating over the bindings, iterate over the base objects that are snapshottable. This allows us to snapshot base objects that are not bindings. In addition this refactors the InternalFieldInfo class to eliminate potential undefined behaviors, and renames it to InternalFieldInfoBase. The {de}serialize callbacks now expect a InternalFieldInfo struct nested in Snapshotable classes that can be used to carry serialization data around. This allows us to create structs inheriting from InternalFieldInfo for Snapshotable objects that need custom fields. PR-URL: nodejs#44192 Refs: nodejs#37476 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
diff --git a/src/env-inl.h b/src/env-inl.h
@@ -835,17 +835,6 @@ void Environment::ForEachBaseObject(T&& iterator) {
   }
 }
 
-template <typename T>
-void Environment::ForEachBindingData(T&& iterator) {
-  BindingDataStore* map = static_cast<BindingDataStore*>(
-      context()->GetAlignedPointerFromEmbedderData(
-          ContextEmbedderIndex::kBindingListIndex));
-  DCHECK_NOT_NULL(map);
-  for (auto& it : *map) {
-    iterator(it.first, it.second);
-  }
-}
-
 void Environment::modify_base_object_count(int64_t delta) {
   base_object_count_ += delta;
 }
diff --git a/src/env.cc b/src/env.cc
@@ -1669,7 +1669,6 @@ EnvSerializeInfo Environment::Serialize(SnapshotCreator* creator) {
   EnvSerializeInfo info;
   Local<Context> ctx = context();
 
-  SerializeBindingData(this, creator, &info);
   // Currently all modules are compiled without cache in builtin snapshot
   // builder.
   info.builtins = std::vector<std::string>(builtins_without_cache.begin(),
@@ -1697,6 +1696,10 @@ EnvSerializeInfo Environment::Serialize(SnapshotCreator* creator) {
   ENVIRONMENT_STRONG_PERSISTENT_VALUES(V)
 #undef V
 
+  // Do this after other creator->AddData() calls so that Snapshotable objects
+  // can use 0 to indicate that a SnapshotIndex is invalid.
+  SerializeSnapshotableObjects(this, creator, &info);
+
   info.context = creator->AddData(ctx, context());
   return info;
 }
@@ -1729,9 +1732,9 @@ std::ostream& operator<<(std::ostream& output,
 
 std::ostream& operator<<(std::ostream& output, const EnvSerializeInfo& i) {
   output << "{\n"
-         << "// -- bindings begins --\n"
-         << i.bindings << ",\n"
-         << "// -- bindings ends --\n"
+         << "// -- native_objects begins --\n"
+         << i.native_objects << ",\n"
+         << "// -- native_objects ends --\n"
          << "// -- builtins begins --\n"
          << i.builtins << ",\n"
          << "// -- builtins ends --\n"
@@ -1758,7 +1761,7 @@ std::ostream& operator<<(std::ostream& output, const EnvSerializeInfo& i) {
 void Environment::EnqueueDeserializeRequest(DeserializeRequestCallback cb,
                                             Local<Object> holder,
                                             int index,
-                                            InternalFieldInfo* info) {
+                                            InternalFieldInfoBase* info) {
   DCHECK_EQ(index, BaseObject::kEmbedderType);
   DeserializeRequest request{cb, {isolate(), holder}, index, info};
   deserialize_requests_.push_back(std::move(request));
diff --git a/src/env.h b/src/env.h
@@ -956,19 +956,19 @@ class CleanupHookCallback {
 typedef void (*DeserializeRequestCallback)(v8::Local<v8::Context> context,
                                            v8::Local<v8::Object> holder,
                                            int index,
-                                           InternalFieldInfo* info);
+                                           InternalFieldInfoBase* info);
 struct DeserializeRequest {
   DeserializeRequestCallback cb;
   v8::Global<v8::Object> holder;
   int index;
-  InternalFieldInfo* info = nullptr;  // Owned by the request
+  InternalFieldInfoBase* info = nullptr;  // Owned by the request
 
   // Move constructor
   DeserializeRequest(DeserializeRequest&& other) = default;
 };
 
 struct EnvSerializeInfo {
-  std::vector<PropInfo> bindings;
+  std::vector<PropInfo> native_objects;
   std::vector<std::string> builtins;
   AsyncHooks::SerializeInfo async_hooks;
   TickInfo::SerializeInfo tick_info;
@@ -1062,7 +1062,7 @@ class Environment : public MemoryRetainer {
   void EnqueueDeserializeRequest(DeserializeRequestCallback cb,
                                  v8::Local<v8::Object> holder,
                                  int index,
-                                 InternalFieldInfo* info);
+                                 InternalFieldInfoBase* info);
   void RunDeserializeRequests();
   // Should be called before InitializeInspector()
   void InitializeDiagnostics();
@@ -1485,7 +1485,7 @@ class Environment : public MemoryRetainer {
   void RemoveUnmanagedFd(int fd);
 
   template <typename T>
-  void ForEachBindingData(T&& iterator);
+  void ForEachBaseObject(T&& iterator);
 
  private:
   inline void ThrowError(v8::Local<v8::Value> (*fun)(v8::Local<v8::String>),
@@ -1643,9 +1643,6 @@ class Environment : public MemoryRetainer {
   std::function<void(Environment*, int)> process_exit_handler_ {
       DefaultProcessExitHandler };
 
-  template <typename T>
-  void ForEachBaseObject(T&& iterator);
-
 #define V(PropertyName, TypeName) v8::Global<TypeName> PropertyName ## _;
   ENVIRONMENT_STRONG_PERSISTENT_VALUES(V)
 #undef V
diff --git a/src/node_blob.cc b/src/node_blob.cc
@@ -462,11 +462,10 @@ BlobBindingData::StoredDataObject BlobBindingData::get_data_object(
   return entry->second;
 }
 
-void BlobBindingData::Deserialize(
-    Local<Context> context,
-    Local<Object> holder,
-    int index,
-    InternalFieldInfo* info) {
+void BlobBindingData::Deserialize(Local<Context> context,
+                                  Local<Object> holder,
+                                  int index,
+                                  InternalFieldInfoBase* info) {
   DCHECK_EQ(index, BaseObject::kEmbedderType);
   HandleScope scope(context->GetIsolate());
   Environment* env = Environment::GetCurrent(context);
@@ -475,15 +474,18 @@ void BlobBindingData::Deserialize(
   CHECK_NOT_NULL(binding);
 }
 
-void BlobBindingData::PrepareForSerialization(
-    Local<Context> context,
-    v8::SnapshotCreator* creator) {
+bool BlobBindingData::PrepareForSerialization(Local<Context> context,
+                                              v8::SnapshotCreator* creator) {
   // Stored blob objects are not actually persisted.
+  // Return true because we need to maintain the reference to the binding from
+  // JS land.
+  return true;
 }
 
-InternalFieldInfo* BlobBindingData::Serialize(int index) {
+InternalFieldInfoBase* BlobBindingData::Serialize(int index) {
   DCHECK_EQ(index, BaseObject::kEmbedderType);
-  InternalFieldInfo* info = InternalFieldInfo::New(type());
+  InternalFieldInfo* info =
+      InternalFieldInfoBase::New<InternalFieldInfo>(type());
   return info;
 }
 
diff --git a/src/node_blob.h b/src/node_blob.h
@@ -146,6 +146,8 @@ class BlobBindingData : public SnapshotableObject {
  public:
   explicit BlobBindingData(Environment* env, v8::Local<v8::Object> wrap);
 
+  using InternalFieldInfo = InternalFieldInfoBase;
+
   SERIALIZABLE_OBJECT_METHODS()
 
   static constexpr FastStringKey type_name{"node::BlobBindingData"};
diff --git a/src/node_file.cc b/src/node_file.cc
@@ -2400,26 +2400,30 @@ BindingData::BindingData(Environment* env, v8::Local<v8::Object> wrap)
 void BindingData::Deserialize(Local<Context> context,
                               Local<Object> holder,
                               int index,
-                              InternalFieldInfo* info) {
+                              InternalFieldInfoBase* info) {
   DCHECK_EQ(index, BaseObject::kEmbedderType);
   HandleScope scope(context->GetIsolate());
   Environment* env = Environment::GetCurrent(context);
   BindingData* binding = env->AddBindingData<BindingData>(context, holder);
   CHECK_NOT_NULL(binding);
 }
 
-void BindingData::PrepareForSerialization(Local<Context> context,
+bool BindingData::PrepareForSerialization(Local<Context> context,
                                           v8::SnapshotCreator* creator) {
   CHECK(file_handle_read_wrap_freelist.empty());
   // We'll just re-initialize the buffers in the constructor since their
   // contents can be thrown away once consumed in the previous call.
   stats_field_array.Release();
   stats_field_bigint_array.Release();
+  // Return true because we need to maintain the reference to the binding from
+  // JS land.
+  return true;
 }
 
-InternalFieldInfo* BindingData::Serialize(int index) {
+InternalFieldInfoBase* BindingData::Serialize(int index) {
   DCHECK_EQ(index, BaseObject::kEmbedderType);
-  InternalFieldInfo* info = InternalFieldInfo::New(type());
+  InternalFieldInfo* info =
+      InternalFieldInfoBase::New<InternalFieldInfo>(type());
   return info;
 }
 
diff --git a/src/node_file.h b/src/node_file.h
@@ -23,6 +23,7 @@ class BindingData : public SnapshotableObject {
   std::vector<BaseObjectPtr<FileHandleReadWrap>>
       file_handle_read_wrap_freelist;
 
+  using InternalFieldInfo = InternalFieldInfoBase;
   SERIALIZABLE_OBJECT_METHODS()
   static constexpr FastStringKey type_name{"node::fs::BindingData"};
   static constexpr EmbedderObjectType type_int =
diff --git a/src/node_process.h b/src/node_process.h
@@ -50,6 +50,8 @@ class BindingData : public SnapshotableObject {
   void AddMethods();
   static void RegisterExternalReferences(ExternalReferenceRegistry* registry);
 
+  using InternalFieldInfo = InternalFieldInfoBase;
+
   SERIALIZABLE_OBJECT_METHODS()
   static constexpr FastStringKey type_name{"node::process::BindingData"};
   static constexpr EmbedderObjectType type_int =
diff --git a/src/node_process_methods.cc b/src/node_process_methods.cc
@@ -524,23 +524,27 @@ void BindingData::SlowNumber(const v8::FunctionCallbackInfo<v8::Value>& args) {
   NumberImpl(FromJSObject<BindingData>(args.Holder()));
 }
 
-void BindingData::PrepareForSerialization(Local<Context> context,
+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();
+  // Return true because we need to maintain the reference to the binding from
+  // JS land.
+  return true;
 }
 
-InternalFieldInfo* BindingData::Serialize(int index) {
+InternalFieldInfoBase* BindingData::Serialize(int index) {
   DCHECK_EQ(index, BaseObject::kEmbedderType);
-  InternalFieldInfo* info = InternalFieldInfo::New(type());
+  InternalFieldInfo* info =
+      InternalFieldInfoBase::New<InternalFieldInfo>(type());
   return info;
 }
 
 void BindingData::Deserialize(Local<Context> context,
                               Local<Object> holder,
                               int index,
-                              InternalFieldInfo* info) {
+                              InternalFieldInfoBase* info) {
   DCHECK_EQ(index, BaseObject::kEmbedderType);
   v8::HandleScope scope(context->GetIsolate());
   Environment* env = Environment::GetCurrent(context);
diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc
@@ -638,7 +638,7 @@ template <>
 EnvSerializeInfo FileReader::Read() {
   per_process::Debug(DebugCategory::MKSNAPSHOT, "Read<EnvSerializeInfo>()\n");
   EnvSerializeInfo result;
-  result.bindings = ReadVector<PropInfo>();
+  result.native_objects = ReadVector<PropInfo>();
   result.builtins = ReadVector<std::string>();
   result.async_hooks = Read<AsyncHooks::SerializeInfo>();
   result.tick_info = Read<TickInfo::SerializeInfo>();
@@ -661,7 +661,7 @@ size_t FileWriter::Write(const EnvSerializeInfo& data) {
   }
 
   // Use += here to ensure order of evaluation.
-  size_t written_total = WriteVector<PropInfo>(data.bindings);
+  size_t written_total = WriteVector<PropInfo>(data.native_objects);
   written_total += WriteVector<std::string>(data.builtins);
   written_total += Write<AsyncHooks::SerializeInfo>(data.async_hooks);
   written_total += Write<TickInfo::SerializeInfo>(data.tick_info);
@@ -1179,17 +1179,6 @@ const char* SnapshotableObject::GetTypeNameChars() const {
   }
 }
 
-bool IsSnapshotableType(FastStringKey key) {
-#define V(PropertyName, NativeTypeName)                                        \
-  if (key == NativeTypeName::type_name) {                                      \
-    return true;                                                               \
-  }
-  SERIALIZABLE_OBJECT_TYPES(V)
-#undef V
-
-  return false;
-}
-
 void DeserializeNodeInternalFields(Local<Object> holder,
                                    int index,
                                    StartupData payload,
@@ -1212,10 +1201,10 @@ void DeserializeNodeInternalFields(Local<Object> holder,
   DCHECK_EQ(index, BaseObject::kEmbedderType);
 
   Environment* env_ptr = static_cast<Environment*>(env);
-  const InternalFieldInfo* info =
-      reinterpret_cast<const InternalFieldInfo*>(payload.data);
+  const InternalFieldInfoBase* info =
+      reinterpret_cast<const InternalFieldInfoBase*>(payload.data);
   // TODO(joyeecheung): we can add a constant kNodeEmbedderId to the
-  // beginning of every InternalFieldInfo to ensure that we don't
+  // beginning of every InternalFieldInfoBase to ensure that we don't
   // step on payloads that were not serialized by Node.js.
   switch (info->type) {
 #define V(PropertyName, NativeTypeName)                                        \
@@ -1225,12 +1214,25 @@ void DeserializeNodeInternalFields(Local<Object> holder,
                        (*holder),                                              \
                        NativeTypeName::type_name.c_str());                     \
     env_ptr->EnqueueDeserializeRequest(                                        \
-        NativeTypeName::Deserialize, holder, index, info->Copy());             \
+        NativeTypeName::Deserialize,                                           \
+        holder,                                                                \
+        index,                                                                 \
+        info->Copy<NativeTypeName::InternalFieldInfo>());                      \
     break;                                                                     \
   }
     SERIALIZABLE_OBJECT_TYPES(V)
 #undef V
-    default: { UNREACHABLE(); }
+    default: {
+      // This should only be reachable during development when trying to
+      // deserialize a snapshot blob built by a version of Node.js that
+      // has more recognizable EmbedderObjectTypes than the deserializing
+      // Node.js binary.
+      fprintf(stderr,
+              "Unknown embedder object type %" PRIu8 ", possibly caused by "
+              "mismatched Node.js versions\n",
+              static_cast<uint8_t>(info->type));
+      ABORT();
+    }
   }
 }
 
@@ -1263,17 +1265,17 @@ StartupData SerializeNodeContextInternalFields(Local<Object> holder,
                      static_cast<int>(index),
                      *holder);
 
-  void* binding_ptr =
+  void* native_ptr =
       holder->GetAlignedPointerFromInternalField(BaseObject::kSlot);
-  per_process::Debug(DebugCategory::MKSNAPSHOT, "binding = %p\n", binding_ptr);
-  DCHECK(static_cast<BaseObject*>(binding_ptr)->is_snapshotable());
-  SnapshotableObject* obj = static_cast<SnapshotableObject*>(binding_ptr);
+  per_process::Debug(DebugCategory::MKSNAPSHOT, "native = %p\n", native_ptr);
+  DCHECK(static_cast<BaseObject*>(native_ptr)->is_snapshotable());
+  SnapshotableObject* obj = static_cast<SnapshotableObject*>(native_ptr);
 
   per_process::Debug(DebugCategory::MKSNAPSHOT,
                      "Object %p is %s, ",
                      *holder,
                      obj->GetTypeNameChars());
-  InternalFieldInfo* info = obj->Serialize(index);
+  InternalFieldInfoBase* info = obj->Serialize(index);
 
   per_process::Debug(DebugCategory::MKSNAPSHOT,
                      "payload size=%d\n",
@@ -1282,31 +1284,35 @@ StartupData SerializeNodeContextInternalFields(Local<Object> holder,
                      static_cast<int>(info->length)};
 }
 
-void SerializeBindingData(Environment* env,
-                          SnapshotCreator* creator,
-                          EnvSerializeInfo* info) {
+void SerializeSnapshotableObjects(Environment* env,
+                                  SnapshotCreator* creator,
+                                  EnvSerializeInfo* info) {
   uint32_t i = 0;
-  env->ForEachBindingData([&](FastStringKey key,
-                              BaseObjectPtr<BaseObject> binding) {
+  env->ForEachBaseObject([&](BaseObject* obj) {
+    // If there are any BaseObjects that are not snapshotable left
+    // during context serialization, V8 would crash due to unregistered
+    // global handles and print detailed information about them.
+    if (!obj->is_snapshotable()) {
+      return;
+    }
+    SnapshotableObject* ptr = static_cast<SnapshotableObject*>(obj);
+
+    const char* type_name = ptr->GetTypeNameChars();
     per_process::Debug(DebugCategory::MKSNAPSHOT,
-                       "Serialize binding %i (%p), object=%p, type=%s\n",
+                       "Serialize snapshotable object %i (%p), "
+                       "object=%p, type=%s\n",
                        static_cast<int>(i),
-                       binding.get(),
-                       *(binding->object()),
-                       key.c_str());
+                       ptr,
+                       *(ptr->object()),
+                       type_name);
 
-    if (IsSnapshotableType(key)) {
-      SnapshotIndex index = creator->AddData(env->context(), binding->object());
+    if (ptr->PrepareForSerialization(env->context(), creator)) {
+      SnapshotIndex index = creator->AddData(env->context(), obj->object());
       per_process::Debug(DebugCategory::MKSNAPSHOT,
                          "Serialized with index=%d\n",
                          static_cast<int>(index));
-      info->bindings.push_back({key.c_str(), i, index});
-      SnapshotableObject* ptr = static_cast<SnapshotableObject*>(binding.get());
-      ptr->PrepareForSerialization(env->context(), creator);
-    } else {
-      UNREACHABLE();
+      info->native_objects.push_back({type_name, i, index});
     }
-
     i++;
   });
 }
diff --git a/src/node_snapshotable.h b/src/node_snapshotable.h
diff --git a/src/node_v8.cc b/src/node_v8.cc
diff --git a/src/node_v8.h b/src/node_v8.h

Original file line number	Diff line number	Diff line change
`@@ -835,17 +835,6 @@ void Environment::ForEachBaseObject(T&& iterator) {`
`835`	`835`	`}`
`836`	`836`	`}`
`837`	`837`
`838`		`-template <typename T>`
`839`		`-void Environment::ForEachBindingData(T&& iterator) {`
`840`		`- BindingDataStore* map = static_cast<BindingDataStore*>(`
`841`		`- context()->GetAlignedPointerFromEmbedderData(`
`842`		`- ContextEmbedderIndex::kBindingListIndex));`
`843`		`- DCHECK_NOT_NULL(map);`
`844`		`- for (auto& it : *map) {`
`845`		`- iterator(it.first, it.second);`
`846`		`- }`
`847`		`-}`
`848`		`-`
`849`	`838`	`void Environment::modify_base_object_count(int64_t delta) {`
`850`	`839`	`base_object_count_ += delta;`
`851`	`840`	`}`