[wasm][gc] Fix infinite GC loop

One fundamental assumption of the wasm code GC is that code becomes
"potentially dead" at most once; if the ref counts drops to zero later,
it should be freed for real.
In the current implementation, it happens that code becomes potentially
dead, then becomes dead for real (it's removed from the set of
potentially dead code), and then we remove the last reference. At that
point, we re-add the code to the potentially dead code, considering it
for garbage collection again. This can lead to an endless loop.

This CL fixes that by remembering which code was already detected as
dead, and does not consider this code for another GC.
This requires freeing code via the {WasmEngine} such that the set of
dead code can be cleaned up.

R=mstarzinger@chromium.org

Bug: v8:8217
Change-Id: If6a95a7918db2ad82edfad5447c536593243db7d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1585845Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#61073}

src/wasm/wasm-code-manager.cc

@@ -400,7 +400,7 @@ V8_WARN_UNUSED_RESULT bool WasmCode::DecRefOnPotentiallyDeadCode() {
 }
 
 // static
-void WasmCode::DecrementRefCount(Vector<WasmCode*> code_vec) {
+void WasmCode::DecrementRefCount(Vector<WasmCode* const> code_vec) {
   // Decrement the ref counter of all given code objects. Keep the ones whose
   // ref count drops to zero.
   std::unordered_map<NativeModule*, std::vector<WasmCode*>> dead_code;
@@ -412,7 +412,7 @@ void WasmCode::DecrementRefCount(Vector<WasmCode*> code_vec) {
   for (auto& dead_code_entry : dead_code) {
     NativeModule* native_module = dead_code_entry.first;
     Vector<WasmCode*> code_vec = VectorOf(dead_code_entry.second);
-    native_module->FreeCode(code_vec);
+    native_module->engine()->FreeDeadCode(native_module, code_vec);
   }
 }
 

src/wasm/wasm-code-manager.h

@@ -170,7 +170,7 @@ class V8_EXPORT_PRIVATE WasmCode final {
 
   // Decrement the ref count on a set of {WasmCode} objects, potentially
   // belonging to different {NativeModule}s. Dead code will be deleted.
-  static void DecrementRefCount(Vector<WasmCode*>);
+  static void DecrementRefCount(Vector<WasmCode* const>);
 
   enum FlushICache : bool { kFlushICache = true, kNoFlushICache = false };
 
@@ -408,6 +408,8 @@ class V8_EXPORT_PRIVATE NativeModule final {
   // Free a set of functions of this module. Uncommits whole pages if possible.
   // The given vector must be ordered by the instruction start address, and all
   // {WasmCode} objects must not be used any more.
+  // Should only be called via {WasmEngine::FreeDeadCode}, so the engine can do
+  // its accounting.
   void FreeCode(Vector<WasmCode* const>);
 
  private:
@@ -663,9 +665,7 @@ class GlobalWasmCodeRef {
     code_->IncRef();
   }
 
-  ~GlobalWasmCodeRef() {
-    if (code_->DecRef()) code_->native_module()->FreeCode(VectorOf(&code_, 1));
-  }
+  ~GlobalWasmCodeRef() { WasmCode::DecrementRefCount({&code_, 1}); }
 
   // Get a pointer to the contained {WasmCode} object. This is only guaranteed
   // to exist as long as this {GlobalWasmCodeRef} exists.

src/wasm/wasm-engine.cc

@@ -150,10 +150,15 @@ struct WasmEngine::NativeModuleInfo {
   // Set of isolates using this NativeModule.
   std::unordered_set<Isolate*> isolates;
 
-  // Set of potentially dead code. The ref-count of these code objects was
-  // incremented for each Isolate that might still execute the code, and is
-  // decremented on {RemoveIsolate} or on a GC.
+  // Set of potentially dead code. This set holds one ref for each code object,
+  // until code is detected to be really dead. At that point, the ref count is
+  // decremented and code is move to the {dead_code} set. If the code is finally
+  // deleted, it is also removed from {dead_code}.
   std::unordered_set<WasmCode*> potentially_dead_code;
+
+  // Code that is not being executed in any isolate any more, but the ref count
+  // did not drop to zero yet.
+  std::unordered_set<WasmCode*> dead_code;
 };
 
 WasmEngine::WasmEngine()
@@ -692,20 +697,22 @@ void WasmEngine::ReportLiveCodeForGC(Isolate* isolate,
     if (fg_task) fg_task->Cancel();
     current_gc_info_->outstanding_isolates.erase(outstanding_isolate_it);
     for (WasmCode* code : live_code) current_gc_info_->dead_code.erase(code);
-
-    if (current_gc_info_->outstanding_isolates.empty()) {
-      // All remaining code in {current_gc_info->dead_code} is really dead.
-      // Remove it from the set of potentially dead code, and decrement its ref
-      // count.
-      dead_code = OwnedVector<WasmCode*>::Of(current_gc_info_->dead_code);
-      for (WasmCode* code : dead_code) {
-        DCHECK_EQ(1, native_modules_.count(code->native_module()));
-        auto* native_module_info = native_modules_[code->native_module()].get();
-        DCHECK_EQ(1, native_module_info->potentially_dead_code.count(code));
-        native_module_info->potentially_dead_code.erase(code);
-      }
-      current_gc_info_.reset();
+    // If there are more outstanding isolates, return here.
+    if (!current_gc_info_->outstanding_isolates.empty()) return;
+
+    // All remaining code in {current_gc_info->dead_code} is really dead.
+    // Move it from the set of potentially dead code to the set of dead code,
+    // and decrement its ref count.
+    dead_code = OwnedVector<WasmCode*>::Of(current_gc_info_->dead_code);
+    for (WasmCode* code : dead_code) {
+      DCHECK_EQ(1, native_modules_.count(code->native_module()));
+      auto* native_module_info = native_modules_[code->native_module()].get();
+      DCHECK_EQ(1, native_module_info->potentially_dead_code.count(code));
+      native_module_info->potentially_dead_code.erase(code);
+      DCHECK_EQ(0, native_module_info->dead_code.count(code));
+      native_module_info->dead_code.insert(code);
     }
+    current_gc_info_.reset();
   }
   if (!dead_code.empty()) WasmCode::DecrementRefCount(dead_code.as_vector());
 }
@@ -714,6 +721,7 @@ bool WasmEngine::AddPotentiallyDeadCode(WasmCode* code) {
   base::MutexGuard guard(&mutex_);
   auto it = native_modules_.find(code->native_module());
   DCHECK_NE(native_modules_.end(), it);
+  if (it->second->dead_code.count(code)) return false;  // Code is already dead.
   auto added = it->second->potentially_dead_code.insert(code);
   if (!added.second) return false;  // An entry already existed.
   new_potentially_dead_code_size_ += code->instructions().size();
@@ -732,6 +740,21 @@ bool WasmEngine::AddPotentiallyDeadCode(WasmCode* code) {
   return true;
 }
 
+void WasmEngine::FreeDeadCode(NativeModule* native_module,
+                              Vector<WasmCode* const> codes) {
+  {
+    base::MutexGuard guard(&mutex_);
+    DCHECK_EQ(1, native_modules_.count(native_module));
+    auto* info = native_modules_[native_module].get();
+    for (WasmCode* code : codes) {
+      DCHECK_EQ(1, info->dead_code.count(code));
+      info->dead_code.erase(code);
+    }
+  }
+
+  native_module->FreeCode(codes);
+}
+
 void WasmEngine::TriggerGC() {
   DCHECK_NULL(current_gc_info_);
   DCHECK(FLAG_wasm_code_gc);

src/wasm/wasm-engine.h

@@ -195,6 +195,9 @@ class V8_EXPORT_PRIVATE WasmEngine {
   // case.
   V8_WARN_UNUSED_RESULT bool AddPotentiallyDeadCode(WasmCode*);
 
+  // Free dead code on a native module.
+  void FreeDeadCode(NativeModule*, Vector<WasmCode* const>);
+
   // Call on process start and exit.
   static void InitializeOncePerProcess();
   static void GlobalTearDown();