[heap, api] Check assumptions for embedder fields on set

Previously, we would set embedder fields and do type checks (on
embedder fields) in the GC. This does not work nicely as embedder
fields contain system pointers whereas we can only operate with
tag-aligned reads/writes. The end result of assembling pointers was
somtimes broken for concurrent marking.

In this CL we reverse the mode and check assumptions when writing the
fields. From Blink we generally only write once and use the fields in
the GC and via reads multiple times.

We assume, that when running with CppHeap, any pointer on an instance
field that points into CppHeap, also has the type field set with the
appropriate tracing information. In debug builds we also verify that
the embedder field indeed points to the start of an Oilpan object.

Bug: chromium:1337690
Change-Id: I9f9a8e691cdcf666861a455dcf8f65f2fe80b034
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3788206
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82120}

src/DEPS

@@ -18,6 +18,7 @@ include_rules = [
   "+src/heap/basic-memory-chunk.h",
   "+src/heap/code-range.h",
   "+src/heap/combined-heap.h",
+  "+src/heap/cppgc-js/cpp-heap.h",
   "+src/heap/embedder-tracing.h",
   "+src/heap/factory.h",
   "+src/heap/factory-inl.h",

src/api/api.cc

@@ -63,7 +63,6 @@
 #include "src/handles/persistent-handles.h"
 #include "src/heap/embedder-tracing.h"
 #include "src/heap/heap-inl.h"
-#include "src/heap/heap-write-barrier.h"
 #include "src/heap/safepoint.h"
 #include "src/init/bootstrapper.h"
 #include "src/init/icu_util.h"
@@ -6040,7 +6039,8 @@ void v8::Object::SetAlignedPointerInInternalField(int index, void* value) {
                       .store_aligned_pointer(obj->GetIsolate(), value),
                   location, "Unaligned pointer");
   DCHECK_EQ(value, GetAlignedPointerFromInternalField(index));
-  internal::WriteBarrier::MarkingFromInternalFields(i::JSObject::cast(*obj));
+  obj->GetHeap()->local_embedder_heap_tracer()->WriteBarrierForEmbedderField(
+      i::JSObject::cast(*obj), index, value);
 }
 
 void v8::Object::SetAlignedPointerInInternalFields(int argc, int indices[],
@@ -6063,7 +6063,8 @@ void v8::Object::SetAlignedPointerInInternalFields(int argc, int indices[],
                     location, "Unaligned pointer");
     DCHECK_EQ(value, GetAlignedPointerFromInternalField(index));
   }
-  internal::WriteBarrier::MarkingFromInternalFields(js_obj);
+  obj->GetHeap()->local_embedder_heap_tracer()->WriteBarrierForEmbedderFields(
+      js_obj, argc, indices, values);
 }
 
 // --- E n v i r o n m e n t ---

src/heap/cppgc-js/cpp-marking-state-inl.h

@@ -5,9 +5,11 @@
 #ifndef V8_HEAP_CPPGC_JS_CPP_MARKING_STATE_INL_H_
 #define V8_HEAP_CPPGC_JS_CPP_MARKING_STATE_INL_H_
 
+#include "src/heap/cppgc-js/cpp-heap.h"
 #include "src/heap/cppgc-js/cpp-marking-state.h"
-#include "src/heap/embedder-tracing-inl.h"
-#include "src/objects/embedder-data-slot.h"
+#include "src/heap/cppgc/heap-object-header.h"
+#include "src/heap/cppgc/page-memory.h"
+#include "src/objects/embedder-data-slot-inl.h"
 #include "src/objects/js-objects.h"
 
 namespace v8 {
@@ -17,28 +19,39 @@ bool CppMarkingState::ExtractEmbedderDataSnapshot(
     Map map, JSObject object, EmbedderDataSnapshot& snapshot) {
   if (JSObject::GetEmbedderFieldCount(map) < 2) return false;
 
+  EmbedderDataSlot::EmbedderDataSlotSnapshot slot_snapshot;
   EmbedderDataSlot::PopulateEmbedderDataSnapshot(
-      map, object, wrapper_descriptor_.wrappable_type_index, snapshot.first);
-  EmbedderDataSlot::PopulateEmbedderDataSnapshot(
-      map, object, wrapper_descriptor_.wrappable_instance_index,
-      snapshot.second);
+      map, object, wrapper_descriptor_.wrappable_instance_index, slot_snapshot);
+  // Check whether snapshot is valid.
+  const EmbedderDataSlot instance_slot(slot_snapshot);
+  bool valid = instance_slot.ToAlignedPointer(isolate_, &snapshot);
+  if (!valid || !snapshot) return false;
+#if defined(CPPGC_CAGED_HEAP)
+  // On 64-bit builds we have to check whether the snapshot captured a valid
+  // pointer. On such builds, the pointer is updated with 32-bit atomics which
+  // means that the marker may observe the high and low parts in inconsistent
+  // states. The high parts specifies the cage and the low part is always
+  // non-null when it is interesting.
+  return cppgc::internal::CagedHeapBase::IsWithinCage(snapshot) &&
+         static_cast<uintptr_t>(
+             static_cast<uint32_t>(reinterpret_cast<uintptr_t>(snapshot))) != 0;
+#elif V8_TARGET_ARCH_X64
+  const cppgc::internal::BasePage* page =
+      reinterpret_cast<const cppgc::internal::BasePage*>(
+          CppHeap::From(isolate_->heap()->cpp_heap())
+              ->page_backend()
+              ->Lookup(const_cast<cppgc::internal::ConstAddress>(
+                  reinterpret_cast<cppgc::internal::Address>(snapshot))));
+  return page != nullptr;
+#else
+  // 32-bit configuration.
   return true;
+#endif
 }
 
 void CppMarkingState::MarkAndPush(const EmbedderDataSnapshot& snapshot) {
-  const EmbedderDataSlot type_slot(snapshot.first);
-  const EmbedderDataSlot instance_slot(snapshot.second);
-  MarkAndPush(type_slot, instance_slot);
-}
-
-void CppMarkingState::MarkAndPush(const EmbedderDataSlot type_slot,
-                                  const EmbedderDataSlot instance_slot) {
-  LocalEmbedderHeapTracer::WrapperInfo info;
-  if (LocalEmbedderHeapTracer::ExtractWrappableInfo(
-          isolate_, wrapper_descriptor_, type_slot, instance_slot, &info)) {
-    marking_state_.MarkAndPush(
-        cppgc::internal::HeapObjectHeader::FromObject(info.second));
-  }
+  marking_state_.MarkAndPush(
+      cppgc::internal::HeapObjectHeader::FromObject(snapshot));
 }
 
 }  // namespace internal

src/heap/cppgc-js/cpp-marking-state.h

@@ -7,10 +7,10 @@
 
 #include <memory>
 
-#include "src/heap/cppgc-js/cpp-heap.h"
+#include "include/v8-cppgc.h"
 #include "src/heap/cppgc/marking-state.h"
 #include "src/heap/cppgc/marking-worklists.h"
-#include "src/objects/embedder-data-slot.h"
+#include "src/objects/map.h"
 
 namespace v8 {
 namespace internal {
@@ -20,9 +20,7 @@ class EmbedderDataSlot;
 
 class CppMarkingState {
  public:
-  using EmbedderDataSnapshot =
-      std::pair<EmbedderDataSlot::EmbedderDataSlotSnapshot,
-                EmbedderDataSlot::EmbedderDataSlotSnapshot>;
+  using EmbedderDataSnapshot = void*;
 
   CppMarkingState(Isolate* isolate, const WrapperDescriptor& wrapper_descriptor,
                   cppgc::internal::MarkingStateBase& main_thread_marking_state)
@@ -46,8 +44,6 @@ class CppMarkingState {
   inline bool ExtractEmbedderDataSnapshot(Map, JSObject, EmbedderDataSnapshot&);
 
   inline void MarkAndPush(const EmbedderDataSnapshot&);
-  inline void MarkAndPush(const EmbedderDataSlot type_slot,
-                          const EmbedderDataSlot instance_slot);
 
   bool IsLocalEmpty() {
     return marking_state_.marking_worklist().IsLocalEmpty();

src/heap/embedder-tracing.cc

@@ -7,6 +7,8 @@
 #include "include/v8-cppgc.h"
 #include "src/base/logging.h"
 #include "src/handles/global-handles.h"
+#include "src/heap/cppgc/globals.h"
+#include "src/heap/cppgc/page-memory.h"
 #include "src/heap/embedder-tracing-inl.h"
 #include "src/heap/gc-tracer.h"
 #include "src/heap/marking-worklist-inl.h"
@@ -185,26 +187,6 @@ void LocalEmbedderHeapTracer::NotifyEmptyEmbedderStack() {
   isolate_->global_handles()->NotifyEmptyEmbedderStack();
 }
 
-void LocalEmbedderHeapTracer::EmbedderWriteBarrier(Heap* heap,
-                                                   JSObject js_object) {
-  DCHECK(InUse());
-  DCHECK(js_object.MayHaveEmbedderFields());
-  if (cpp_heap_) {
-    DCHECK_NOT_NULL(heap->mark_compact_collector());
-    const EmbedderDataSlot type_slot(js_object,
-                                     wrapper_descriptor_.wrappable_type_index);
-    const EmbedderDataSlot instance_slot(
-        js_object, wrapper_descriptor_.wrappable_instance_index);
-    heap->mark_compact_collector()
-        ->local_marking_worklists()
-        ->cpp_marking_state()
-        ->MarkAndPush(type_slot, instance_slot);
-    return;
-  }
-  LocalEmbedderHeapTracer::ProcessingScope scope(this);
-  scope.TracePossibleWrapper(js_object);
-}
-
 bool DefaultEmbedderRootsHandler::IsRoot(
     const v8::TracedReference<v8::Value>& handle) {
   return !tracer_ || tracer_->IsRootForNonTracingGC(handle);
@@ -218,5 +200,94 @@ void DefaultEmbedderRootsHandler::ResetRoot(
   tracer_->ResetHandleInNonTracingGC(handle);
 }
 
+void LocalEmbedderHeapTracer::WriteBarrierForEmbedderField(JSObject host,
+                                                           int index,
+                                                           void* value) {
+  DCHECK(host.MayHaveEmbedderFields());
+  if (!InUse()) return;
+  // If the index refers to an instance index, then we must also find the
+  // appropriate type information already present on the object. Retrieve the
+  // type information and invoke the more detailed barrier.
+  if (index == wrapper_descriptor_.wrappable_instance_index) {
+    int indices[] = {wrapper_descriptor_.wrappable_type_index,
+                     wrapper_descriptor_.wrappable_instance_index};
+    void* values[] = {nullptr, value};
+    const EmbedderDataSlot type_slot(host,
+                                     wrapper_descriptor_.wrappable_type_index);
+    if (type_slot.ToAlignedPointer(isolate_, &values[0])) {
+      LocalEmbedderHeapTracer::WriteBarrierForEmbedderFields(host, 2, indices,
+                                                             values);
+    }
+  }
+}
+
+void LocalEmbedderHeapTracer::WriteBarrierForEmbedderFields(JSObject host,
+                                                            int argc,
+                                                            int indices[],
+                                                            void* values[]) {
+  DCHECK(host.MayHaveEmbedderFields());
+  if (!InUse()) return;
+
+  auto* heap = isolate_->heap();
+
+  if (!cpp_heap_) {
+    // When running without CppHeap, we have to check on visiting each instance
+    // on the main thread in the GC.
+    if (heap->incremental_marking()->IsMarking()) {
+      LocalEmbedderHeapTracer::ProcessingScope scope(this);
+      scope.TracePossibleWrapper(host);
+    }
+    return;
+  }
+
+  // For running with CppHeap, we assume that when we set a reference that
+  // points into a CppHeap on an instance index, the type index must indicate
+  // that this type should be GCed.
+
+  void* type = nullptr;
+  void* instance = nullptr;
+  for (int i = 0; i < argc; i++) {
+    if (indices[i] == wrapper_descriptor().wrappable_type_index) {
+      type = values[i];
+    }
+    if (indices[i] == wrapper_descriptor().wrappable_instance_index) {
+      instance = values[i];
+    }
+  }
+
+  if (!instance) return;
+
+  bool on_cpp_heap = false;
+#if defined(DEBUG) || !defined(CPPGC_CAGED_HEAP)
+  const cppgc::internal::BasePage* page =
+      reinterpret_cast<const cppgc::internal::BasePage*>(
+          cpp_heap_->page_backend()->Lookup(
+              const_cast<cppgc::internal::ConstAddress>(
+                  reinterpret_cast<cppgc::internal::Address>(instance))));
+#endif  //  defined(DEBUG) || !defined(CPPGC_CAGED_HEAP)
+#if defined(CPPGC_CAGED_HEAP)
+  // Bail out if the address is not contained within the cage.
+  on_cpp_heap = cppgc::internal::CagedHeapBase::IsWithinCage(instance);
+#else   // !defined(CPPGC_CAGED_HEAP)
+  on_cpp_heap = page != nullptr;
+#endif  // defined(CPPGC_CAGED_HEAP)
+
+  if (!on_cpp_heap) return;
+
+  CHECK_NOT_NULL(type);
+  CHECK_EQ(*static_cast<uint16_t*>(type),
+           wrapper_descriptor().embedder_id_for_garbage_collected);
+  // We also assume that instance points directly to a heap object.
+  DCHECK_IMPLIES(
+      on_cpp_heap,
+      page->ObjectHeaderFromInnerAddress(instance).ObjectStart() == instance);
+
+  if (heap->incremental_marking()->IsMarking())
+    heap->mark_compact_collector()
+        ->local_marking_worklists()
+        ->cpp_marking_state()
+        ->MarkAndPush(instance);
+}
+
 }  // namespace internal
 }  // namespace v8

src/heap/embedder-tracing.h

@@ -158,7 +158,11 @@ class V8_EXPORT_PRIVATE LocalEmbedderHeapTracer final {
     return embedder_stack_state_;
   }
 
-  void EmbedderWriteBarrier(Heap*, JSObject);
+  // Write barriers when setting embedder fields on JSObjects. These barriers
+  // do not mark V8 objects but rather keep the objects managed by CppHeap or
+  // EmbedderHeapTracer consistent.
+  void WriteBarrierForEmbedderField(JSObject, int, void*);
+  void WriteBarrierForEmbedderFields(JSObject, int, int[], void*[]);
 
  private:
   static constexpr size_t kEmbedderAllocatedThreshold = 128 * KB;

src/heap/heap-write-barrier-inl.h

@@ -349,14 +349,6 @@ void WriteBarrier::MarkingFromGlobalHandle(Object value) {
   MarkingSlowFromGlobalHandle(*heap, heap_value);
 }
 
-// static
-void WriteBarrier::MarkingFromInternalFields(JSObject host) {
-  if (V8_ENABLE_THIRD_PARTY_HEAP_BOOL) return;
-  auto heap = GetHeapIfMarking(host);
-  if (!heap) return;
-  MarkingSlowFromInternalFields(*heap, host);
-}
-
 #ifdef ENABLE_SLOW_DCHECKS
 // static
 template <typename T>

src/heap/heap-write-barrier.cc

@@ -48,14 +48,6 @@ void WriteBarrier::MarkingSlowFromGlobalHandle(Heap* heap, HeapObject value) {
   heap->main_thread_local_heap()->marking_barrier()->WriteWithoutHost(value);
 }
 
-// static
-void WriteBarrier::MarkingSlowFromInternalFields(Heap* heap, JSObject host) {
-  auto* local_embedder_heap_tracer = heap->local_embedder_heap_tracer();
-  if (!local_embedder_heap_tracer->InUse()) return;
-
-  local_embedder_heap_tracer->EmbedderWriteBarrier(heap, host);
-}
-
 void WriteBarrier::MarkingSlow(Heap* heap, Code host, RelocInfo* reloc_info,
                                HeapObject value) {
   MarkingBarrier* marking_barrier = CurrentMarkingBarrier(heap);

src/heap/heap-write-barrier.h

@@ -62,7 +62,6 @@ class V8_EXPORT_PRIVATE WriteBarrier {
   static int MarkingFromCode(Address raw_host, Address raw_slot);
   // Invoked from global handles where no host object is available.
   static inline void MarkingFromGlobalHandle(Object value);
-  static inline void MarkingFromInternalFields(JSObject host);
 
   static void SetForThread(MarkingBarrier*);
   static void ClearForThread(MarkingBarrier*);
@@ -88,7 +87,6 @@ class V8_EXPORT_PRIVATE WriteBarrier {
   static void MarkingSlow(Heap* heap, DescriptorArray,
                           int number_of_own_descriptors);
   static void MarkingSlowFromGlobalHandle(Heap* heap, HeapObject value);
-  static void MarkingSlowFromInternalFields(Heap* heap, JSObject host);
 
   static void SharedSlow(Heap* heap, Code host, RelocInfo*, HeapObject value);
 

src/heap/marking-visitor-inl.h

@@ -293,18 +293,19 @@ template <typename ConcreteVisitor, typename MarkingState>
 template <typename T>
 inline int MarkingVisitorBase<ConcreteVisitor, MarkingState>::
     VisitEmbedderTracingSubClassWithEmbedderTracing(Map map, T object) {
-  const bool requires_snapshot =
+  const bool supports_snapshot =
       local_marking_worklists_->SupportsExtractWrapper();
   MarkingWorklists::Local::WrapperSnapshot wrapper_snapshot;
-  const bool valid_snapshot =
-      requires_snapshot &&
+  const bool has_valid_snapshot =
+      supports_snapshot &&
       local_marking_worklists_->ExtractWrapper(map, object, wrapper_snapshot);
   const int size = concrete_visitor()->VisitJSObjectSubclass(map, object);
   if (size) {
-    if (valid_snapshot) {
-      // Success: The object needs to be processed for embedder references.
+    if (has_valid_snapshot) {
+      // Process the snapshot. If the snapshot is supported but not valid, we
+      // observed an invalid state that the write barrier will take care of.
       local_marking_worklists_->PushExtractedWrapper(wrapper_snapshot);
-    } else if (!requires_snapshot) {
+    } else if (!supports_snapshot) {
       // Snapshot not supported. Just fall back to pushing the wrapper itself
       // instead which will be processed on the main thread.
       local_marking_worklists_->PushWrapper(object);

src/heap/marking-worklist.h

@@ -162,7 +162,7 @@ class V8_EXPORT_PRIVATE MarkingWorklists::Local final {
   inline void PushOnHold(HeapObject object);
   inline bool PopOnHold(HeapObject* object);
 
-  using WrapperSnapshot = CppMarkingState::EmbedderDataSnapshot;
+  using WrapperSnapshot = void*;
   inline bool ExtractWrapper(Map map, JSObject object,
                              WrapperSnapshot& snapshot);
   inline void PushExtractedWrapper(const WrapperSnapshot& snapshot);

src/init/v8.cc

@@ -19,6 +19,7 @@
 #include "src/execution/frames.h"
 #include "src/execution/isolate.h"
 #include "src/execution/simulator.h"
+#include "src/heap/cppgc-js/cpp-heap.h"
 #include "src/init/bootstrapper.h"
 #include "src/libsampler/sampler.h"
 #include "src/objects/elements.h"