Makes sure FreeListCategories never contain empty elements

The linked lists of FreeLists could contain empty elements
(FreeListCategories whose `top()` is null). The code is carefuly
written so that this case won't break anything (probably just slow
things a little bit).  When `FreeList::FindNodeIn` (the fast path of
`FreeList::Allocate`) found such an empty `FreeListCategory`, it
removed it by calling `FreeList::RemoveCategory`, and looked in the
next `FreeListCategory` of the same size. However, on the slow path of
`FreeList::Allocate`, the functions that iterates the
`FreeListCategory` are `FreeList::TryFindNodeIn` and
`FreeListCategory::PickNodeFromList`, none of which removed empty
elements. Therefore, it could happen that a `FreeListCategory` "real"
first element could be used, but was never considered due to the top of
the linked list being empty. The behavior for the slow path should be
the same as for the fast path on that regard.

The problem was actually deeper than that: FreeListCategories were not
always in a consistent state, since they could have empty members. The
removal of those empty elements should be done as soon as they are
created, ie when allocating the last element.

This CL ensures that empty FreeListCategories are removed as soon as
they become empty.

Bug: v8:9329
Change-Id: Idda8096dc5978745894854a0405da59f7e8691a6
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1648476
Commit-Queue: Darius Mercadier <dmercadier@google.com>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62058}

src/heap/spaces.cc

@@ -2934,7 +2934,8 @@ FreeSpace FreeListCategory::PickNodeFromList(size_t minimum_size,
                                              size_t* node_size) {
   DCHECK(page()->CanAllocate());
   FreeSpace node = top();
-  if (node.is_null() || static_cast<size_t>(node.Size()) < minimum_size) {
+  DCHECK(!node.is_null());
+  if (static_cast<size_t>(node.Size()) < minimum_size) {
     *node_size = 0;
     return FreeSpace();
   }
@@ -3053,20 +3054,26 @@ FreeSpace FreeList::FindNodeIn(FreeListCategoryType type, size_t minimum_size,
     node = current->PickNodeFromList(minimum_size, node_size);
     if (!node.is_null()) {
       DCHECK(IsVeryLong() || Available() == SumFreeLists());
+      if (current->is_empty()) {
+        RemoveCategory(current);
+      }
       return node;
     }
-    RemoveCategory(current);
   }
   return node;
 }
 
 FreeSpace FreeList::TryFindNodeIn(FreeListCategoryType type,
                                   size_t minimum_size, size_t* node_size) {
-  if (categories_[type] == nullptr) return FreeSpace();
-  FreeSpace node = categories_[type]->PickNodeFromList(minimum_size, node_size);
+  FreeListCategory* category = categories_[type];
+  if (category == nullptr) return FreeSpace();
+  FreeSpace node = category->PickNodeFromList(minimum_size, node_size);
   if (!node.is_null()) {
     DCHECK(IsVeryLong() || Available() == SumFreeLists());
   }
+  if (category->is_empty()) {
+    RemoveCategory(category);
+  }
   return node;
 }
 
@@ -3080,11 +3087,11 @@ FreeSpace FreeList::SearchForNodeInList(FreeListCategoryType type,
     node = current->SearchForNodeInList(minimum_size, node_size);
     if (!node.is_null()) {
       DCHECK(IsVeryLong() || Available() == SumFreeLists());
+      if (current->is_empty()) {
+        RemoveCategory(current);
+      }
       return node;
     }
-    if (current->is_empty()) {
-      RemoveCategory(current);
-    }
   }
   return node;
 }

test/cctest/heap/heap-tester.h

@@ -53,7 +53,8 @@
   V(Regress845060)                                        \
   V(RegressMissingWriteBarrierInAllocate)                 \
   V(WriteBarriersInCopyJSObject)                          \
-  V(AllocateObjTinyFreeList)
+  V(AllocateObjTinyFreeList)                              \
+  V(EmptyFreeListCategoriesRemoved)
 
 #define HEAP_TEST(Name)                                                   \
   CcTest register_test_##Name(v8::internal::heap::HeapTester::Test##Name, \

test/cctest/heap/test-spaces.cc

@@ -794,6 +794,49 @@ HEAP_TEST(AllocateObjTinyFreeList) {
   CHECK_EQ(tiny_obj_page, tiniest_obj_page);
 }
 
+HEAP_TEST(EmptyFreeListCategoriesRemoved) {
+  CcTest::InitializeVM();
+  Isolate* isolate = CcTest::i_isolate();
+  HandleScope scope(isolate);
+
+  heap::SealCurrentObjects(CcTest::heap());
+
+  // The maximum size for a Tiny FixedArray.
+  // (there is no specific reason for using Tiny rather than any other category)
+  constexpr size_t tiny_obj_size =
+      (FreeList::kTinyListMax - FixedArray::kHeaderSize) / kTaggedSize;
+
+  Page* tiny_obj_page;
+  {
+    // Allocate a Tiny object that will be destroyed later.
+    HandleScope tiny_scope(isolate);
+    Handle<FixedArray> tiny_obj = isolate->factory()->NewFixedArray(
+        static_cast<int>(tiny_obj_size), AllocationType::kOld);
+    tiny_obj_page = Page::FromHeapObject(*tiny_obj);
+  }
+
+  // Fill up the page entirely.
+  PagedSpace* old_space = CcTest::heap()->old_space();
+  int space_remaining =
+      static_cast<int>(*old_space->allocation_limit_address() -
+                       *old_space->allocation_top_address());
+  std::vector<Handle<FixedArray>> handles = heap::CreatePadding(
+      old_space->heap(), space_remaining, AllocationType::kOld);
+
+  // Call gc to reclaim |tiny_obj| (since its HandleScope went out of scope).
+  CcTest::CollectAllGarbage();
+  isolate->heap()->mark_compact_collector()->EnsureSweepingCompleted();
+  isolate->heap()->old_space()->FreeLinearAllocationArea();
+
+  // Allocates a new tiny_obj, which should take the place of the old one.
+  Handle<FixedArray> tiny_obj = isolate->factory()->NewFixedArray(
+      static_cast<int>(tiny_obj_size), AllocationType::kOld);
+  CHECK_EQ(tiny_obj_page, Page::FromHeapObject(*tiny_obj));
+
+  // The Tiny FreeListCategory should now be empty
+  CHECK_NULL(isolate->heap()->old_space()->free_list()->categories_[kTiny]);
+}
+
 }  // namespace heap
 }  // namespace internal
 }  // namespace v8