[heap] Immediately update external memory on JSArrayBuffer::Detach

This CL changes the accounting of array buffers, such that Detach
deducts the backing store immediately. Previously this was corrected
in the next GC cycle.

Not updating backing_store_bytes_ immediately could cause an overflow
in WasmMemoryObject::Grow. Grow first detaches the backing store from
the old JSArrayBuffer and then attaches it to a new one. This results
in the backing store being accounted twice temporarily, this could cause
overflows on 32-bit systems.

Bug: chromium:1204455
Change-Id: I7cf2ca9a12bb5caf7bcffa25a34567774cf155b8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2871458
Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74429}

src/heap/array-buffer-sweeper.cc

@@ -101,30 +101,25 @@ void ArrayBufferSweeper::EnsureFinished() {
       UNREACHABLE();
   }
 
-  DecrementExternalMemoryCounters();
+  UpdateCountersForConcurrentlySweptExtensions();
   sweeping_in_progress_ = false;
 }
 
-void ArrayBufferSweeper::AdjustCountersAndMergeIfPossible() {
+void ArrayBufferSweeper::MergeBackExtensionsWhenSwept() {
   if (sweeping_in_progress_) {
     DCHECK(job_.has_value());
     if (job_->state_ == SweepingState::kDone) {
       Merge();
       sweeping_in_progress_ = false;
     } else {
-      DecrementExternalMemoryCounters();
+      UpdateCountersForConcurrentlySweptExtensions();
     }
   }
 }
 
-void ArrayBufferSweeper::DecrementExternalMemoryCounters() {
+void ArrayBufferSweeper::UpdateCountersForConcurrentlySweptExtensions() {
   size_t freed_bytes = freed_bytes_.exchange(0, std::memory_order_relaxed);
-
-  if (freed_bytes > 0) {
-    heap_->DecrementExternalBackingStoreBytes(
-        ExternalBackingStoreType::kArrayBuffer, freed_bytes);
-    heap_->update_external_memory(-static_cast<int64_t>(freed_bytes));
-  }
+  DecrementExternalMemoryCounters(freed_bytes);
 }
 
 void ArrayBufferSweeper::RequestSweepYoung() {
@@ -166,7 +161,7 @@ void ArrayBufferSweeper::RequestSweep(SweepingScope scope) {
     Prepare(scope);
     job_->Sweep();
     Merge();
-    DecrementExternalMemoryCounters();
+    UpdateCountersForConcurrentlySweptExtensions();
   }
 }
 
@@ -228,18 +223,52 @@ void ArrayBufferSweeper::Append(JSArrayBuffer object,
     old_bytes_ += bytes;
   }
 
-  AdjustCountersAndMergeIfPossible();
-  DecrementExternalMemoryCounters();
+  MergeBackExtensionsWhenSwept();
   IncrementExternalMemoryCounters(bytes);
 }
 
+void ArrayBufferSweeper::Detach(JSArrayBuffer object,
+                                ArrayBufferExtension* extension) {
+  size_t bytes = extension->ClearAccountingLength();
+
+  // We cannot free the extension eagerly here, since extensions are tracked in
+  // a singly linked list. The next GC will remove it automatically.
+
+  if (!sweeping_in_progress_) {
+    // If concurrent sweeping isn't running at the moment, we can also adjust
+    // young_bytes_ or old_bytes_ right away.
+    if (Heap::InYoungGeneration(object)) {
+      DCHECK_GE(young_bytes_, bytes);
+      young_bytes_ -= bytes;
+      young_.bytes_ -= bytes;
+    } else {
+      DCHECK_GE(old_bytes_, bytes);
+      old_bytes_ -= bytes;
+      old_.bytes_ -= bytes;
+    }
+  }
+
+  MergeBackExtensionsWhenSwept();
+  DecrementExternalMemoryCounters(bytes);
+}
+
 void ArrayBufferSweeper::IncrementExternalMemoryCounters(size_t bytes) {
+  if (bytes == 0) return;
   heap_->IncrementExternalBackingStoreBytes(
       ExternalBackingStoreType::kArrayBuffer, bytes);
   reinterpret_cast<v8::Isolate*>(heap_->isolate())
       ->AdjustAmountOfExternalAllocatedMemory(static_cast<int64_t>(bytes));
 }
 
+void ArrayBufferSweeper::DecrementExternalMemoryCounters(size_t bytes) {
+  if (bytes == 0) return;
+  heap_->DecrementExternalBackingStoreBytes(
+      ExternalBackingStoreType::kArrayBuffer, bytes);
+  // Unlike IncrementExternalMemoryCounters we don't use
+  // AdjustAmountOfExternalAllocatedMemory such that we never start a GC here.
+  heap_->update_external_memory(-static_cast<int64_t>(bytes));
+}
+
 void ArrayBufferSweeper::IncrementFreedBytes(size_t bytes) {
   if (bytes == 0) return;
   freed_bytes_.fetch_add(bytes, std::memory_order_relaxed);

src/heap/array-buffer-sweeper.h

@@ -59,8 +59,12 @@ class ArrayBufferSweeper {
   void RequestSweepYoung();
   void RequestSweepFull();
 
+  // Track the given ArrayBufferExtension for the given JSArrayBuffer.
   void Append(JSArrayBuffer object, ArrayBufferExtension* extension);
 
+  // Detaches an ArrayBufferExtension from a JSArrayBuffer.
+  void Detach(JSArrayBuffer object, ArrayBufferExtension* extension);
+
   ArrayBufferList young() { return young_; }
   ArrayBufferList old() { return old_; }
 
@@ -98,10 +102,11 @@ class ArrayBufferSweeper {
   base::Optional<SweepingJob> job_;
 
   void Merge();
-  void AdjustCountersAndMergeIfPossible();
+  void MergeBackExtensionsWhenSwept();
 
-  void DecrementExternalMemoryCounters();
+  void UpdateCountersForConcurrentlySweptExtensions();
   void IncrementExternalMemoryCounters(size_t bytes);
+  void DecrementExternalMemoryCounters(size_t bytes);
   void IncrementFreedBytes(size_t bytes);
 
   void RequestSweep(SweepingScope sweeping_task);

src/heap/heap.cc

@@ -4009,6 +4009,11 @@ void Heap::AppendArrayBufferExtension(JSArrayBuffer object,
   array_buffer_sweeper_->Append(object, extension);
 }
 
+void Heap::DetachArrayBufferExtension(JSArrayBuffer object,
+                                      ArrayBufferExtension* extension) {
+  return array_buffer_sweeper_->Detach(object, extension);
+}
+
 void Heap::AutomaticallyRestoreInitialHeapLimit(double threshold_percent) {
   initial_max_old_generation_size_threshold_ =
       initial_max_old_generation_size_ * threshold_percent;

src/heap/heap.h

@@ -704,6 +704,8 @@ class Heap {
 
   void AppendArrayBufferExtension(JSArrayBuffer object,
                                   ArrayBufferExtension* extension);
+  void DetachArrayBufferExtension(JSArrayBuffer object,
+                                  ArrayBufferExtension* extension);
 
   GlobalSafepoint* safepoint() { return safepoint_.get(); }
 

src/heap/new-spaces.cc

@@ -744,9 +744,11 @@ void NewSpace::Verify(Isolate* isolate) {
     CHECK_EQ(external_space_bytes[t], ExternalBackingStoreBytes(t));
   }
 
+  if (!FLAG_concurrent_array_buffer_sweeping) {
     size_t bytes = heap()->array_buffer_sweeper()->young().BytesSlow();
     CHECK_EQ(bytes,
              ExternalBackingStoreBytes(ExternalBackingStoreType::kArrayBuffer));
+  }
 
   // Check semi-spaces.
   CHECK_EQ(from_space_.id(), kFromSpace);

src/heap/paged-spaces.cc

@@ -731,7 +731,7 @@ void PagedSpace::Verify(Isolate* isolate, ObjectVisitor* visitor) {
   }
   CHECK(allocation_pointer_found_in_space);
 
-  if (identity() == OLD_SPACE) {
+  if (identity() == OLD_SPACE && !FLAG_concurrent_array_buffer_sweeping) {
     size_t bytes = heap()->array_buffer_sweeper()->old().BytesSlow();
     CHECK_EQ(bytes,
              ExternalBackingStoreBytes(ExternalBackingStoreType::kArrayBuffer));

src/objects/js-array-buffer.cc

@@ -86,9 +86,12 @@ void JSArrayBuffer::Detach(bool force_for_wasm_memory) {
   }
 
   Isolate* const isolate = GetIsolate();
-  if (backing_store()) {
-    std::shared_ptr<BackingStore> backing_store;
-      backing_store = RemoveExtension();
+  ArrayBufferExtension* extension = this->extension();
+
+  if (extension) {
+    DisallowGarbageCollection disallow_gc;
+    isolate->heap()->DetachArrayBufferExtension(*this, extension);
+    std::shared_ptr<BackingStore> backing_store = RemoveExtension();
     CHECK_IMPLIES(force_for_wasm_memory, backing_store->is_wasm_memory());
   }
 

src/objects/js-array-buffer.h

@@ -167,7 +167,7 @@ class ArrayBufferExtension : public Malloced {
   std::atomic<GcState> young_gc_state_;
   std::shared_ptr<BackingStore> backing_store_;
   ArrayBufferExtension* next_;
-  std::size_t accounting_length_;
+  std::atomic<size_t> accounting_length_;
 
   GcState young_gc_state() {
     return young_gc_state_.load(std::memory_order_relaxed);
@@ -205,10 +205,16 @@ class ArrayBufferExtension : public Malloced {
   std::shared_ptr<BackingStore> backing_store() { return backing_store_; }
   BackingStore* backing_store_raw() { return backing_store_.get(); }
 
-  size_t accounting_length() { return accounting_length_; }
+  size_t accounting_length() {
+    return accounting_length_.load(std::memory_order_relaxed);
+  }
 
   void set_accounting_length(size_t accounting_length) {
-    accounting_length_ = accounting_length;
+    accounting_length_.store(accounting_length, std::memory_order_relaxed);
+  }
+
+  size_t ClearAccountingLength() {
+    return accounting_length_.exchange(0, std::memory_order_relaxed);
   }
 
   std::shared_ptr<BackingStore> RemoveBackingStore() {