cppgc: Fix crash when finalizing incremental GC.

The gc_in_progress flag was reset to false only after sweeping was done.
As a result, if we call CollectGarbage during an incremental GC and
after marking has finished, the we will observe that a gc is still in
progress but will not have a marker and crash.

The immediate solution is to move resetting the gc_in_progress flag such
that it indicates whether we didn't have the atomic pause yet. That
means we could have gc_in_progress==false and incremental sweeping still
running, which semantically negates the meaning of gc_in_progress.

Observing that gc_in_progress essentially becomes equivalent to having a
marker, this CL removes the gc_in_progress flag and replaces checks on
it with checks on marker.

Bug: chromium:1156170
Change-Id: Ic4b441ec248b5f7e222e988870e46d5166dd4dcc
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2584875
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Auto-Submit: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71712}

src/heap/cppgc/heap.cc

@@ -105,6 +105,8 @@ Heap::~Heap() {
   sweeper_.FinishIfRunning();
 }
 
+bool Heap::IsMarking() const { return marker_.get(); }
+
 void Heap::CollectGarbage(Config config) {
   DCHECK_EQ(Config::MarkingType::kAtomic, config.marking_type);
   CheckConfig(config, marking_support_, sweeping_support_);
@@ -113,9 +115,9 @@ void Heap::CollectGarbage(Config config) {
 
   config_ = config;
 
-  if (!gc_in_progress_) StartGarbageCollection(config);
+  if (!IsMarking()) StartGarbageCollection(config);
 
-  DCHECK(marker_);
+  DCHECK(IsMarking());
 
   FinalizeGarbageCollection(config.stack_state);
 }
@@ -125,7 +127,7 @@ void Heap::StartIncrementalGarbageCollection(Config config) {
   DCHECK_NE(marking_support_, MarkingType::kAtomic);
   CheckConfig(config, marking_support_, sweeping_support_);
 
-  if (gc_in_progress_ || in_no_gc_scope()) return;
+  if (IsMarking() || in_no_gc_scope()) return;
 
   config_ = config;
 
@@ -136,7 +138,7 @@ void Heap::FinalizeIncrementalGarbageCollectionIfRunning(Config config) {
   DCHECK_NE(marking_support_, MarkingType::kAtomic);
   CheckConfig(config, marking_support_, sweeping_support_);
 
-  if (!gc_in_progress_) return;
+  if (!IsMarking()) return;
 
   DCHECK(!in_no_gc_scope());
 
@@ -146,14 +148,13 @@ void Heap::FinalizeIncrementalGarbageCollectionIfRunning(Config config) {
 }
 
 void Heap::StartGarbageCollection(Config config) {
-  DCHECK(!gc_in_progress_);
+  DCHECK(!IsMarking());
 
   DCHECK(!in_no_gc_scope());
 
   // Finish sweeping in case it is still running.
   sweeper_.FinishIfRunning();
 
-  gc_in_progress_ = true;
   epoch_++;
 
 #if defined(CPPGC_YOUNG_GENERATION)
@@ -169,10 +170,9 @@ void Heap::StartGarbageCollection(Config config) {
 }
 
 void Heap::FinalizeGarbageCollection(Config::StackState stack_state) {
-  DCHECK(gc_in_progress_);
+  DCHECK(IsMarking());
   DCHECK(!in_no_gc_scope());
   config_.stack_state = stack_state;
-  DCHECK(marker_);
   {
     // This guards atomic pause marking, meaning that no internal method or
     // external callbacks are allowed to allocate new objects.
@@ -199,7 +199,7 @@ void Heap::FinalizeGarbageCollection(Config::StackState stack_state) {
   sweeper_.NotifyDoneIfNeeded();
 }
 
-void Heap::PostGarbageCollection() { gc_in_progress_ = false; }
+void Heap::PostGarbageCollection() {}
 
 void Heap::DisableHeapGrowingForTesting() { growing_.DisableForTesting(); }
 

src/heap/cppgc/heap.h

@@ -48,6 +48,8 @@ class V8_EXPORT_PRIVATE Heap final : public HeapBase,
 
   void PostGarbageCollection() final;
 
+  bool IsMarking() const;
+
   Config config_;
   GCInvoker gc_invoker_;
   HeapGrowing growing_;
@@ -55,7 +57,6 @@ class V8_EXPORT_PRIVATE Heap final : public HeapBase,
   const MarkingType marking_support_;
   const SweepingType sweeping_support_;
 
-  bool gc_in_progress_ = false;
   size_t epoch_ = 0;
 };