From 4a3d24cd677fe4fbdc643c3ad3d100dcdf2d3fa8 Mon Sep 17 00:00:00 2001
From: Olivier Flückiger <[email protected]>
Date: Thu, 16 Jul 2026 12:00:07 +0000
Subject: [PATCH] [sandbox] Re-Introduc CAS loops in JSDispatchEntry

Remove the invariant that the JSDispatchEntry WB needs to be invoked
on every object, to fix the mark bit race.

Additionally, this CL:
- Checks !is_marking() in SetCodeAndEntrypointPointer to have a fast
  normal release store when no marker is running.
- Bottlenecks Code updates and WB in the JSDispatchTable.
- Updates the WB verification accordingly.

Bug: 525686865, 525689847

Change-Id: I40d00400e952bc0e0d9114e5942f3da4e0320e65
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/8064734
Reviewed-by: Dominik Inführ <[email protected]>
Reviewed-by: Samuel Groß <[email protected]>
Auto-Submit: Olivier Flückiger <[email protected]>
Commit-Queue: Samuel Groß <[email protected]>
Cr-Commit-Position: refs/heads/main@{#108711}
---
diff --git a/src/heap/factory.cc b/src/heap/factory.cc
index 66368b0..c461222 100644
--- a/src/heap/factory.cc
+++ b/src/heap/factory.cc
@@ -5472,28 +5472,20 @@
       JSDispatchTable& jdt = isolate_->js_dispatch_table();
       Tagged<Code> old_code = jdt.GetCode(dispatch_handle);
 
-      // A write barrier is needed when settings code, because the update can
-      // race with marking which could leave the dispatch slot unmarked.
-      // TODO(olivf): This should be fixed by using a more traditional WB
-      // for dispatch handles (i.e. have a marking queue with dispatch handles
-      // instead of marking through the handle).
-      constexpr WriteBarrierMode mode_if_setting_code =
-          WriteBarrierMode::UPDATE_WRITE_BARRIER;
-
       // TODO(olivf): We should go through the cases where this is still
       // needed and maybe find some alternative to initialize it correctly
       // from the beginning.
       if (old_code->is_builtin()) {
-        jdt.SetCodeNoWriteBarrier(dispatch_handle, *code);
-        function->set_dispatch_handle(dispatch_handle, mode_if_setting_code);
+        jdt.SetCodeNoWriteBarrier(dispatch_handle, *code, isolate);
+        function->set_dispatch_handle(dispatch_handle, mode);
       } else {
         // On a transition of a feedback cell from one closure to many, make
         // sure that the code on the feedback cell isn't native context
         // specialized, and if it was, eagerly re-optimize.
         if (cell_transition == FeedbackCell::kOneToMany &&
             old_code->is_context_specialized()) {
-          jdt.SetCodeNoWriteBarrier(dispatch_handle, *code);
-          function->set_dispatch_handle(dispatch_handle, mode_if_setting_code);
+          jdt.SetCodeNoWriteBarrier(dispatch_handle, *code, isolate);
+          function->set_dispatch_handle(dispatch_handle, mode);
           DCHECK(old_code->kind() == CodeKind::MAGLEV ||
                  old_code->kind() == CodeKind::TURBOFAN_JS);
           if (!old_code->marked_for_deoptimization()) {
diff --git a/src/heap/heap-write-barrier-inl.h b/src/heap/heap-write-barrier-inl.h
index 32bce78..564a3b0 100644
--- a/src/heap/heap-write-barrier-inl.h
+++ b/src/heap/heap-write-barrier-inl.h
@@ -295,7 +295,7 @@
                                        WriteBarrierMode mode) {
 #if V8_VERIFY_WRITE_BARRIERS
   if (v8_flags.verify_write_barriers) {
-    CHECK(WriteBarrier::VerifyDispatchHandleMarkingState(host, handle, mode));
+    WriteBarrier::VerifyDispatchHandleWriteBarrier(host, handle, mode);
   }
 #endif  // V8_VERIFY_WRITE_BARRIERS
   if (mode == SKIP_WRITE_BARRIER) {
diff --git a/src/heap/heap-write-barrier.cc b/src/heap/heap-write-barrier.cc
index 479bf55..378b75b 100644
--- a/src/heap/heap-write-barrier.cc
+++ b/src/heap/heap-write-barrier.cc
@@ -191,7 +191,6 @@
   // Mark both the table entry and its content.
   Isolate* isolate = Isolate::Current();
   JSDispatchTable& jdt = isolate->js_dispatch_table();
-  static_assert(JSDispatchTable::kWriteBarrierSetsEntryMarkBit);
 #ifdef DEBUG
   Heap* heap = isolate->heap();
   jdt.VerifyEntry(handle, heap->js_dispatch_table_space(),
@@ -574,42 +573,36 @@
 #if V8_VERIFY_WRITE_BARRIERS
 
 // static
-bool WriteBarrier::VerifyDispatchHandleMarkingState(Tagged<HeapObject> host,
+void WriteBarrier::VerifyDispatchHandleWriteBarrier(Tagged<HeapObject> host,
                                                     JSDispatchHandle handle,
                                                     WriteBarrierMode mode) {
   JSDispatchTable& jdt = Isolate::Current()->js_dispatch_table();
   Tagged<Code> value = jdt.GetCode(handle);
 
   if (mode == SKIP_WRITE_BARRIER) {
+    // Builtins do not need write barriers, neither do their JSDispatch handles
+    // as the handles don't move and are immortal. This additional check is
+    // needed since in some configurations and cctests the builtins Code objects
+    // are not in R/O space.
     if (value->is_builtin()) {
-      // Builtins are immortal and immovable, so no write barrier needed.
-      BasePage* page = BasePage::FromHeapObject(value);
-      DCHECK(page->never_evacuate());
-      return true;
+      static_assert(!JSDispatchTable::kSupportsCompaction);
+      return;
     }
 
-    if (WriteBarrier::IsRequired(host, value)) {
-      return false;
+    // First check the barrier for the value
+    VerifySkipWriteBarrier(host, value, mode);
+
+    // Now check the barrier for the handle itself
+    if (jdt.InReadOnlySegment(handle)) {
+      return;
     }
+    if (IsMostRecentYoungAllocation(host->address())) {
+      return;
+    }
+    UNREACHABLE();
+  } else {
+    DCHECK_EQ(mode, UPDATE_WRITE_BARRIER);
   }
-
-  if (CurrentMarkingBarrier(host)->is_not_major()) return true;
-
-  // Ensure we don't have a black -> white -> black edge. This could happen when
-  // skipping a write barrier while concurrently the dispatch entry is marked
-  // from another JSFunction.
-  if (ReadOnlyHeap::Contains(host) ||
-      (IsMarking(host) && mode != SKIP_WRITE_BARRIER) ||
-      !CurrentMarkingBarrier(host)->IsMarked(host)) {
-    return true;
-  }
-  if (jdt.IsMarked(handle)) {
-    return true;
-  }
-  if (ReadOnlyHeap::Contains(value)) {
-    return true;
-  }
-  return !CurrentMarkingBarrier(host)->IsMarked(value);
 }
 
 #endif  // V8_VERIFY_WRITE_BARRIERS
diff --git a/src/heap/heap-write-barrier.h b/src/heap/heap-write-barrier.h
index 4439a05..bc075fc 100644
--- a/src/heap/heap-write-barrier.h
+++ b/src/heap/heap-write-barrier.h
@@ -154,7 +154,7 @@
   template <typename T>
   static inline bool IsRequired(const HeapObject* host, T value);
 
-  static bool VerifyDispatchHandleMarkingState(Tagged<HeapObject> host,
+  static void VerifyDispatchHandleWriteBarrier(Tagged<HeapObject> host,
                                                JSDispatchHandle value,
                                                WriteBarrierMode mode);
 #endif  // V8_VERIFY_WRITE_BARRIERS
diff --git a/src/heap/mark-compact.cc b/src/heap/mark-compact.cc
index 6881d42..5fa5816 100644
--- a/src/heap/mark-compact.cc
+++ b/src/heap/mark-compact.cc
@@ -3295,8 +3295,8 @@
                              code->kind() == CodeKind::TURBOFAN_JS ||
                              code->is_interpreter_trampoline_builtin());
                       entry.SetCodeAndEntrypointPointer(
-                          compile_lazy.ptr(),
-                          compile_lazy->instruction_start());
+                          compile_lazy.ptr(), compile_lazy->instruction_start(),
+                          isolate);
                     }
                   });
       })
@@ -5998,7 +5998,8 @@
 #undef CASE
             return code->instruction_start();
           })();
-          jdt.SetCodeAndEntrypointNoWriteBarrier(handle, code, new_entrypoint);
+          jdt.SetCodeAndEntrypointNoWriteBarrier(handle, code, new_entrypoint,
+                                                 heap_->isolate());
           CHECK_IMPLIES(jdt.IsTieringRequested(handle),
                         old_entrypoint == new_entrypoint);
         }
diff --git a/src/heap/read-only-promotion.cc b/src/heap/read-only-promotion.cc
index 254ed31..de6cad2 100644
--- a/src/heap/read-only-promotion.cc
+++ b/src/heap/read-only-promotion.cc
@@ -514,7 +514,8 @@
           Tagged<HeapObject> new_code = it->second;
           CHECK(IsCode(new_code));
           // TODO(saelo): is it worth logging something in this case?
-          jdt.SetCodeNoWriteBarrier(handle, TrustedCast<Code>(new_code));
+          jdt.SetCodeNoWriteBarrier(handle, TrustedCast<Code>(new_code),
+                                    isolate);
         });
     // Note the we should technically also update the entries in the
     // read_only_js_dispatch_table_space but it's currently not needed as we
diff --git a/src/objects/code.cc b/src/objects/code.cc
index 62e6371..20b4a94 100644
--- a/src/objects/code.cc
+++ b/src/objects/code.cc
@@ -180,7 +180,8 @@
       if (v8_flags.reopt_after_lazy_deopts &&
           isolate->concurrent_recompilation_enabled()) {
         jdt.SetCodeNoWriteBarrier(
-            handle, *BUILTIN_CODE(isolate, InterpreterEntryTrampoline));
+            handle, *BUILTIN_CODE(isolate, InterpreterEntryTrampoline),
+            isolate);
         // Somewhat arbitrary list of lazy deopt reasons which we expect to be
         // stable enough to warrant either immediate re-optimization, or
         // re-optimization after one invocation (to detect potential follow-up
@@ -215,12 +216,9 @@
             break;
         }
       } else {
-        jdt.SetCodeNoWriteBarrier(handle, *BUILTIN_CODE(isolate, CompileLazy));
+        jdt.SetCodeNoWriteBarrier(handle, *BUILTIN_CODE(isolate, CompileLazy),
+                                  isolate);
       }
-      // TODO(olivf, 525686865): Fix the race in dispatch handle marking by
-      // going back to a CAS loop.
-      static_assert(JSDispatchTable::kWriteBarrierSetsEntryMarkBit);
-      jdt.Mark(handle);
     }
     // Ensure we don't try to patch the entry multiple times.
     set_js_dispatch_handle(kNullJSDispatchHandle);
diff --git a/src/objects/js-function-inl.h b/src/objects/js-function-inl.h
index 253a444..d5fd5df 100644
--- a/src/objects/js-function-inl.h
+++ b/src/objects/js-function-inl.h
@@ -218,17 +218,14 @@
 }
 void JSFunction::UpdateDispatchEntry(Isolate* isolate, Tagged<Code> new_code,
                                      WriteBarrierMode mode) {
-  JSDispatchHandle handle = dispatch_handle();
-  isolate->js_dispatch_table().SetCodeNoWriteBarrier(handle, new_code);
-  WriteBarrier::ForJSDispatchHandle(this, handle, mode);
+  isolate->js_dispatch_table().SetCode(dispatch_handle(), new_code, this,
+                                       isolate, mode);
 }
 void JSFunction::UpdateDispatchEntryKeepTieringRequest(Isolate* isolate,
                                                        Tagged<Code> new_code,
                                                        WriteBarrierMode mode) {
-  JSDispatchHandle handle = dispatch_handle();
-  isolate->js_dispatch_table().SetCodeKeepTieringRequestNoWriteBarrier(
-      handle, new_code);
-  WriteBarrier::ForJSDispatchHandle(this, handle, mode);
+  isolate->js_dispatch_table().SetCodeKeepTieringRequest(
+      dispatch_handle(), new_code, this, isolate, mode);
 }
 JSDispatchHandle JSFunction::dispatch_handle() const {
   return dispatch_handle_.Relaxed_Load();
diff --git a/src/sandbox/js-dispatch-table-inl.h b/src/sandbox/js-dispatch-table-inl.h
index 3e5d3dd..e56cb58 100644
--- a/src/sandbox/js-dispatch-table-inl.h
+++ b/src/sandbox/js-dispatch-table-inl.h
@@ -12,6 +12,7 @@
 
 #include "src/builtins/builtins-inl.h"
 #include "src/common/code-memory-access-inl.h"
+#include "src/execution/isolate.h"
 #include "src/objects/objects-inl.h"
 #include "src/sandbox/external-entity-table-inl.h"
 #include "src/snapshot/embedded/embedded-data.h"
@@ -88,23 +89,42 @@
 }
 
 void JSDispatchTable::SetCodeNoWriteBarrier(JSDispatchHandle handle,
-                                            Tagged<Code> new_code) {
+                                            Tagged<Code> new_code,
+                                            Isolate* isolate) {
   SetCodeAndEntrypointNoWriteBarrier(handle, new_code,
-                                     new_code->instruction_start());
+                                     new_code->instruction_start(), isolate);
+}
+
+void JSDispatchTable::SetCode(JSDispatchHandle handle, Tagged<Code> new_code,
+                              Tagged<HeapObject> host, Isolate* isolate,
+                              WriteBarrierMode mode) {
+  SetCodeNoWriteBarrier(handle, new_code, isolate);
+  WriteBarrier::ForJSDispatchHandle(host, handle, mode);
 }
 
 void JSDispatchTable::SetCodeKeepTieringRequestNoWriteBarrier(
-    JSDispatchHandle handle, Tagged<Code> new_code) {
+    JSDispatchHandle handle, Tagged<Code> new_code, Isolate* isolate) {
   if (IsTieringRequested(handle)) {
-    SetCodeAndEntrypointNoWriteBarrier(handle, new_code, GetEntrypoint(handle));
+    SetCodeAndEntrypointNoWriteBarrier(handle, new_code, GetEntrypoint(handle),
+                                       isolate);
   } else {
     SetCodeAndEntrypointNoWriteBarrier(handle, new_code,
-                                       new_code->instruction_start());
+                                       new_code->instruction_start(), isolate);
   }
 }
 
+void JSDispatchTable::SetCodeKeepTieringRequest(JSDispatchHandle handle,
+                                                Tagged<Code> new_code,
+                                                Tagged<HeapObject> host,
+                                                Isolate* isolate,
+                                                WriteBarrierMode mode) {
+  SetCodeKeepTieringRequestNoWriteBarrier(handle, new_code, isolate);
+  WriteBarrier::ForJSDispatchHandle(host, handle, mode);
+}
+
 void JSDispatchTable::SetCodeAndEntrypointNoWriteBarrier(
-    JSDispatchHandle handle, Tagged<Code> new_code, Address new_entrypoint) {
+    JSDispatchHandle handle, Tagged<Code> new_code, Address new_entrypoint,
+    Isolate* isolate) {
   SBXCHECK(IsCompatibleCode(new_code, GetParameterCount(handle)));
 
   // The object should be in old space to avoid creating old-to-new references.
@@ -119,7 +139,8 @@
   uint32_t index = HandleToIndex(handle);
   DCHECK_GE(index, kEndOfReadOnlyIndex);
   CFIMetadataWriteScope write_scope("JSDispatchTable update");
-  at(index).SetCodeAndEntrypointPointer(new_code.ptr(), new_entrypoint);
+  at(index).SetCodeAndEntrypointPointer(new_code.ptr(), new_entrypoint,
+                                        isolate);
 }
 
 void JSDispatchTable::SetTieringRequest(JSDispatchHandle handle,
@@ -199,7 +220,8 @@
 }
 
 void JSDispatchEntry::SetCodeAndEntrypointPointer(Address new_object,
-                                                  Address new_entrypoint) {
+                                                  Address new_entrypoint,
+                                                  Isolate* isolate) {
   Address old_payload = encoded_word_.load(std::memory_order_relaxed);
   Address marking_bit = old_payload & kMarkingBit;
   Address parameter_count = old_payload & kParameterCountMask;
@@ -210,7 +232,20 @@
       ~kMarkingBit;
   Address new_payload = object | marking_bit | parameter_count;
   entrypoint_.store(new_entrypoint, std::memory_order_relaxed);
-  encoded_word_.store(new_payload, std::memory_order_release);
+  if (!isolate->isolate_data()->is_marking()) {
+    encoded_word_.store(new_payload, std::memory_order_release);
+  } else {
+    while (true) {
+      if (encoded_word_.compare_exchange_weak(old_payload, new_payload,
+                                              std::memory_order_release,
+                                              std::memory_order_relaxed)) {
+        break;
+      }
+      marking_bit = old_payload & kMarkingBit;
+      DCHECK_EQ(parameter_count, old_payload & kParameterCountMask);
+      new_payload = object | marking_bit | parameter_count;
+    }
+  }
   DCHECK(!IsFreelistEntry());
 }
 
@@ -268,17 +303,11 @@
 }
 
 void JSDispatchEntry::Mark() {
-  Address old_value = encoded_word_.load(std::memory_order_relaxed);
-  Address new_value = old_value | kMarkingBit;
-  // We don't need this cas to succeed. If marking races with
-  // `SetCodeAndEntrypointPointer`, then we are bound to re-set the mark bit in
-  // the write barrier.
-  static_assert(JSDispatchTable::kWriteBarrierSetsEntryMarkBit);
-  encoded_word_.compare_exchange_strong(old_value, new_value,
-                                        std::memory_order_relaxed);
+  encoded_word_.fetch_or(kMarkingBit, std::memory_order_relaxed);
 }
 
 void JSDispatchEntry::Unmark() {
+  DCHECK(Isolate::Current()->heap()->IsInGC());
   Address value = encoded_word_.load(std::memory_order_relaxed);
   value &= ~kMarkingBit;
   encoded_word_.store(value, std::memory_order_relaxed);
diff --git a/src/sandbox/js-dispatch-table.h b/src/sandbox/js-dispatch-table.h
index 7737f8b..0912c40 100644
--- a/src/sandbox/js-dispatch-table.h
+++ b/src/sandbox/js-dispatch-table.h
@@ -9,6 +9,7 @@
 #include "src/base/atomicops.h"
 #include "src/base/memory.h"
 #include "src/common/globals.h"
+#include "src/objects/objects.h"
 #include "src/runtime/runtime.h"
 #include "src/sandbox/external-entity-table.h"
 
@@ -18,6 +19,7 @@
 class Isolate;
 class Counters;
 class Code;
+class HeapObject;
 enum class TieringBuiltin;
 
 /**
@@ -42,7 +44,8 @@
   inline uint16_t GetParameterCount() const;
... (file diff truncated)