[snapshot] Fix reconstruction of shared heap object cache

Currently the reconstruction of the shared heap object cache for testing
incorrectly includes the terminating undefined value. Unlike the RO cache
reconstruction, which does not change, the shared heap object cache may be
extended by serializing the live Isolate during testing, so it should skip the
original terminating undefined.

Bug: v8:12007, v8:12584
Change-Id: If73b865567ed7d5b658506e15b1dc8d14bd755d6
Cq-Include-Trybots: luci.v8.try:v8_linux64_gc_stress_custom_snapshot_dbg_ng
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3421726
Auto-Submit: Shu-yu Guo <syg@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78836}

src/snapshot/serializer-deserializer.cc

@@ -14,7 +14,8 @@ namespace {
 DISABLE_CFI_PERF
 void IterateObjectCache(Isolate* isolate, std::vector<Object>* cache,
                         Root root_id, RootVisitor* visitor) {
-  for (size_t i = 0;; ++i) {
+  size_t i;
+  for (i = 0;; ++i) {
     // Extend the array ready to get a value when deserializing.
     if (cache->size() <= i) cache->push_back(Smi::zero());
     // During deserialization, the visitor populates the object cache and

src/snapshot/shared-heap-serializer.cc

@@ -44,6 +44,9 @@ SharedHeapSerializer::SharedHeapSerializer(
       serialized_objects_(isolate->heap())
 #endif
 {
+  if (ShouldReconstructSharedHeapObjectCacheForTesting()) {
+    ReconstructSharedHeapObjectCacheForTesting();
+  }
 }
 
 SharedHeapSerializer::~SharedHeapSerializer() {
@@ -87,16 +90,22 @@ bool SharedHeapSerializer::SerializeUsingSharedHeapObjectCache(
   if (!ShouldBeInSharedHeapObjectCache(*obj)) return false;
   int cache_index = SerializeInObjectCache(obj);
 
-  // When testing deserialization of a snapshot from a live isolate, the shared
-  // object cache needs to be extended because the live isolate may have had new
-  // internalized strings that were not present in the startup snapshot to be
-  // serialized.
-  if (reconstruct_read_only_and_shared_object_caches_for_testing()) {
-    const size_t existing_cache_size =
-        isolate()->shared_heap_object_cache()->size();
-    DCHECK_LE(base::checked_cast<size_t>(cache_index), existing_cache_size);
-    if (base::checked_cast<size_t>(cache_index) == existing_cache_size) {
-      isolate()->shared_heap_object_cache()->push_back(*obj);
+  // When testing deserialization of a snapshot from a live Isolate where there
+  // is also a shared Isolate, the shared object cache needs to be extended
+  // because the live isolate may have had new internalized strings that were
+  // not present in the startup snapshot to be serialized.
+  if (ShouldReconstructSharedHeapObjectCacheForTesting()) {
+    std::vector<Object>* existing_cache =
+        isolate()->shared_isolate()->shared_heap_object_cache();
+    const size_t existing_cache_size = existing_cache->size();
+    // This is strictly < because the existing cache contains the terminating
+    // undefined value, which the reconstructed cache does not.
+    DCHECK_LT(base::checked_cast<size_t>(cache_index), existing_cache_size);
+    if (base::checked_cast<size_t>(cache_index) == existing_cache_size - 1) {
+      ReadOnlyRoots roots(isolate());
+      DCHECK(existing_cache->back().IsUndefined(roots));
+      existing_cache->back() = *obj;
+      existing_cache->push_back(roots.undefined_value());
     }
   }
 
@@ -180,17 +189,29 @@ void SharedHeapSerializer::SerializeObjectImpl(Handle<HeapObject> obj) {
 #endif
 }
 
-void SharedHeapSerializer::
-    ReconstructSharedHeapObjectCacheForTestingIfNeeded() {
-  if (!reconstruct_read_only_and_shared_object_caches_for_testing()) return;
-  std::vector<Object>* cache = isolate()->shared_heap_object_cache();
-  DCHECK_EQ(isolate()->shared_isolate()->shared_heap_object_cache(), cache);
-  for (size_t i = 0, size = cache->size(); i < size; i++) {
+bool SharedHeapSerializer::ShouldReconstructSharedHeapObjectCacheForTesting()
+    const {
+  // When the live Isolate being serialized is not a client Isolate, there's no
+  // need to reconstruct the shared heap object cache because it is not actually
+  // shared.
+  return reconstruct_read_only_and_shared_object_caches_for_testing() &&
+         isolate()->shared_isolate() != nullptr;
+}
+
+void SharedHeapSerializer::ReconstructSharedHeapObjectCacheForTesting() {
+  std::vector<Object>* cache =
+      isolate()->shared_isolate()->shared_heap_object_cache();
+  // Don't reconstruct the final element, which is always undefined and marks
+  // the end of the cache, since serializing the live Isolate may extend the
+  // shared object cache.
+  for (size_t i = 0, size = cache->size(); i < size - 1; i++) {
     Handle<HeapObject> obj(HeapObject::cast(cache->at(i)), isolate());
+    DCHECK(ShouldBeInSharedHeapObjectCache(*obj));
     int cache_index = SerializeInObjectCache(obj);
     USE(cache_index);
     DCHECK_EQ(cache_index, i);
   }
+  DCHECK(cache->back().IsUndefined(isolate()));
 }
 
 }  // namespace internal

src/snapshot/shared-heap-serializer.h

@@ -43,13 +43,15 @@ class V8_EXPORT_PRIVATE SharedHeapSerializer : public RootsSerializer {
   bool SerializeUsingSharedHeapObjectCache(SnapshotByteSink* sink,
                                            Handle<HeapObject> obj);
 
-  void ReconstructSharedHeapObjectCacheForTestingIfNeeded();
-
   static bool CanBeInSharedOldSpace(HeapObject obj);
 
   static bool ShouldBeInSharedHeapObjectCache(HeapObject obj);
 
  private:
+  bool ShouldReconstructSharedHeapObjectCacheForTesting() const;
+
+  void ReconstructSharedHeapObjectCacheForTesting();
+
   void SerializeStringTable(StringTable* string_table);
 
   void SerializeObjectImpl(Handle<HeapObject> obj) override;

src/snapshot/snapshot.cc

@@ -383,7 +383,6 @@ v8::StartupData Snapshot::Create(
 
   SharedHeapSerializer shared_heap_serializer(isolate, flags,
                                               &read_only_serializer);
-  shared_heap_serializer.ReconstructSharedHeapObjectCacheForTestingIfNeeded();
 
   StartupSerializer startup_serializer(isolate, flags, &read_only_serializer,
                                        &shared_heap_serializer);