heap: ManualGCScope should finalize marking

Since ManualGCScope changes marking flags it should finalize any
ongoing GC before changing the flags. Otherwise, the GC may observe
inconsistent state.

Bug: chromium:1285706
Change-Id: Ie8ef6a1117ba0523d0bed0c46d9116ffbc02069c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3386607
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Auto-Submit: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78618}

src/heap/marking-visitor.h

@@ -127,9 +127,8 @@ template <typename ConcreteVisitor, typename MarkingState>
 class MarkingVisitorBase : public HeapVisitor<int, ConcreteVisitor> {
  public:
   MarkingVisitorBase(MarkingWorklists::Local* local_marking_worklists,
-                     WeakObjects::Local* local_weak_objects,
-                     //  WeakObjects* weak_objects,
-                     Heap* heap, unsigned mark_compact_epoch,
+                     WeakObjects::Local* local_weak_objects, Heap* heap,
+                     unsigned mark_compact_epoch,
                      base::EnumSet<CodeFlushMode> code_flush_mode,
                      bool is_embedder_tracing_enabled,
                      bool should_keep_ages_unchanged)

test/cctest/cctest.cc

@@ -38,6 +38,7 @@
 #include "src/base/strings.h"
 #include "src/codegen/compiler.h"
 #include "src/codegen/optimized-compilation-info.h"
+#include "src/common/globals.h"
 #include "src/compiler/pipeline.h"
 #include "src/debug/debug.h"
 #include "src/flags/flags.h"
@@ -423,3 +424,37 @@ bool IsValidUnwrapObject(v8::Object* object) {
           instance_type == i::Internals::kJSObjectType ||
           instance_type == i::Internals::kJSSpecialApiObjectType);
 }
+
+ManualGCScope::ManualGCScope(i::Isolate* isolate)
+    : flag_concurrent_marking_(i::FLAG_concurrent_marking),
+      flag_concurrent_sweeping_(i::FLAG_concurrent_sweeping),
+      flag_stress_concurrent_allocation_(i::FLAG_stress_concurrent_allocation),
+      flag_stress_incremental_marking_(i::FLAG_stress_incremental_marking),
+      flag_parallel_marking_(i::FLAG_parallel_marking),
+      flag_detect_ineffective_gcs_near_heap_limit_(
+          i::FLAG_detect_ineffective_gcs_near_heap_limit) {
+  // Some tests run threaded (back-to-back) and thus the GC may already be
+  // running by the time a ManualGCScope is created. Finalizing existing marking
+  // prevents any undefined/unexpected behavior.
+  if (isolate && isolate->heap()->incremental_marking()->IsMarking()) {
+    CcTest::CollectGarbage(i::OLD_SPACE, isolate);
+  }
+
+  i::FLAG_concurrent_marking = false;
+  i::FLAG_concurrent_sweeping = false;
+  i::FLAG_stress_incremental_marking = false;
+  i::FLAG_stress_concurrent_allocation = false;
+  // Parallel marking has a dependency on concurrent marking.
+  i::FLAG_parallel_marking = false;
+  i::FLAG_detect_ineffective_gcs_near_heap_limit = false;
+}
+
+ManualGCScope::~ManualGCScope() {
+  i::FLAG_concurrent_marking = flag_concurrent_marking_;
+  i::FLAG_concurrent_sweeping = flag_concurrent_sweeping_;
+  i::FLAG_stress_concurrent_allocation = flag_stress_concurrent_allocation_;
+  i::FLAG_stress_incremental_marking = flag_stress_incremental_marking_;
+  i::FLAG_parallel_marking = flag_parallel_marking_;
+  i::FLAG_detect_ineffective_gcs_near_heap_limit =
+      flag_detect_ineffective_gcs_near_heap_limit_;
+}

test/cctest/cctest.h

@@ -199,18 +199,21 @@ class CcTest {
   static void TearDown();
 
  private:
-  friend int main(int argc, char** argv);
+  static CcTest* last_;
+  static v8::ArrayBuffer::Allocator* allocator_;
+  static v8::Isolate* isolate_;
+  static bool initialize_called_;
+  static v8::base::Atomic32 isolate_used_;
+
   TestFunction* callback_;
   const char* file_;
   const char* name_;
   bool enabled_;
   bool initialize_;
   CcTest* prev_;
-  static CcTest* last_;
-  static v8::ArrayBuffer::Allocator* allocator_;
-  static v8::Isolate* isolate_;
-  static bool initialize_called_;
-  static v8::base::Atomic32 isolate_used_;
+
+  friend int main(int argc, char** argv);
+  friend class ManualGCScope;
 };
 
 // Switches between all the Api tests using the threading support.
@@ -676,42 +679,25 @@ class StaticOneByteResource : public v8::String::ExternalOneByteStringResource {
   const char* data_;
 };
 
+// ManualGCScope allows for disabling GC heuristics. This is useful for tests
+// that want to check specific corner cases around GC.
+//
+// The scope will finalize any ongoing GC on the provided Isolate. If no Isolate
+// is manually provided, it is assumed that a CcTest setup (e.g.
+// CcTest::InitializeVM()) is used.
 class V8_NODISCARD ManualGCScope {
  public:
-  ManualGCScope()
-      : flag_concurrent_marking_(i::FLAG_concurrent_marking),
-        flag_concurrent_sweeping_(i::FLAG_concurrent_sweeping),
-        flag_stress_concurrent_allocation_(
-            i::FLAG_stress_concurrent_allocation),
-        flag_stress_incremental_marking_(i::FLAG_stress_incremental_marking),
-        flag_parallel_marking_(i::FLAG_parallel_marking),
-        flag_detect_ineffective_gcs_near_heap_limit_(
-            i::FLAG_detect_ineffective_gcs_near_heap_limit) {
-    i::FLAG_concurrent_marking = false;
-    i::FLAG_concurrent_sweeping = false;
-    i::FLAG_stress_incremental_marking = false;
-    i::FLAG_stress_concurrent_allocation = false;
-    // Parallel marking has a dependency on concurrent marking.
-    i::FLAG_parallel_marking = false;
-    i::FLAG_detect_ineffective_gcs_near_heap_limit = false;
-  }
-  ~ManualGCScope() {
-    i::FLAG_concurrent_marking = flag_concurrent_marking_;
-    i::FLAG_concurrent_sweeping = flag_concurrent_sweeping_;
-    i::FLAG_stress_concurrent_allocation = flag_stress_concurrent_allocation_;
-    i::FLAG_stress_incremental_marking = flag_stress_incremental_marking_;
-    i::FLAG_parallel_marking = flag_parallel_marking_;
-    i::FLAG_detect_ineffective_gcs_near_heap_limit =
-        flag_detect_ineffective_gcs_near_heap_limit_;
-  }
+  explicit ManualGCScope(
+      i::Isolate* isolate = reinterpret_cast<i::Isolate*>(CcTest::isolate_));
+  ~ManualGCScope();
 
  private:
-  bool flag_concurrent_marking_;
-  bool flag_concurrent_sweeping_;
-  bool flag_stress_concurrent_allocation_;
-  bool flag_stress_incremental_marking_;
-  bool flag_parallel_marking_;
-  bool flag_detect_ineffective_gcs_near_heap_limit_;
+  const bool flag_concurrent_marking_;
+  const bool flag_concurrent_sweeping_;
+  const bool flag_stress_concurrent_allocation_;
+  const bool flag_stress_incremental_marking_;
+  const bool flag_parallel_marking_;
+  const bool flag_detect_ineffective_gcs_near_heap_limit_;
 };
 
 // This is an abstract base class that can be overridden to implement a test