[heap] Correctly flush multi-referenced bytecode

The implementation of bytecode flushing assumes that there is a single
owning SharedFunctionInfo for each bytecode. However, sometimes there
can be two, because a couple of code paths copy content from one
SharedFunctionInfo to another (BackgroundCompileTask::FinalizeFunction
and BackgroundMergeTask::CompleteMergeInForeground). Usually this works
out okay in practice, because we only copy content from a
SharedFunctionInfo when we're about to abandon all references to it.
However, we shouldn't rely on this lucky timing, especially considering
a possible future where conservative stack scanning could retain the
copied-from SharedFunctionInfo for an arbitrarily long time due to
spurious stack references. This change updates the bytecode flushing
implementation to correctly handle the case of two SharedFunctionInfos
that point to the same BytecodeArray.

I don't know if this fixes the linked bug, but so far it's the only
semi-plausible explanation I've found.

Bug: chromium:1359773
Change-Id: Iaa2c6e4953afcb46df2ac4b17828271151d85e59
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3916272
Commit-Queue: Seth Brenith <seth.brenith@microsoft.com>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#83728}

src/heap/mark-compact.cc

@@ -3134,8 +3134,17 @@ void MarkCompactCollector::ProcessOldCodeCandidates() {
   SharedFunctionInfo flushing_candidate;
   while (local_weak_objects()->code_flushing_candidates_local.Pop(
       &flushing_candidate)) {
-    bool is_bytecode_live = non_atomic_marking_state()->IsBlackOrGrey(
-        flushing_candidate.GetBytecodeArray(isolate()));
+    // During flushing a BytecodeArray is transformed into an UncompiledData in
+    // place. Seeing an UncompiledData here implies that another
+    // SharedFunctionInfo had a reference to the same ByteCodeArray and flushed
+    // it before processing this candidate. This can happen when using
+    // CloneSharedFunctionInfo().
+    bool bytecode_already_decompiled =
+        flushing_candidate.function_data(isolate(), kAcquireLoad)
+            .IsUncompiledData(isolate());
+    bool is_bytecode_live = !bytecode_already_decompiled &&
+                            non_atomic_marking_state()->IsBlackOrGrey(
+                                flushing_candidate.GetBytecodeArray(isolate()));
     if (v8_flags.flush_baseline_code && flushing_candidate.HasBaselineCode()) {
       CodeT baseline_codet =
           CodeT::cast(flushing_candidate.function_data(kAcquireLoad));
@@ -3168,9 +3177,17 @@ void MarkCompactCollector::ProcessOldCodeCandidates() {
       DCHECK(v8_flags.flush_baseline_code ||
              !flushing_candidate.HasBaselineCode());
 
-      // If the BytecodeArray is dead, flush it, which will replace the field
-      // with an uncompiled data object.
-      FlushBytecodeFromSFI(flushing_candidate);
+      if (bytecode_already_decompiled) {
+        flushing_candidate.DiscardCompiledMetadata(
+            isolate(),
+            [](HeapObject object, ObjectSlot slot, HeapObject target) {
+              RecordSlot(object, slot, target);
+            });
+      } else {
+        // If the BytecodeArray is dead, flush it, which will replace the field
+        // with an uncompiled data object.
+        FlushBytecodeFromSFI(flushing_candidate);
+      }
     }
 
     // Now record the slot, which has either been updated to an uncompiled data,

src/objects/shared-function-info.cc

@@ -347,7 +347,7 @@ void SharedFunctionInfo::DiscardCompiledMetadata(
     std::function<void(HeapObject object, ObjectSlot slot, HeapObject target)>
         gc_notify_updated_slot) {
   DisallowGarbageCollection no_gc;
-  if (is_compiled()) {
+  if (HasFeedbackMetadata()) {
     if (v8_flags.trace_flush_bytecode) {
       CodeTracer::Scope scope(GetIsolate()->GetCodeTracer());
       PrintF(scope.file(), "[discarding compiled metadata for ");

test/cctest/heap/test-heap.cc

@@ -1136,6 +1136,72 @@ TEST(TestBytecodeFlushing) {
   }
 }
 
+TEST(TestMultiReferencedBytecodeFlushing) {
+#ifndef V8_LITE_MODE
+  v8_flags.turbofan = false;
+  v8_flags.always_turbofan = false;
+  i::v8_flags.optimize_for_size = false;
+#endif  // V8_LITE_MODE
+#if ENABLE_SPARKPLUG
+  v8_flags.always_sparkplug = false;
+#endif  // ENABLE_SPARKPLUG
+  i::v8_flags.flush_bytecode = true;
+  i::v8_flags.allow_natives_syntax = true;
+
+  CcTest::InitializeVM();
+  v8::Isolate* isolate = CcTest::isolate();
+  Isolate* i_isolate = CcTest::i_isolate();
+  Factory* factory = i_isolate->factory();
+
+  {
+    v8::HandleScope scope(isolate);
+    v8::Context::New(isolate)->Enter();
+    const char* source =
+        "function foo() {"
+        "  var x = 42;"
+        "  var y = 42;"
+        "  var z = x + y;"
+        "};"
+        "foo()";
+    Handle<String> foo_name = factory->InternalizeUtf8String("foo");
+
+    // This compile will add the code to the compilation cache.
+    {
+      v8::HandleScope new_scope(isolate);
+      CompileRun(source);
+    }
+
+    // Check function is compiled.
+    Handle<Object> func_value =
+        Object::GetProperty(i_isolate, i_isolate->global_object(), foo_name)
+            .ToHandleChecked();
+    CHECK(func_value->IsJSFunction());
+    Handle<JSFunction> function = Handle<JSFunction>::cast(func_value);
+    Handle<SharedFunctionInfo> shared = handle(function->shared(), i_isolate);
+    CHECK(shared->is_compiled());
+
+    // Make a copy of the SharedFunctionInfo which points to the same bytecode.
+    Handle<SharedFunctionInfo> copy =
+        i_isolate->factory()->CloneSharedFunctionInfo(shared);
+
+    // Simulate several GCs that use full marking.
+    const int kAgingThreshold = 7;
+    for (int i = 0; i < kAgingThreshold; i++) {
+      CcTest::CollectAllGarbage();
+    }
+
+    // foo should no longer be in the compilation cache
+    CHECK(!shared->is_compiled());
+    CHECK(!copy->is_compiled());
+    CHECK(!function->is_compiled());
+
+    // The feedback metadata for both SharedFunctionInfo instances should have
+    // been reset.
+    CHECK(!shared->HasFeedbackMetadata());
+    CHECK(!copy->HasFeedbackMetadata());
+  }
+}
+
 HEAP_TEST(Regress10560) {
   i::v8_flags.flush_bytecode = true;
   i::v8_flags.allow_natives_syntax = true;