cppgc: Fix flaky crash due to used bytes counters mismatch

Cppgc reports used bytes statistics to CppHeap. CppHeap should forward
the stats to v8. However, if we are not allowed to trigger a GC, CppHeap
will cache the stats until the reporting.
On GC finalization, CppHeap resets v8's counters to the current marked
bytes counter.
If the last reported stats before GC finalization are cached, CppHeap
doesn't clear the cache on GC finalization. On the next stats reporting,
CppHeap will report the cached values. If the cache is a decrease that
is larger than the current marked bytes, a DCHECK in
LocalEmbedderHeapTracer::DecreaseAllocatedSize will fail.

Bug: chromium:1056170
Change-Id: I47933abc3e5f5c4a91454e0ec03adde5cf61d8fc
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3056970
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Anton Bikineev <bikineev@chromium.org>
Cr-Commit-Position: refs/heads/master@{#75986}

src/heap/cppgc-js/cpp-heap.cc

@@ -474,13 +474,6 @@ void CppHeap::TraceEpilogue(TraceSummary* trace_summary) {
     marker_->LeaveAtomicPause();
   }
   marker_.reset();
-  if (isolate_) {
-    auto* tracer = isolate_->heap()->local_embedder_heap_tracer();
-    DCHECK_NOT_NULL(tracer);
-    tracer->UpdateRemoteStats(
-        stats_collector_->marked_bytes(),
-        stats_collector_->marking_time().InMillisecondsF());
-  }
   ExecutePreFinalizers();
   // TODO(chromium:1056170): replace build flag with dedicated flag.
 #if DEBUG
@@ -528,6 +521,20 @@ void CppHeap::AllocatedObjectSizeDecreased(size_t bytes) {
   ReportBufferedAllocationSizeIfPossible();
 }
 
+void CppHeap::ResetAllocatedObjectSize(size_t bytes) {
+  DCHECK(!sweeper().IsSweepingOnMutatorThread());
+  DCHECK(!in_no_gc_scope());
+  buffered_allocated_bytes_ = 0;
+  if (isolate_) {
+    auto* tracer = isolate_->heap()->local_embedder_heap_tracer();
+    DCHECK_NOT_NULL(tracer);
+    DCHECK_EQ(bytes, stats_collector_->marked_bytes());
+    tracer->UpdateRemoteStats(
+        stats_collector_->marked_bytes(),
+        stats_collector_->marking_time().InMillisecondsF());
+  }
+}
+
 void CppHeap::ReportBufferedAllocationSizeIfPossible() {
   // Avoid reporting to V8 in the following conditions as that may trigger GC
   // finalizations where not allowed.

src/heap/cppgc-js/cpp-heap.h

@@ -114,7 +114,7 @@ class V8_EXPORT_PRIVATE CppHeap final
   // StatsCollector::AllocationObserver interface.
   void AllocatedObjectSizeIncreased(size_t) final;
   void AllocatedObjectSizeDecreased(size_t) final;
-  void ResetAllocatedObjectSize(size_t) final {}
+  void ResetAllocatedObjectSize(size_t) final;
 
   MetricRecorderAdapter* GetMetricRecorder() const;
 

test/unittests/heap/unified-heap-unittest.cc

@@ -5,7 +5,10 @@
 #include <memory>
 
 #include "include/cppgc/allocation.h"
+#include "include/cppgc/explicit-management.h"
 #include "include/cppgc/garbage-collected.h"
+#include "include/cppgc/heap-consistency.h"
+#include "include/cppgc/internal/api-constants.h"
 #include "include/cppgc/persistent.h"
 #include "include/cppgc/platform.h"
 #include "include/cppgc/testing.h"
@@ -132,6 +135,43 @@ TEST_F(UnifiedHeapTest, WriteBarrierCppToV8Reference) {
             wrappable->wrapper()->GetAlignedPointerFromInternalField(1));
 }
 
+#if DEBUG
+namespace {
+class Unreferenced : public cppgc::GarbageCollected<Unreferenced> {
+ public:
+  void Trace(cppgc::Visitor*) const {}
+};
+}  // namespace
+
+TEST_F(UnifiedHeapTest, FreeUnreferencedDuringNoGcScope) {
+  v8::HandleScope scope(v8_isolate());
+  v8::Local<v8::Context> context = v8::Context::New(v8_isolate());
+  v8::Context::Scope context_scope(context);
+  auto* unreferenced = cppgc::MakeGarbageCollected<Unreferenced>(
+      allocation_handle(),
+      cppgc::AdditionalBytes(cppgc::internal::api_constants::kMB));
+  // Force safepoint to force flushing of cached allocated/freed sizes in cppgc.
+  cpp_heap().stats_collector()->NotifySafePointForTesting();
+  {
+    cppgc::subtle::NoGarbageCollectionScope scope(cpp_heap());
+    cppgc::internal::FreeUnreferencedObject(cpp_heap(), unreferenced);
+    // Force safepoint to make sure allocated size decrease due to freeing
+    // unreferenced object is reported to CppHeap. Due to
+    // NoGarbageCollectionScope, CppHeap will cache the reported decrease and
+    // won't report it further.
+    cpp_heap().stats_collector()->NotifySafePointForTesting();
+  }
+  // Running a GC resets the allocated size counters to the current marked bytes
+  // counter.
+  CollectGarbageWithoutEmbedderStack(cppgc::Heap::SweepingType::kAtomic);
+  // If CppHeap didn't clear it's cached values when the counters were reset,
+  // the next safepoint will try to decrease the cached value from the last
+  // marked bytes (which is smaller than the cached value) and crash.
+  cppgc::MakeGarbageCollected<Unreferenced>(allocation_handle());
+  cpp_heap().stats_collector()->NotifySafePointForTesting();
+}
+#endif  // DEBUG
+
 #if !V8_OS_FUCHSIA
 TEST_F(UnifiedHeapTest, TracedReferenceRetainsFromStack) {
   v8::HandleScope scope(v8_isolate());