Fix a failing DCHECK

The destructor for BackgroundMergeTask checks that the task doesn't have
pending foreground work. However, there are valid cases where the task
can be abandoned without completing its foreground work, either because
another copy of the same script showed up in the Isolate compilation
cache or because the serialized code data had an incorrect source hash
and was rejected. This change removes the problematic DCHECK and adds a
new one in code-serializer.cc at a point where we can actually be sure
there isn't pending foreground work.

Bug: chromium:1400781
Change-Id: Idb3538229d25f297adf5b2696c4b4b50d85557b1
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4105926
Commit-Queue: Seth Brenith <seth.brenith@microsoft.com>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/main@{#84869}

src/codegen/background-merge-task.h

@@ -25,8 +25,6 @@ struct ScriptDetails;
 // compilation cache.
 class V8_EXPORT_PRIVATE BackgroundMergeTask {
  public:
-  ~BackgroundMergeTask();
-
   // Step 1: on the main thread, check whether the Isolate compilation cache
   // contains the script.
   void SetUpOnMainThread(Isolate* isolate, Handle<String> source_text,

src/codegen/compiler.cc

@@ -1963,10 +1963,6 @@ class ConstantPoolPointerForwarder {
   std::unordered_map<int, Handle<SharedFunctionInfo>> forwarding_table_;
 };
 
-BackgroundMergeTask::~BackgroundMergeTask() {
-  DCHECK(!HasPendingForegroundWork());
-}
-
 void BackgroundMergeTask::SetUpOnMainThread(Isolate* isolate,
                                             Handle<String> source_text,
                                             const ScriptDetails& script_details,

src/snapshot/code-serializer.cc

@@ -613,6 +613,9 @@ MaybeHandle<SharedFunctionInfo> CodeSerializer::FinishOffThreadDeserialize(
 
   FinalizeDeserialization(isolate, result, timer);
 
+  DCHECK(!background_merge_task ||
+         !background_merge_task->HasPendingForegroundWork());
+
   return scope.CloseAndEscape(result);
 }
 

test/unittests/api/deserialize-unittest.cc

@@ -822,4 +822,98 @@ TEST_F(MergeDeserializedCodeTest, MergeThatCompilesLazyFunction) {
   CHECK(expected->StrictEquals(actual));
 }
 
+TEST_F(MergeDeserializedCodeTest, MergeThatStartsButDoesNotFinish) {
+  i::v8_flags.merge_background_deserialized_script_with_compilation_cache =
+      true;
+  constexpr int kSimultaneousScripts = 10;
+  std::vector<std::unique_ptr<v8::ScriptCompiler::CachedData>> cached_data;
+  IsolateAndContextScope scope(this);
+  i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate());
+  ScriptOrigin default_origin(isolate(), NewString(""));
+
+  // Compile the script for the first time to produce code cache data.
+  {
+    v8::HandleScope handle_scope(isolate());
+    Local<Script> script =
+        Script::Compile(context(), NewString(kSourceCode), &default_origin)
+            .ToLocalChecked();
+    CHECK(!script->Run(context()).IsEmpty());
+
+    // Create a bunch of copies of the code cache data.
+    for (int i = 0; i < kSimultaneousScripts; ++i) {
+      cached_data.emplace_back(
+          ScriptCompiler::CreateCodeCache(script->GetUnboundScript()));
+    }
+
+    // Age the top-level bytecode so that the Isolate compilation cache will
+    // contain only the Script.
+    i::BytecodeArray bytecode =
+        GetSharedFunctionInfo(script).GetBytecodeArray(i_isolate);
+    for (int j = 0; j < i::v8_flags.bytecode_old_age; ++j) {
+      bytecode.MakeOlder();
+    }
+  }
+
+  i_isolate->heap()->CollectAllGarbage(i::Heap::kNoGCFlags,
+                                       i::GarbageCollectionReason::kTesting);
+
+  // A second round of GC is necessary in case incremental marking had already
+  // started before the bytecode was aged.
+  i_isolate->heap()->CollectAllGarbage(i::Heap::kNoGCFlags,
+                                       i::GarbageCollectionReason::kTesting);
+
+  // Start several background deserializations.
+  std::vector<std::unique_ptr<DeserializeThread>> deserialize_threads;
+  for (int i = 0; i < kSimultaneousScripts; ++i) {
+    deserialize_threads.push_back(std::make_unique<DeserializeThread>(
+        ScriptCompiler::StartConsumingCodeCache(
+            isolate(), std::make_unique<ScriptCompiler::CachedData>(
+                           cached_data[i]->data, cached_data[i]->length,
+                           ScriptCompiler::CachedData::BufferNotOwned))));
+  }
+  for (int i = 0; i < kSimultaneousScripts; ++i) {
+    CHECK(deserialize_threads[i]->Start());
+  }
+  for (int i = 0; i < kSimultaneousScripts; ++i) {
+    deserialize_threads[i]->Join();
+  }
+
+  // Start background merges for all of those simultaneous scripts.
+  std::vector<std::unique_ptr<ScriptCompiler::ConsumeCodeCacheTask>> tasks;
+  std::vector<std::unique_ptr<MergeThread>> merge_threads;
+  for (int i = 0; i < kSimultaneousScripts; ++i) {
+    tasks.push_back(deserialize_threads[i]->TakeTask());
+    tasks[i]->SourceTextAvailable(isolate(), NewString(kSourceCode),
+                                  default_origin);
+    CHECK(tasks[i]->ShouldMergeWithExistingScript());
+    merge_threads.push_back(std::make_unique<MergeThread>(tasks[i].get()));
+  }
+  for (int i = 0; i < kSimultaneousScripts; ++i) {
+    CHECK(merge_threads[i]->Start());
+  }
+  for (int i = 0; i < kSimultaneousScripts; ++i) {
+    merge_threads[i]->Join();
+  }
+
+  // Complete compilation of each script on the main thread. The first one will
+  // actually finish its merge; the others will abandon their in-progress merges
+  // and instead use the result from the first script since it will be in the
+  // Isolate compilation cache.
+  i::Handle<i::SharedFunctionInfo> first_script_sfi;
+  for (int i = 0; i < kSimultaneousScripts; ++i) {
+    ScriptCompiler::Source source(NewString(kSourceCode), default_origin,
+                                  cached_data[i].release(), tasks[i].release());
+    Local<Script> script =
+        ScriptCompiler::Compile(context(), &source,
+                                ScriptCompiler::kConsumeCodeCache)
+            .ToLocalChecked();
+    if (i == 0) {
+      first_script_sfi = i::handle(GetSharedFunctionInfo(script), i_isolate);
+    } else {
+      CHECK_EQ(*first_script_sfi, GetSharedFunctionInfo(script));
+    }
+    CHECK(!script->Run(context()).IsEmpty());
+  }
+}
+
 }  // namespace v8