[heap] Fix off-thread merging tests

Fix off-thread space merging unit tests, by

  a) Doing a GC on each test teardown (avoiding leaking GC state between
     tests), and
  b) Removing the AllocationStep when merging, which could cause a
     deadlock if incremental marking triggered a free-list refill.

Bug: v8:10142
Change-Id: Id2670115f96ab3b13613ea357bd44a639d0151e1
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2142260
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Auto-Submit: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67077}

src/heap/spaces.cc

@@ -1762,14 +1762,12 @@ void PagedSpace::MergeLocalSpace(LocalSpace* other) {
         !p->IsFlagSet(Page::NEVER_ALLOCATE_ON_PAGE),
         p->AvailableInFreeList() == p->AvailableInFreeListFromAllocatedBytes());
 
-    if (merging_from_off_thread) {
-      // TODO(leszeks): Allocation groups are currently not handled properly by
-      // the sampling allocation profiler. We'll have to come up with a better
-      // solution for allocation stepping before shipping.
-      AllocationStepAfterMerge(
-          p->area_start(),
-          static_cast<int>(p->HighWaterMark() - p->area_start()));
-    }
+    // TODO(leszeks): Here we should allocation step, but:
+    //   1. Allocation groups are currently not handled properly by the sampling
+    //      allocation profiler, and
+    //   2. Observers might try to take the space lock, which isn't reentrant.
+    // We'll have to come up with a better solution for allocation stepping
+    // before shipping, which will likely be using LocalHeap.
   }
 
   if (merging_from_off_thread) {
@@ -4401,7 +4399,9 @@ void OldLargeObjectSpace::MergeOffThreadSpace(
     other->RemovePage(page, size);
     AddPage(page, size);
 
-    AllocationStepAfterMerge(object.address(), size);
+    // TODO(leszeks): Here we should AllocationStep, see the TODO in
+    // PagedSpace::MergeOffThreadSpace.
+
     if (heap()->incremental_marking()->black_allocation()) {
       heap()->incremental_marking()->marking_state()->WhiteToBlack(object);
     }

test/unittests/heap/spaces-unittest.cc

@@ -15,7 +15,15 @@
 namespace v8 {
 namespace internal {
 
-using SpacesTest = TestWithIsolate;
+class SpacesTest : public TestWithIsolate {
+ protected:
+  void TearDown() final {
+    // Clean up the heap between tests.
+    i_isolate()->heap()->CollectAllAvailableGarbage(
+        GarbageCollectionReason::kTesting);
+    TestWithIsolate::TearDown();
+  }
+};
 
 TEST_F(SpacesTest, CompactionSpaceMerge) {
   Heap* heap = i_isolate()->heap();
@@ -167,10 +175,15 @@ TEST_F(SpacesTest, OffThreadSpaceMergeDuringIncrementalMarking) {
     expected_merged_pages += pages_in_off_thread_space;
   }
 
-  heap->FinalizeIncrementalMarkingAtomically(GarbageCollectionReason::kTesting);
-
+  // Check the page size before finalizing marking, since the GC will see the
+  // empty pages and will evacuate them.
+  // TODO(leszeks): Maybe allocate real objects, and hold on to them with
+  // Handles, to make sure incremental marking finalization doesn't clear them
+  // away.
   EXPECT_EQ(pages_in_old_space + expected_merged_pages,
             old_space->CountTotalPages());
+
+  heap->FinalizeIncrementalMarkingAtomically(GarbageCollectionReason::kTesting);
 }
 
 class LargeOffThreadAllocationThread final : public base::Thread {
@@ -266,7 +279,10 @@ TEST_F(SpacesTest, OffThreadLargeObjectSpaceMergeDuringIncrementalMarking) {
     threads[i]->Join();
   }
 
-  int pages_in_old_space = lo_space->PageCount();
+  heap->StartIncrementalMarking(Heap::kNoGCFlags,
+                                GarbageCollectionReason::kTesting);
+
+  int pages_in_lo_space = lo_space->PageCount();
 
   int expected_merged_pages = 0;
   for (int i = 0; i < kNumThreads; ++i) {
@@ -276,7 +292,14 @@ TEST_F(SpacesTest, OffThreadLargeObjectSpaceMergeDuringIncrementalMarking) {
     expected_merged_pages += pages_in_off_thread_space;
   }
 
-  EXPECT_EQ(pages_in_old_space + expected_merged_pages, lo_space->PageCount());
+  // Check the page size before finalizing marking, since the GC will see the
+  // empty pages and will evacuate them.
+  // TODO(leszeks): Maybe allocate real objects, and hold on to them with
+  // Handles, to make sure incremental marking finalization doesn't clear them
+  // away.
+  EXPECT_EQ(pages_in_lo_space + expected_merged_pages, lo_space->PageCount());
+
+  heap->FinalizeIncrementalMarkingAtomically(GarbageCollectionReason::kTesting);
 }
 
 TEST_F(SpacesTest, WriteBarrierFromHeapObject) {