Refactor construction of [Shared]ArrayBuffers.

The backing store is now propagated to the constructors directly,
instead of being attached after the construction. This ensures that
the backing store is allocated before the array buffer so that we can
trigger GCs on backing store allocation (if allocation fails).

The only exception is builtin where we have to allocate the array buffer
before the backing store to comply with the spec.

Bug: v8:9380
Tbr: verwaest@chromium.org
Change-Id: Ib37db65853f3673dd769368cc3e8b6538ad07ff2
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1853444
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64240}

src/api/api.cc

@@ -3738,7 +3738,7 @@ std::shared_ptr<v8::BackingStore> v8::ArrayBuffer::GetBackingStore() {
   std::shared_ptr<i::BackingStore> backing_store = self->GetBackingStore();
   if (!backing_store) {
     backing_store =
-        i::BackingStore::NewEmptyBackingStore(i::SharedFlag::kNotShared);
+        i::BackingStore::EmptyBackingStore(i::SharedFlag::kNotShared);
   }
   i::GlobalBackingStoreRegistry::Register(backing_store);
   std::shared_ptr<i::BackingStoreBase> bs_base = backing_store;
@@ -3749,8 +3749,7 @@ std::shared_ptr<v8::BackingStore> v8::SharedArrayBuffer::GetBackingStore() {
   i::Handle<i::JSArrayBuffer> self = Utils::OpenHandle(this);
   std::shared_ptr<i::BackingStore> backing_store = self->GetBackingStore();
   if (!backing_store) {
-    backing_store =
-        i::BackingStore::NewEmptyBackingStore(i::SharedFlag::kShared);
+    backing_store = i::BackingStore::EmptyBackingStore(i::SharedFlag::kShared);
   }
   i::GlobalBackingStoreRegistry::Register(backing_store);
   std::shared_ptr<i::BackingStoreBase> bs_base = backing_store;
@@ -7427,8 +7426,8 @@ Local<ArrayBuffer> v8::ArrayBuffer::New(Isolate* isolate, void* data,
   std::shared_ptr<i::BackingStore> backing_store = LookupOrCreateBackingStore(
       i_isolate, data, byte_length, i::SharedFlag::kNotShared, mode);
 
-  i::Handle<i::JSArrayBuffer> obj = i_isolate->factory()->NewJSArrayBuffer();
-  obj->Attach(std::move(backing_store));
+  i::Handle<i::JSArrayBuffer> obj =
+      i_isolate->factory()->NewJSArrayBuffer(std::move(backing_store));
   if (mode == ArrayBufferCreationMode::kExternalized) {
     obj->set_is_external(true);
   }
@@ -7448,8 +7447,8 @@ Local<ArrayBuffer> v8::ArrayBuffer::New(
   Utils::ApiCheck(
       !i_backing_store->is_shared(), "v8_ArrayBuffer_New",
       "Cannot construct ArrayBuffer with a BackingStore of SharedArrayBuffer");
-  i::Handle<i::JSArrayBuffer> obj = i_isolate->factory()->NewJSArrayBuffer();
-  obj->Attach(std::move(i_backing_store));
+  i::Handle<i::JSArrayBuffer> obj =
+      i_isolate->factory()->NewJSArrayBuffer(std::move(i_backing_store));
   return Utils::ToLocal(obj);
 }
 
@@ -7595,9 +7594,8 @@ i::Handle<i::JSArrayBuffer> SetupSharedArrayBuffer(
       i_isolate, data, byte_length, i::SharedFlag::kShared, mode);
 
   i::Handle<i::JSArrayBuffer> obj =
-      i_isolate->factory()->NewJSSharedArrayBuffer();
+      i_isolate->factory()->NewJSSharedArrayBuffer(std::move(backing_store));
 
-  obj->Attach(backing_store);
   if (mode == ArrayBufferCreationMode::kExternalized) {
     obj->set_is_external(true);
   }
@@ -7717,8 +7715,7 @@ Local<SharedArrayBuffer> v8::SharedArrayBuffer::New(Isolate* isolate,
   }
 
   i::Handle<i::JSArrayBuffer> obj =
-      i_isolate->factory()->NewJSSharedArrayBuffer();
-  obj->Attach(std::move(backing_store));
+      i_isolate->factory()->NewJSSharedArrayBuffer(std::move(backing_store));
   return Utils::ToLocalShared(obj);
 }
 
@@ -7744,8 +7741,7 @@ Local<SharedArrayBuffer> v8::SharedArrayBuffer::New(
       i_backing_store->is_shared(), "v8_SharedArrayBuffer_New",
       "Cannot construct SharedArrayBuffer with BackingStore of ArrayBuffer");
   i::Handle<i::JSArrayBuffer> obj =
-      i_isolate->factory()->NewJSSharedArrayBuffer();
-  obj->Attach(std::move(i_backing_store));
+      i_isolate->factory()->NewJSSharedArrayBuffer(std::move(i_backing_store));
   return Utils::ToLocalShared(obj);
 }
 

src/builtins/builtins-arraybuffer.cc

@@ -31,35 +31,37 @@ namespace {
 Object ConstructBuffer(Isolate* isolate, Handle<JSFunction> target,
                        Handle<JSReceiver> new_target, Handle<Object> length,
                        InitializedFlag initialized) {
+  SharedFlag shared = (*target != target->native_context().array_buffer_fun())
+                          ? SharedFlag::kShared
+                          : SharedFlag::kNotShared;
   Handle<JSObject> result;
   ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
       isolate, result,
       JSObject::New(target, new_target, Handle<AllocationSite>::null()));
   auto array_buffer = Handle<JSArrayBuffer>::cast(result);
-  SharedFlag shared = (*target != target->native_context().array_buffer_fun())
-                          ? SharedFlag::kShared
-                          : SharedFlag::kNotShared;
+  // Ensure that all fields are initialized because BackingStore::Allocate is
+  // allowed to GC. Note that we cannot move the allocation of the ArrayBuffer
+  // after BackingStore::Allocate because of the spec.
+  array_buffer->Setup(shared, nullptr);
 
   size_t byte_length;
   if (!TryNumberToSize(*length, &byte_length) ||
       byte_length > JSArrayBuffer::kMaxByteLength) {
     // ToNumber failed.
-    array_buffer->SetupEmpty(shared);
     THROW_NEW_ERROR_RETURN_FAILURE(
         isolate, NewRangeError(MessageTemplate::kInvalidArrayBufferLength));
   }
 
   auto backing_store =
       BackingStore::Allocate(isolate, byte_length, shared, initialized);
-  if (backing_store) {
-    array_buffer->Attach(std::move(backing_store));
-    return *array_buffer;
+  if (!backing_store) {
+    // Allocation of backing store failed.
+    THROW_NEW_ERROR_RETURN_FAILURE(
+        isolate, NewRangeError(MessageTemplate::kArrayBufferAllocationFailed));
   }
 
-  // Allocation of backing store failed.
-  array_buffer->SetupEmpty(shared);
-  THROW_NEW_ERROR_RETURN_FAILURE(
-      isolate, NewRangeError(MessageTemplate::kArrayBufferAllocationFailed));
+  array_buffer->Attach(std::move(backing_store));
+  return *array_buffer;
 }
 
 }  // namespace

src/heap/factory.cc

@@ -3088,41 +3088,42 @@ Handle<SyntheticModule> Factory::NewSyntheticModule(
   return module;
 }
 
-Handle<JSArrayBuffer> Factory::NewJSArrayBuffer(AllocationType allocation) {
+Handle<JSArrayBuffer> Factory::NewJSArrayBuffer(
+    std::shared_ptr<BackingStore> backing_store, AllocationType allocation) {
   Handle<Map> map(isolate()->native_context()->array_buffer_fun().initial_map(),
                   isolate());
   auto result =
       Handle<JSArrayBuffer>::cast(NewJSObjectFromMap(map, allocation));
-  result->SetupEmpty(SharedFlag::kNotShared);
+  result->Setup(SharedFlag::kNotShared, std::move(backing_store));
   return result;
 }
 
 MaybeHandle<JSArrayBuffer> Factory::NewJSArrayBufferAndBackingStore(
     size_t byte_length, InitializedFlag initialized,
     AllocationType allocation) {
-  // TODO(titzer): Don't bother allocating a 0-length backing store.
-  // This is currently required because the embedder API for
-  // TypedArray::HasBuffer() checks if the backing store is nullptr.
-  // That check should be changed.
-
-  std::unique_ptr<BackingStore> backing_store = BackingStore::Allocate(
-      isolate(), byte_length, SharedFlag::kNotShared, initialized);
-  if (!backing_store) return MaybeHandle<JSArrayBuffer>();
+  std::unique_ptr<BackingStore> backing_store = nullptr;
+
+  if (byte_length > 0) {
+    backing_store = BackingStore::Allocate(isolate(), byte_length,
+                                           SharedFlag::kNotShared, initialized);
+    if (!backing_store) return MaybeHandle<JSArrayBuffer>();
+  }
   Handle<Map> map(isolate()->native_context()->array_buffer_fun().initial_map(),
                   isolate());
   auto array_buffer =
       Handle<JSArrayBuffer>::cast(NewJSObjectFromMap(map, allocation));
-  array_buffer->Attach(std::move(backing_store));
+  array_buffer->Setup(SharedFlag::kNotShared, std::move(backing_store));
   return array_buffer;
 }
 
-Handle<JSArrayBuffer> Factory::NewJSSharedArrayBuffer() {
+Handle<JSArrayBuffer> Factory::NewJSSharedArrayBuffer(
+    std::shared_ptr<BackingStore> backing_store) {
   Handle<Map> map(
       isolate()->native_context()->shared_array_buffer_fun().initial_map(),
       isolate());
   auto result = Handle<JSArrayBuffer>::cast(
       NewJSObjectFromMap(map, AllocationType::kYoung));
-  result->SetupEmpty(SharedFlag::kShared);
+  result->Setup(SharedFlag::kShared, std::move(backing_store));
   return result;
 }
 

src/heap/factory.h

@@ -665,13 +665,15 @@ class V8_EXPORT_PRIVATE Factory {
       v8::Module::SyntheticModuleEvaluationSteps evaluation_steps);
 
   Handle<JSArrayBuffer> NewJSArrayBuffer(
+      std::shared_ptr<BackingStore> backing_store,
       AllocationType allocation = AllocationType::kYoung);
 
   MaybeHandle<JSArrayBuffer> NewJSArrayBufferAndBackingStore(
       size_t byte_length, InitializedFlag initialized,
       AllocationType allocation = AllocationType::kYoung);
 
-  Handle<JSArrayBuffer> NewJSSharedArrayBuffer();
+  Handle<JSArrayBuffer> NewJSSharedArrayBuffer(
+      std::shared_ptr<BackingStore> backing_store);
 
   static void TypeAndSizeForElementsKind(ElementsKind kind,
                                          ExternalArrayType* array_type,

src/objects/backing-store.cc

@@ -455,7 +455,7 @@ std::unique_ptr<BackingStore> BackingStore::WrapAllocation(
   return std::unique_ptr<BackingStore>(result);
 }
 
-std::unique_ptr<BackingStore> BackingStore::NewEmptyBackingStore(
+std::unique_ptr<BackingStore> BackingStore::EmptyBackingStore(
     SharedFlag shared) {
   auto result = new BackingStore(nullptr,  // start
                                  0,        // length
@@ -630,8 +630,7 @@ void GlobalBackingStoreRegistry::UpdateSharedWasmMemoryObjects(
 
     if (old_buffer->byte_length() != backing_store->byte_length()) {
       Handle<JSArrayBuffer> new_buffer =
-          isolate->factory()->NewJSSharedArrayBuffer();
-      new_buffer->Attach(backing_store);
+          isolate->factory()->NewJSSharedArrayBuffer(std::move(backing_store));
       memory_object->update_instances(isolate, new_buffer);
     }
   }

src/objects/backing-store.h

@@ -64,7 +64,7 @@ class V8_EXPORT_PRIVATE BackingStore : public BackingStoreBase {
                                                       bool free_on_destruct);
 
   // Create an empty backing store.
-  static std::unique_ptr<BackingStore> NewEmptyBackingStore(SharedFlag shared);
+  static std::unique_ptr<BackingStore> EmptyBackingStore(SharedFlag shared);
 
   // Accessors.
   void* buffer_start() const { return buffer_start_; }

src/objects/js-array-buffer.cc

@@ -34,16 +34,31 @@ bool CanonicalNumericIndexString(Isolate* isolate, Handle<Object> s,
 }
 }  // anonymous namespace
 
-void JSArrayBuffer::SetupEmpty(SharedFlag shared) {
+void JSArrayBuffer::Setup(SharedFlag shared,
+                          std::shared_ptr<BackingStore> backing_store) {
   clear_padding();
   set_bit_field(0);
   set_is_shared(shared == SharedFlag::kShared);
   set_is_detachable(shared != SharedFlag::kShared);
-  set_backing_store(nullptr);
-  set_byte_length(0);
   for (int i = 0; i < v8::ArrayBuffer::kEmbedderFieldCount; i++) {
     SetEmbedderField(i, Smi::kZero);
   }
+  if (!backing_store) {
+    set_backing_store(nullptr);
+    set_byte_length(0);
+  } else {
+    Attach(std::move(backing_store));
+  }
+}
+
+void JSArrayBuffer::Attach(std::shared_ptr<BackingStore> backing_store) {
+  DCHECK_NOT_NULL(backing_store);
+  DCHECK_EQ(is_shared(), backing_store->is_shared());
+  set_backing_store(backing_store->buffer_start());
+  set_byte_length(backing_store->byte_length());
+  if (backing_store->is_wasm_memory()) set_is_detachable(false);
+  if (!backing_store->free_on_destruct()) set_is_external(true);
+  GetIsolate()->heap()->RegisterBackingStore(*this, std::move(backing_store));
 }
 
 void JSArrayBuffer::Detach(bool force_for_wasm_memory) {
@@ -73,19 +88,6 @@ void JSArrayBuffer::Detach(bool force_for_wasm_memory) {
   set_was_detached(true);
 }
 
-void JSArrayBuffer::Attach(std::shared_ptr<BackingStore> backing_store) {
-  SetupEmpty(backing_store->is_shared() ? SharedFlag::kShared
-                                        : SharedFlag::kNotShared);
-
-  if (backing_store->is_wasm_memory()) set_is_detachable(false);
-
-  set_backing_store(backing_store->buffer_start());
-  set_byte_length(backing_store->byte_length());
-  if (!backing_store->free_on_destruct()) set_is_external(true);
-
-  GetIsolate()->heap()->RegisterBackingStore(*this, std::move(backing_store));
-}
-
 std::shared_ptr<BackingStore> JSArrayBuffer::GetBackingStore() {
   return GetIsolate()->heap()->LookupBackingStore(*this);
 }
@@ -121,7 +123,7 @@ Handle<JSArrayBuffer> JSTypedArray::GetBuffer() {
   }
 
   // Attach the backing store to the array buffer.
-  array_buffer->Attach(std::move(backing_store));
+  array_buffer->Setup(SharedFlag::kNotShared, std::move(backing_store));
 
   // Clear the elements of the typed array.
   self->set_elements(ReadOnlyRoots(isolate).empty_byte_array());

src/objects/js-array-buffer.h

@@ -73,17 +73,15 @@ class JSArrayBuffer : public JSObject {
 
   DECL_CAST(JSArrayBuffer)
 
-  // Immediately after creating an array buffer, the internal untagged fields
-  // are garbage. They need to be initialized with either {SetupEmpty()} or
-  // have a backing store attached via {Attach()}.
-
-  // Setup an array buffer with no backing store.
-  V8_EXPORT_PRIVATE void SetupEmpty(SharedFlag shared);
-
-  // Attach a backing store to this array buffer.
-  // (note: this registers it with src/heap/array-buffer-tracker.h)
+  // Initializes the fields of the ArrayBuffer. The provided backing_store can
+  // be nullptr. If it is not nullptr, then the function registers it with
+  // src/heap/array-buffer-tracker.h.
+  V8_EXPORT_PRIVATE void Setup(SharedFlag shared,
+                               std::shared_ptr<BackingStore> backing_store);
+
+  // Attaches the backing store to an already constructed empty ArrayBuffer.
+  // This is intended to be used only in ArrayBufferConstructor builtin.
   V8_EXPORT_PRIVATE void Attach(std::shared_ptr<BackingStore> backing_store);
-
   // Detach the backing store from this array buffer if it is detachable.
   // This sets the internal pointer and length to 0 and unregisters the backing
   // store from the array buffer tracker. If the array buffer is not detachable,

src/snapshot/deserializer.cc

@@ -311,11 +311,10 @@ HeapObject Deserializer::PostProcessNewObject(HeapObject obj,
       // Serializer writes backing store ref in |backing_store| field.
       size_t store_index = reinterpret_cast<size_t>(buffer.backing_store());
       auto backing_store = backing_stores_[store_index];
-      if (backing_store) {
-        buffer.Attach(backing_store);
-      } else {
-        buffer.SetupEmpty(SharedFlag::kNotShared);
-      }
+      SharedFlag shared = backing_store && backing_store->is_shared()
+                              ? SharedFlag::kShared
+                              : SharedFlag::kNotShared;
+      buffer.Setup(shared, backing_store);
     }
   } else if (obj.IsBytecodeArray()) {
     // TODO(mythria): Remove these once we store the default values for these

src/wasm/wasm-objects.cc

@@ -1252,8 +1252,7 @@ Handle<WasmMemoryObject> WasmMemoryObject::New(
     // If no buffer was provided, create a zero-length one.
     auto backing_store =
         BackingStore::AllocateWasmMemory(isolate, 0, 0, SharedFlag::kNotShared);
-    buffer = isolate->factory()->NewJSArrayBuffer();
-    buffer->Attach(std::move(backing_store));
+    buffer = isolate->factory()->NewJSArrayBuffer(std::move(backing_store));
   }
 
   Handle<JSFunction> memory_ctor(
@@ -1295,10 +1294,8 @@ MaybeHandle<WasmMemoryObject> WasmMemoryObject::New(Isolate* isolate,
 
   Handle<JSArrayBuffer> buffer =
       (shared == SharedFlag::kShared)
-          ? isolate->factory()->NewJSSharedArrayBuffer()
-          : isolate->factory()->NewJSArrayBuffer();
-
-  buffer->Attach(std::move(backing_store));
+          ? isolate->factory()->NewJSSharedArrayBuffer(std::move(backing_store))
+          : isolate->factory()->NewJSArrayBuffer(std::move(backing_store));
 
   return New(isolate, buffer, maximum);
 }
@@ -1389,8 +1386,8 @@ int32_t WasmMemoryObject::Grow(Isolate* isolate,
   if (backing_store->GrowWasmMemoryInPlace(isolate, pages, maximum_pages)) {
     // Detach old and create a new one with the grown backing store.
     old_buffer->Detach(true);
-    Handle<JSArrayBuffer> new_buffer = isolate->factory()->NewJSArrayBuffer();
-    new_buffer->Attach(backing_store);
+    Handle<JSArrayBuffer> new_buffer =
+        isolate->factory()->NewJSArrayBuffer(std::move(backing_store));
     memory_object->update_instances(isolate, new_buffer);
     return static_cast<int32_t>(old_pages);  // success
   }
@@ -1401,8 +1398,8 @@ int32_t WasmMemoryObject::Grow(Isolate* isolate,
 
   // Detach old and create a new one with the new backing store.
   old_buffer->Detach(true);
-  Handle<JSArrayBuffer> new_buffer = isolate->factory()->NewJSArrayBuffer();
-  new_buffer->Attach(std::move(new_backing_store));
+  Handle<JSArrayBuffer> new_buffer =
+      isolate->factory()->NewJSArrayBuffer(std::move(new_backing_store));
   memory_object->update_instances(isolate, new_buffer);
   return static_cast<int32_t>(old_pages);  // success
 }

test/unittests/objects/value-serializer-unittest.cc

@@ -2018,8 +2018,8 @@ class ValueSerializerTestWithSharedArrayBufferClone
           i_isolate, pages, pages, i::SharedFlag::kShared);
       memcpy(backing_store->buffer_start(), data, byte_length);
       i::Handle<i::JSArrayBuffer> buffer =
-          i_isolate->factory()->NewJSSharedArrayBuffer();
-      buffer->Attach(std::move(backing_store));
+          i_isolate->factory()->NewJSSharedArrayBuffer(
+              std::move(backing_store));
       return Utils::ToLocalShared(buffer);
     } else {
       return SharedArrayBuffer::New(isolate(), data, byte_length);