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

This reverts commit 61193620.

Reason for revert: Blocking roll: https://chromium-review.googlesource.com/c/chromium/src/+/3802992/

Original change's description:
> [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}

Bug: chromium:1337690
Change-Id: Iaece8f51883c7d001fb18ef48faaf271c48b8f11
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3804245
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Owners-Override: Leszek Swirski <leszeks@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#82127}

src/DEPS

@@ -18,7 +18,6 @@ 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,6 +63,7 @@
 #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"
@@ -6039,8 +6040,7 @@ void v8::Object::SetAlignedPointerInInternalField(int index, void* value) {
                       .store_aligned_pointer(obj->GetIsolate(), value),
                   location, "Unaligned pointer");
   DCHECK_EQ(value, GetAlignedPointerFromInternalField(index));
-  obj->GetHeap()->local_embedder_heap_tracer()->WriteBarrierForEmbedderField(
-      i::JSObject::cast(*obj), index, value);
+  internal::WriteBarrier::MarkingFromInternalFields(i::JSObject::cast(*obj));
 }
 
 void v8::Object::SetAlignedPointerInInternalFields(int argc, int indices[],
@@ -6063,8 +6063,7 @@ void v8::Object::SetAlignedPointerInInternalFields(int argc, int indices[],
                     location, "Unaligned pointer");
     DCHECK_EQ(value, GetAlignedPointerFromInternalField(index));
   }
-  obj->GetHeap()->local_embedder_heap_tracer()->WriteBarrierForEmbedderFields(
-      js_obj, argc, indices, values);
+  internal::WriteBarrier::MarkingFromInternalFields(js_obj);
 }
 
 // --- E n v i r o n m e n t ---

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

@@ -5,11 +5,9 @@
 #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/cppgc/heap-object-header.h"
-#include "src/heap/cppgc/page-memory.h"
-#include "src/objects/embedder-data-slot-inl.h"
+#include "src/heap/embedder-tracing-inl.h"
+#include "src/objects/embedder-data-slot.h"
 #include "src/objects/js-objects.h"
 
 namespace v8 {
@@ -19,39 +17,28 @@ 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_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.
+      map, object, wrapper_descriptor_.wrappable_type_index, snapshot.first);
+  EmbedderDataSlot::PopulateEmbedderDataSnapshot(
+      map, object, wrapper_descriptor_.wrappable_instance_index,
+      snapshot.second);
   return true;
-#endif
 }
 
 void CppMarkingState::MarkAndPush(const EmbedderDataSnapshot& snapshot) {
-  marking_state_.MarkAndPush(
-      cppgc::internal::HeapObjectHeader::FromObject(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));
+  }
 }
 
 }  // namespace internal

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

@@ -7,10 +7,10 @@
 
 #include <memory>
 
-#include "include/v8-cppgc.h"
+#include "src/heap/cppgc-js/cpp-heap.h"
 #include "src/heap/cppgc/marking-state.h"
 #include "src/heap/cppgc/marking-worklists.h"
-#include "src/objects/map.h"
+#include "src/objects/embedder-data-slot.h"
 
 namespace v8 {
 namespace internal {
@@ -20,7 +20,9 @@ class EmbedderDataSlot;
 
 class CppMarkingState {
  public:
-  using EmbedderDataSnapshot = void*;
+  using EmbedderDataSnapshot =
+      std::pair<EmbedderDataSlot::EmbedderDataSlotSnapshot,
+                EmbedderDataSlot::EmbedderDataSlotSnapshot>;
 
   CppMarkingState(Isolate* isolate, const WrapperDescriptor& wrapper_descriptor,
                   cppgc::internal::MarkingStateBase& main_thread_marking_state)
@@ -44,6 +46,8 @@ 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,8 +7,6 @@
 #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"
@@ -187,6 +185,26 @@ 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);
@@ -200,94 +218,5 @@ 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,11 +158,7 @@ class V8_EXPORT_PRIVATE LocalEmbedderHeapTracer final {
     return embedder_stack_state_;
   }
 
-  // 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*[]);
+  void EmbedderWriteBarrier(Heap*, JSObject);
 
  private:
   static constexpr size_t kEmbedderAllocatedThreshold = 128 * KB;

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

@@ -349,6 +349,14 @@ 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,6 +48,14 @@ 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,6 +62,7 @@ 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*);
@@ -87,6 +88,7 @@ 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,19 +293,18 @@ template <typename ConcreteVisitor, typename MarkingState>
 template <typename T>
 inline int MarkingVisitorBase<ConcreteVisitor, MarkingState>::
     VisitEmbedderTracingSubClassWithEmbedderTracing(Map map, T object) {
-  const bool supports_snapshot =
+  const bool requires_snapshot =
       local_marking_worklists_->SupportsExtractWrapper();
   MarkingWorklists::Local::WrapperSnapshot wrapper_snapshot;
-  const bool has_valid_snapshot =
-      supports_snapshot &&
+  const bool valid_snapshot =
+      requires_snapshot &&
       local_marking_worklists_->ExtractWrapper(map, object, wrapper_snapshot);
   const int size = concrete_visitor()->VisitJSObjectSubclass(map, object);
   if (size) {
-    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.
+    if (valid_snapshot) {
+      // Success: The object needs to be processed for embedder references.
       local_marking_worklists_->PushExtractedWrapper(wrapper_snapshot);
-    } else if (!supports_snapshot) {
+    } else if (!requires_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 = void*;
+  using WrapperSnapshot = CppMarkingState::EmbedderDataSnapshot;
   inline bool ExtractWrapper(Map map, JSObject object,
                              WrapperSnapshot& snapshot);
   inline void PushExtractedWrapper(const WrapperSnapshot& snapshot);

src/init/v8.cc

@@ -19,7 +19,6 @@
 #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"