heap: Clear TracedGlobal references in dead objects on stand-alone GCs

TracedGlobal references (without callback) are eagerly cleared during
unified heap garbage collections before sweeping is started. This
change performs similar eager clearing for Blink stand-alone garbage
collections.

This allows for removing the destructor of v8::TraceGlobal and its
wrapping type TraceWrapperV8Reference.

Bug: 995684
Change-Id: Ie0394bb23ee4c5e1ecf4d1ad2a591aa77787ab00
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1760922
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688484}

third_party/blink/renderer/bindings/core/v8/v8_gc_controller.cc

@@ -243,28 +243,6 @@ void V8GCController::CollectAllGarbageForTesting(
 
 namespace {
 
-// Visitor forwarding all handle slots to the provided Blink visitor.
-class DOMWrapperSlotsForwardingVisitor final
-    : public v8::PersistentHandleVisitor,
-      public v8::EmbedderHeapTracer::TracedGlobalHandleVisitor {
- public:
-  explicit DOMWrapperSlotsForwardingVisitor(DOMWrapperSlotsVisitor* visitor)
-      : visitor_(visitor) {}
-
-  void VisitPersistentHandle(v8::Persistent<v8::Value>* value,
-                             uint16_t class_id) final {
-    visitor_->VisitSlot(value, sizeof(v8::Persistent<v8::Value>));
-  }
-
-  void VisitTracedGlobalHandle(const v8::TracedGlobal<v8::Value>& value) final {
-    visitor_->VisitSlot(&const_cast<v8::TracedGlobal<v8::Value>&>(value),
-                        sizeof(v8::TracedGlobal<v8::Value>));
-  }
-
- private:
-  DOMWrapperSlotsVisitor* const visitor_;
-};
-
 // Visitor forwarding all DOM wrapper handles to the provided Blink visitor.
 class DOMWrapperForwardingVisitor final
     : public v8::PersistentHandleVisitor,
@@ -318,15 +296,4 @@ void V8GCController::TraceDOMWrappers(v8::Isolate* isolate, Visitor* visitor) {
   tracer->IterateTracedGlobalHandles(&forwarding_visitor);
 }
 
-// static
-void V8GCController::TraceDOMWrapperSlots(v8::Isolate* isolate,
-                                          DOMWrapperSlotsVisitor* visitor) {
-  DCHECK(isolate);
-  DOMWrapperSlotsForwardingVisitor forwarding_visitor(visitor);
-  isolate->VisitHandlesWithClassIds(&forwarding_visitor);
-  v8::EmbedderHeapTracer* const tracer = static_cast<v8::EmbedderHeapTracer*>(
-      ThreadState::Current()->unified_heap_controller());
-  tracer->IterateTracedGlobalHandles(&forwarding_visitor);
-}
-
 }  // namespace blink

third_party/blink/renderer/bindings/core/v8/v8_gc_controller.h

@@ -40,7 +40,6 @@
 namespace blink {
 
 class Node;
-class DOMWrapperSlotsVisitor;
 
 class CORE_EXPORT V8GCController {
   STATIC_ONLY(V8GCController);
@@ -70,10 +69,6 @@ class CORE_EXPORT V8GCController {
   // Called upon terminating a thread when Oilpan clears references from V8
   // wrappers to DOM wrappables.
   static void ClearDOMWrappers(v8::Isolate*);
-
-  // Called for iterating wrapper slots in Oilpan object. This is e.g. used for
-  // unpoisoning memory.
-  static void TraceDOMWrapperSlots(v8::Isolate*, DOMWrapperSlotsVisitor*);
 };
 
 }  // namespace blink

third_party/blink/renderer/bindings/core/v8/v8_initializer.cc

@@ -609,7 +609,6 @@ static void InitializeV8Common(v8::Isolate* isolate) {
   isolate->AddGCEpilogueCallback(V8GCController::GcEpilogue);
   ThreadState::Current()->AttachToIsolate(
       isolate, V8GCController::TraceDOMWrappers,
-      V8GCController::TraceDOMWrapperSlots,
       EmbedderGraphBuilder::BuildEmbedderGraphCallback);
 
   isolate->SetMicrotasksPolicy(v8::MicrotasksPolicy::kScoped);

third_party/blink/renderer/platform/heap/thread_state.cc

@@ -185,12 +185,10 @@ void ThreadState::DetachCurrentThread() {
 void ThreadState::AttachToIsolate(
     v8::Isolate* isolate,
     V8TraceRootsCallback v8_trace_roots,
-    V8VisitHandleSlotsCallback v8_visit_handle_slots,
     V8BuildEmbedderGraphCallback v8_build_embedder_graph) {
   DCHECK(isolate);
   isolate_ = isolate;
   v8_trace_roots_ = v8_trace_roots;
-  v8_visit_handle_slots_ = v8_visit_handle_slots;
   v8_build_embedder_graph_ = v8_build_embedder_graph;
   unified_heap_controller_.reset(new UnifiedHeapController(this));
   isolate_->SetEmbedderHeapTracer(unified_heap_controller_.get());
@@ -209,7 +207,6 @@ void ThreadState::DetachFromIsolate() {
   }
   isolate_ = nullptr;
   v8_trace_roots_ = nullptr;
-  v8_visit_handle_slots_ = nullptr;
   v8_build_embedder_graph_ = nullptr;
   unified_heap_controller_.reset();
 }
@@ -1428,6 +1425,42 @@ void ThreadState::AtomicPauseMarkEpilogue(BlinkGC::MarkingType marking_type) {
   LeaveGCForbiddenScope();
 }
 
+namespace {
+
+class ClearReferencesInDeadObjectsVisitor final
+    : public v8::PersistentHandleVisitor,
+      public v8::EmbedderHeapTracer::TracedGlobalHandleVisitor {
+ public:
+  explicit ClearReferencesInDeadObjectsVisitor(ThreadHeap* heap)
+      : heap_(heap) {}
+
+  void VisitPersistentHandle(v8::Persistent<v8::Value>* value,
+                             uint16_t class_id) final {
+    if (InDeadObject(value))
+      value->Reset();
+  }
+
+  void VisitTracedGlobalHandle(const v8::TracedGlobal<v8::Value>& value) final {
+    // TODO(mlippautz): Avoid const_cast after changing the API to allow
+    // modificaton of the TracedGlobal handle.
+    if (InDeadObject(&const_cast<v8::TracedGlobal<v8::Value>&>(value)))
+      const_cast<v8::TracedGlobal<v8::Value>&>(value).Reset();
+  }
+
+ private:
+  bool InDeadObject(void* address) {
+    // Filter slots not on the heap.
+    if (!heap_->LookupPageForAddress(reinterpret_cast<Address>(address)))
+      return false;
+
+    return !HeapObjectHeader::FromInnerAddress(address)->IsMarked();
+  }
+
+  ThreadHeap* const heap_;
+};
+
+}  // namespace
+
 void ThreadState::AtomicPauseSweepAndCompact(
     BlinkGC::MarkingType marking_type,
     BlinkGC::SweepingType sweeping_type) {
@@ -1462,6 +1495,15 @@ void ThreadState::AtomicPauseSweepAndCompact(
   // to disallow a GC during the pre-finalizers.
   SetGCPhase(GCPhase::kSweeping);
 
+  if (!IsUnifiedHeapGC() && GetIsolate()) {
+    // Clear any Blink->V8 references in dead objects in case of a stand-alone
+    // garbage collection. This is necessary to avoid calling destructors on the
+    // references.
+    ClearReferencesInDeadObjectsVisitor visitor(&Heap());
+    GetIsolate()->VisitHandlesWithClassIds(&visitor);
+    unified_heap_controller()->IterateTracedGlobalHandles(&visitor);
+  }
+
   // Allocation is allowed during the pre-finalizers and destructors.
   // However, they must not mutate an object graph in a way in which
   // a dead object gets resurrected.
@@ -1519,11 +1561,26 @@ namespace {
 // b. Running a stand-alone V8 GC (e.g. Scavenger) that clears the handle.
 //
 // Both operations run on the main thread and not concurrent.
-class UnpoisonHandlesVisitor final : public DOMWrapperSlotsVisitor {
+class UnpoisonHandlesVisitor final
+    : public v8::PersistentHandleVisitor,
+      public v8::EmbedderHeapTracer::TracedGlobalHandleVisitor {
  public:
   explicit UnpoisonHandlesVisitor(ThreadHeap* heap) : heap_(heap) {}
 
-  void VisitSlot(void* address, size_t size) final {
+  void VisitPersistentHandle(v8::Persistent<v8::Value>* value,
+                             uint16_t class_id) final {
+    VisitSlot(value, sizeof(v8::Persistent<v8::Value>));
+  }
+
+  void VisitTracedGlobalHandle(const v8::TracedGlobal<v8::Value>& value) final {
+    // TODO(mlippautz): Avoid const_cast after changing the API to allow
+    // modificaton of the TracedGlobal handle.
+    VisitSlot(&const_cast<v8::TracedGlobal<v8::Value>&>(value),
+              sizeof(v8::TracedGlobal<v8::Value>));
+  }
+
+ private:
+  void VisitSlot(void* address, size_t size) {
     // Filter slots not on the heap.
     if (!heap_->LookupPageForAddress(reinterpret_cast<Address>(address)))
       return;
@@ -1535,7 +1592,6 @@ class UnpoisonHandlesVisitor final : public DOMWrapperSlotsVisitor {
     }
   }
 
- private:
   ThreadHeap* const heap_;
 };
 
@@ -1562,7 +1618,8 @@ void ThreadState::PoisonUnmarkedObjects() {
   // (cleared) until the destructors are run.
   if (GetIsolate()) {
     UnpoisonHandlesVisitor visitor(&Heap());
-    v8_visit_handle_slots_(GetIsolate(), &visitor);
+    GetIsolate()->VisitHandlesWithClassIds(&visitor);
+    unified_heap_controller()->IterateTracedGlobalHandles(&visitor);
   }
 }
 #endif  // ADDRESS_SANITIZER

third_party/blink/renderer/platform/heap/thread_state.h

@@ -127,14 +127,6 @@ class PLATFORM_EXPORT BlinkGCObserver {
   ThreadState* thread_state_;
 };
 
-// Used for visiting slots for DOM wrapper handles.
-class DOMWrapperSlotsVisitor {
- public:
-  // Vistation of a handle at |address| of |size|. It is *not* guaranteed that
-  // |address| is part of the managed heap.
-  virtual void VisitSlot(void* address, size_t size) = 0;
-};
-
 class PLATFORM_EXPORT ThreadState final : private RAILModeObserver {
   USING_FAST_MALLOC(ThreadState);
 
@@ -194,8 +186,6 @@ class PLATFORM_EXPORT ThreadState final : private RAILModeObserver {
   class SweepForbiddenScope;
 
   using V8TraceRootsCallback = void (*)(v8::Isolate*, Visitor*);
-  using V8VisitHandleSlotsCallback = void (*)(v8::Isolate*,
-                                              DOMWrapperSlotsVisitor*);
   using V8BuildEmbedderGraphCallback = void (*)(v8::Isolate*,
                                                 v8::EmbedderGraph*,
                                                 void*);
@@ -247,7 +237,6 @@ class PLATFORM_EXPORT ThreadState final : private RAILModeObserver {
   // there garbage collectors together.
   void AttachToIsolate(v8::Isolate*,
                        V8TraceRootsCallback,
-                       V8VisitHandleSlotsCallback,
                        V8BuildEmbedderGraphCallback);
 
   // Removes the association from a potentially attached |v8::Isolate|.
@@ -279,10 +268,7 @@ class PLATFORM_EXPORT ThreadState final : private RAILModeObserver {
 
   // Returns true if unified heap marking is in progress.
   bool IsUnifiedGCMarkingInProgress() const {
-    return IsMarkingInProgress() &&
-           (current_gc_data_.reason == BlinkGC::GCReason::kUnifiedHeapGC ||
-            current_gc_data_.reason ==
-                BlinkGC::GCReason::kUnifiedHeapForMemoryReductionGC);
+    return IsMarkingInProgress() && IsUnifiedHeapGC();
   }
 
   // Returns true if sweeping is in progress.
@@ -294,6 +280,12 @@ class PLATFORM_EXPORT ThreadState final : private RAILModeObserver {
            BlinkGC::GCReason::kUnifiedHeapForMemoryReductionGC;
   }
 
+  bool IsUnifiedHeapGC() const {
+    return current_gc_data_.reason == BlinkGC::GCReason::kUnifiedHeapGC ||
+           current_gc_data_.reason ==
+               BlinkGC::GCReason::kUnifiedHeapForMemoryReductionGC;
+  }
+
   void EnableCompactionForNextGCForTesting();
 
   void IncrementalMarkingStart(BlinkGC::GCReason);
@@ -608,7 +600,6 @@ class PLATFORM_EXPORT ThreadState final : private RAILModeObserver {
 
   v8::Isolate* isolate_ = nullptr;
   V8TraceRootsCallback v8_trace_roots_ = nullptr;
-  V8VisitHandleSlotsCallback v8_visit_handle_slots_ = nullptr;
   V8BuildEmbedderGraphCallback v8_build_embedder_graph_ = nullptr;
   std::unique_ptr<UnifiedHeapController> unified_heap_controller_;