cppgc: Adjust explicit management calls

- Take HeapHandle& parameter to allow a use case of free() on an already
  dead object during sweeping.
- Change free() from T* to T& which forces an object and allows the
  caller to place the nullptr check before retrieving a heap handle.

Bug: chromium:1056170
Change-Id: I80689d27d3abe410d177cd8c86b31ff2fe579a77
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2874461
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74387}

include/cppgc/explicit-management.h

@@ -12,9 +12,12 @@
 #include "cppgc/type-traits.h"
 
 namespace cppgc {
+
+class HeapHandle;
+
 namespace internal {
 
-V8_EXPORT void FreeUnreferencedObject(void*);
+V8_EXPORT void FreeUnreferencedObject(HeapHandle&, void*);
 V8_EXPORT bool Resize(void*, size_t);
 
 }  // namespace internal
@@ -30,15 +33,19 @@ namespace subtle {
  * to `object` after calling `FreeUnreferencedObject()`. In case such a
  * reference exists, it's use results in a use-after-free.
  *
+ * To aid in using the API, `FreeUnreferencedObject()` may be called from
+ * destructors on objects that would be reclaimed in the same garbage collection
+ * cycle.
+ *
+ * \param heap_handle The corresponding heap.
  * \param object Reference to an object that is of type `GarbageCollected` and
  *   should be immediately reclaimed.
  */
 template <typename T>
-void FreeUnreferencedObject(T* object) {
+void FreeUnreferencedObject(HeapHandle& heap_handle, T& object) {
   static_assert(IsGarbageCollectedTypeV<T>,
                 "Object must be of type GarbageCollected.");
-  if (!object) return;
-  internal::FreeUnreferencedObject(object);
+  internal::FreeUnreferencedObject(heap_handle, &object);
 }
 
 /**
@@ -53,6 +60,8 @@ void FreeUnreferencedObject(T* object) {
  * object down, the reclaimed area is not used anymore. Any subsequent use
  * results in a use-after-free.
  *
+ * The `object` must be live when calling `Resize()`.
+ *
  * \param object Reference to an object that is of type `GarbageCollected` and
  *   should be resized.
  * \param additional_bytes Bytes in addition to sizeof(T) that the object should

src/heap/cppgc/explicit-management.cc

@@ -16,31 +16,27 @@ namespace internal {
 
 namespace {
 
-std::pair<bool, BasePage*> CanModifyObject(void* object) {
-  // object is guaranteed to be of type GarbageCollected, so getting the
-  // BasePage is okay for regular and large objects.
-  auto* base_page = BasePage::FromPayload(object);
-  auto* heap = base_page->heap();
+bool InGC(HeapHandle& heap_handle) {
+  const auto& heap = HeapBase::From(heap_handle);
   // Whenever the GC is active, avoid modifying the object as it may mess with
   // state that the GC needs.
-  const bool in_gc = heap->in_atomic_pause() || heap->marker() ||
-                     heap->sweeper().IsSweepingInProgress();
-  return {!in_gc, base_page};
+  return heap.in_atomic_pause() || heap.marker() ||
+         heap.sweeper().IsSweepingInProgress();
 }
 
 }  // namespace
 
-void FreeUnreferencedObject(void* object) {
-  bool can_free;
-  BasePage* base_page;
-  std::tie(can_free, base_page) = CanModifyObject(object);
-  if (!can_free) {
+void FreeUnreferencedObject(HeapHandle& heap_handle, void* object) {
+  if (InGC(heap_handle)) {
     return;
   }
 
   auto& header = HeapObjectHeader::FromPayload(object);
   header.Finalize();
 
+  // `object` is guaranteed to be of type GarbageCollected, so getting the
+  // BasePage is okay for regular and large objects.
+  BasePage* base_page = BasePage::FromPayload(object);
   if (base_page->is_large()) {  // Large object.
     base_page->space()->RemovePage(base_page);
     base_page->heap()->stats_collector()->NotifyExplicitFree(
@@ -121,10 +117,11 @@ bool Shrink(HeapObjectHeader& header, BasePage& base_page, size_t new_size,
 }  // namespace
 
 bool Resize(void* object, size_t new_object_size) {
-  bool can_resize;
-  BasePage* base_page;
-  std::tie(can_resize, base_page) = CanModifyObject(object);
-  if (!can_resize) {
+  // `object` is guaranteed to be of type GarbageCollected, so getting the
+  // BasePage is okay for regular and large objects.
+  BasePage* base_page = BasePage::FromPayload(object);
+
+  if (InGC(*base_page->heap())) {
     return false;
   }
 

test/unittests/heap/cppgc/explicit-management-unittest.cc

@@ -58,7 +58,7 @@ TEST_F(ExplicitManagementTest, FreeRegularObjectToLAB) {
   ASSERT_EQ(lab.start(), header.PayloadEnd());
   const size_t lab_size_before_free = lab.size();
   const size_t allocated_size_before = AllocatedObjectSize();
-  subtle::FreeUnreferencedObject(o);
+  subtle::FreeUnreferencedObject(GetHeapHandle(), *o);
   EXPECT_EQ(lab.start(), reinterpret_cast<Address>(needle));
   EXPECT_EQ(lab_size_before_free + size, lab.size());
   // LAB is included in allocated object size, so no change is expected.
@@ -78,7 +78,7 @@ TEST_F(ExplicitManagementTest, FreeRegularObjectToFreeList) {
   ResetLinearAllocationBuffers();
   ASSERT_EQ(lab.start(), nullptr);
   const size_t allocated_size_before = AllocatedObjectSize();
-  subtle::FreeUnreferencedObject(o);
+  subtle::FreeUnreferencedObject(GetHeapHandle(), *o);
   EXPECT_EQ(lab.start(), nullptr);
   EXPECT_EQ(allocated_size_before - size, AllocatedObjectSize());
   EXPECT_TRUE(space->free_list().ContainsForTesting({needle, size}));
@@ -95,7 +95,7 @@ TEST_F(ExplicitManagementTest, FreeLargeObject) {
   const size_t size = LargePage::From(page)->PayloadSize();
   EXPECT_TRUE(heap->page_backend()->Lookup(needle));
   const size_t allocated_size_before = AllocatedObjectSize();
-  subtle::FreeUnreferencedObject(o);
+  subtle::FreeUnreferencedObject(GetHeapHandle(), *o);
   EXPECT_FALSE(heap->page_backend()->Lookup(needle));
   EXPECT_EQ(allocated_size_before - size, AllocatedObjectSize());
 }
@@ -107,20 +107,14 @@ TEST_F(ExplicitManagementTest, FreeBailsOutDuringGC) {
   auto* heap = BasePage::FromPayload(o)->heap();
   heap->SetInAtomicPauseForTesting(true);
   const size_t allocated_size_before = AllocatedObjectSize();
-  subtle::FreeUnreferencedObject(o);
+  subtle::FreeUnreferencedObject(GetHeapHandle(), *o);
   EXPECT_EQ(allocated_size_before, AllocatedObjectSize());
   heap->SetInAtomicPauseForTesting(false);
   ResetLinearAllocationBuffers();
-  subtle::FreeUnreferencedObject(o);
+  subtle::FreeUnreferencedObject(GetHeapHandle(), *o);
   EXPECT_EQ(snapshot_before, AllocatedObjectSize());
 }
 
-TEST_F(ExplicitManagementTest, FreeNull) {
-  DynamicallySized* o = nullptr;
-  // Noop.
-  subtle::FreeUnreferencedObject(o);
-}
-
 TEST_F(ExplicitManagementTest, GrowAtLAB) {
   auto* o =
       MakeGarbageCollected<DynamicallySized>(GetHeap()->GetAllocationHandle());

test/unittests/heap/cppgc/tests.h

@@ -88,6 +88,10 @@ class TestWithHeap : public TestWithPlatform {
     return allocation_handle_;
   }
 
+  cppgc::HeapHandle& GetHeapHandle() const {
+    return GetHeap()->GetHeapHandle();
+  }
+
   std::unique_ptr<MarkerBase>& GetMarkerRef() {
     return Heap::From(GetHeap())->marker_;
   }