From 81078e2bc2ad3c12bff0fb08577443291f4558ab Mon Sep 17 00:00:00 2001
From: Omer Katz <omerkatz@chromium.org>
Date: Wed, 17 Feb 2021 20:27:12 +0100
Subject: [PATCH] cppgc: Fix IsMarking checks.

IsMarking returns true as long as a marker exists. That means IsMarking
is true during weak processing as well.
ActiveScriptWrappableManager in blink uses a weak callback that updates
a HeapVector and thus can trigger a write barrier during the atomic
pause (which violates a DCHECK in the barrier).

Bug: chromium:1056170
Change-Id: I6304b38da9751320836a5e2407e8c7d529367bad
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2700676
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72831}
---
 src/heap/cppgc/heap-state.cc    |  3 ++-
 src/heap/cppgc/marker.cc        |  8 ++++----
 src/heap/cppgc/marker.h         |  4 +++-
 src/heap/cppgc/write-barrier.cc | 10 ++++++----
 4 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/src/heap/cppgc/heap-state.cc b/src/heap/cppgc/heap-state.cc
index d857cf2ebe..32084697c1 100644
--- a/src/heap/cppgc/heap-state.cc
+++ b/src/heap/cppgc/heap-state.cc
@@ -12,7 +12,8 @@ namespace subtle {
 // static
 bool HeapState::IsMarking(const HeapHandle& heap_handle) {
   const auto& heap_base = internal::HeapBase::From(heap_handle);
-  return heap_base.marker();
+  const internal::MarkerBase* marker = heap_base.marker();
+  return marker && marker->IsMarking();
 }
 
 // static
diff --git a/src/heap/cppgc/marker.cc b/src/heap/cppgc/marker.cc
index e3baed59e5..71e0a6bd7a 100644
--- a/src/heap/cppgc/marker.cc
+++ b/src/heap/cppgc/marker.cc
@@ -202,7 +202,7 @@ MarkerBase::~MarkerBase() {
 }
 
 void MarkerBase::StartMarking() {
-  DCHECK(!is_marking_started_);
+  DCHECK(!is_marking_);
   StatsCollector::EnabledScope stats_scope(
       heap().stats_collector(),
       config_.marking_type == MarkingConfig::MarkingType::kAtomic
@@ -212,7 +212,7 @@ void MarkerBase::StartMarking() {
   heap().stats_collector()->NotifyMarkingStarted(config_.collection_type,
                                                  config_.is_forced_gc);
 
-  is_marking_started_ = true;
+  is_marking_ = true;
   if (EnterIncrementalMarkingIfNeeded(config_, heap())) {
     StatsCollector::EnabledScope stats_scope(
         heap().stats_collector(), StatsCollector::kMarkIncrementalStart);
@@ -268,7 +268,7 @@ void MarkerBase::LeaveAtomicPause() {
   heap().stats_collector()->NotifyMarkingCompleted(
       // GetOverallMarkedBytes also includes concurrently marked bytes.
       schedule_.GetOverallMarkedBytes());
-  is_marking_started_ = false;
+  is_marking_ = false;
   {
     // Weakness callbacks are forbidden from allocating objects.
     cppgc::subtle::DisallowGarbageCollectionScope disallow_gc_scope(heap_);
@@ -278,7 +278,7 @@ void MarkerBase::LeaveAtomicPause() {
 }
 
 void MarkerBase::FinishMarking(MarkingConfig::StackState stack_state) {
-  DCHECK(is_marking_started_);
+  DCHECK(is_marking_);
   StatsCollector::EnabledScope stats_scope(heap().stats_collector(),
                                            StatsCollector::kAtomicMark);
   EnterAtomicPause(stack_state);
diff --git a/src/heap/cppgc/marker.h b/src/heap/cppgc/marker.h
index 8518d66b85..50288bd0cb 100644
--- a/src/heap/cppgc/marker.h
+++ b/src/heap/cppgc/marker.h
@@ -131,6 +131,8 @@ class V8_EXPORT_PRIVATE MarkerBase {
 
   void NotifyCompactionCancelled();
 
+  bool IsMarking() const { return is_marking_; }
+
  protected:
   static constexpr v8::base::TimeDelta kMaximumIncrementalStepDuration =
       v8::base::TimeDelta::FromMilliseconds(2);
@@ -170,7 +172,7 @@ class V8_EXPORT_PRIVATE MarkerBase {
 
   MarkingWorklists marking_worklists_;
   MutatorMarkingState mutator_marking_state_;
-  bool is_marking_started_{false};
+  bool is_marking_{false};
 
   IncrementalMarkingSchedule schedule_;
 
diff --git a/src/heap/cppgc/write-barrier.cc b/src/heap/cppgc/write-barrier.cc
index 5d00f19d28..009f9f96f4 100644
--- a/src/heap/cppgc/write-barrier.cc
+++ b/src/heap/cppgc/write-barrier.cc
@@ -149,13 +149,15 @@ bool WriteBarrierTypeForNonCagedHeapPolicy::IsMarking(const void* object,
   // a pointer on the same page.
   const auto* page = BasePage::FromPayload(object);
   *handle = page->heap();
-  return page->heap()->marker();
+  const MarkerBase* marker = page->heap()->marker();
+  return marker && marker->IsMarking();
 }
 
 // static
 bool WriteBarrierTypeForNonCagedHeapPolicy::IsMarking(HeapHandle& heap_handle) {
   const auto& heap_base = internal::HeapBase::From(heap_handle);
-  return heap_base.marker();
+  const MarkerBase* marker = heap_base.marker();
+  return marker && marker->IsMarking();
 }
 
 #if defined(CPPGC_CAGED_HEAP)
@@ -164,8 +166,8 @@ bool WriteBarrierTypeForNonCagedHeapPolicy::IsMarking(HeapHandle& heap_handle) {
 bool WriteBarrierTypeForCagedHeapPolicy::IsMarking(
     const HeapHandle& heap_handle, WriteBarrier::Params& params) {
   const auto& heap_base = internal::HeapBase::From(heap_handle);
-  if (heap_base.marker()) {
-    return true;
+  if (const MarkerBase* marker = heap_base.marker()) {
+    return marker->IsMarking();
   }
   // Also set caged heap start here to avoid another call immediately after
   // checking IsMarking().