[arraybuffer] Switch backing store from DataHandle to v8::BackingStore

With this CL we finally use v8::BackingStore as the backing store
implementation of blink::ArrayBuffer.

Changes:
* Use v8::BackingStore as the backing store for ArrayBufferContents.
* Remove ArraybufferContents::DataHolder and
  ArrayBufferContents::DataHandle.
* Replace all uses of DataHolder in ArraybufferContents with uses of
  v8::BackingStore.
* Replace the use of DataHandle in the V8 bindings.
* Add an exception to the presubmit check to allow std::shared_ptr in
  ArrayBufferContents. It is contained there and does not escape.

R=haraken@chromium.org, ulan@chromium.org

Bug: chromium:1008840
Change-Id: Iffa31e889daf4ae54de96f55b4612833d096a884
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1886858
Commit-Queue: Andreas Haas <ahaas@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712571}

PRESUBMIT.py

@@ -837,7 +837,9 @@ _BANNED_CPP_FUNCTIONS = (
         'std::shared_ptr should not be used. Use scoped_refptr instead.',
       ),
       True,
-      [_THIRD_PARTY_EXCEPT_BLINK],  # Not an error in third_party folders.
+      [_THIRD_PARTY_EXCEPT_BLINK,
+       '^third_party/blink/renderer/core/typed_arrays/array_buffer/' +
+         'array_buffer_contents\.(cc|h)'],
     ),
     (
       r'/\bstd::weak_ptr\b',

third_party/blink/renderer/bindings/templates/interface.cc.tmpl

@@ -1100,6 +1100,9 @@ v8::Local<v8::Object> {{v8_class}}::FindInstanceInPrototypeChain(
 {{cpp_class}}* V8{{interface_name}}::ToImpl(v8::Local<v8::Object> object) {
   DCHECK(object->Is{{interface_name}}());
   v8::Local<v8::{{interface_name}}> v8buffer = object.As<v8::{{interface_name}}>();
+  // TODO(ahaas): The use of IsExternal is wrong here. Instead we should call
+  // ToScriptWrappable(object)->ToImpl<ArrayBuffer>() and check for nullptr.
+  // We can then also avoid the call to Externalize below.
   if (v8buffer->IsExternal()) {
     const WrapperTypeInfo* wrapper_type = ToWrapperTypeInfo(object);
     CHECK(wrapper_type);
@@ -1109,12 +1112,9 @@ v8::Local<v8::Object> {{v8_class}}::FindInstanceInPrototypeChain(
 
   // Transfer the ownership of the allocated memory to an {{interface_name}} without
   // copying.
-  v8::{{interface_name}}::Contents v8_contents = v8buffer->Externalize();
-  ArrayBufferContents::DataHandle data(v8_contents.Data(),
-                                       v8_contents.ByteLength(),
-                                       v8_contents.Deleter(),
-                                       v8_contents.DeleterData());
-  ArrayBufferContents contents(std::move(data), ArrayBufferContents::k{% if interface_name == 'ArrayBuffer' %}Not{% endif %}Shared);
+  auto backing_store = v8buffer->GetBackingStore();
+  v8buffer->Externalize(backing_store);
+  ArrayBufferContents contents(std::move(backing_store));
   {{cpp_class}}* buffer = {{cpp_class}}::Create(contents);
   v8::Local<v8::Object> associatedWrapper = buffer->AssociateWithWrapper(v8::Isolate::GetCurrent(), buffer->GetWrapperTypeInfo(), object);
   DCHECK(associatedWrapper == object);

third_party/blink/renderer/bindings/tests/results/core/v8_array_buffer.cc

@@ -69,6 +69,9 @@ static_assert(
 TestArrayBuffer* V8ArrayBuffer::ToImpl(v8::Local<v8::Object> object) {
   DCHECK(object->IsArrayBuffer());
   v8::Local<v8::ArrayBuffer> v8buffer = object.As<v8::ArrayBuffer>();
+  // TODO(ahaas): The use of IsExternal is wrong here. Instead we should call
+  // ToScriptWrappable(object)->ToImpl<ArrayBuffer>() and check for nullptr.
+  // We can then also avoid the call to Externalize below.
   if (v8buffer->IsExternal()) {
     const WrapperTypeInfo* wrapper_type = ToWrapperTypeInfo(object);
     CHECK(wrapper_type);
@@ -78,12 +81,9 @@ TestArrayBuffer* V8ArrayBuffer::ToImpl(v8::Local<v8::Object> object) {
 
   // Transfer the ownership of the allocated memory to an ArrayBuffer without
   // copying.
-  v8::ArrayBuffer::Contents v8_contents = v8buffer->Externalize();
-  ArrayBufferContents::DataHandle data(v8_contents.Data(),
-                                       v8_contents.ByteLength(),
-                                       v8_contents.Deleter(),
-                                       v8_contents.DeleterData());
-  ArrayBufferContents contents(std::move(data), ArrayBufferContents::kNotShared);
+  auto backing_store = v8buffer->GetBackingStore();
+  v8buffer->Externalize(backing_store);
+  ArrayBufferContents contents(std::move(backing_store));
   TestArrayBuffer* buffer = TestArrayBuffer::Create(contents);
   v8::Local<v8::Object> associatedWrapper = buffer->AssociateWithWrapper(v8::Isolate::GetCurrent(), buffer->GetWrapperTypeInfo(), object);
   DCHECK(associatedWrapper == object);

third_party/blink/renderer/core/typed_arrays/array_buffer/array_buffer_contents.cc

@@ -34,79 +34,93 @@
 
 namespace blink {
 
-ArrayBufferContents::ArrayBufferContents()
-    : holder_(base::AdoptRef(new DataHolder())) {}
+ArrayBufferContents::ArrayBufferContents(void* data,
+                                         size_t length,
+                                         DataDeleter deleter,
+                                         SharingType is_shared) {
+  if (!data) {
+    return;
+  }
+  if (is_shared == kNotShared) {
+    backing_store_ =
+        v8::ArrayBuffer::NewBackingStore(data, length, deleter, nullptr);
+  } else {
+    backing_store_ =
+        v8::SharedArrayBuffer::NewBackingStore(data, length, deleter, nullptr);
+  }
+}
 
 ArrayBufferContents::ArrayBufferContents(
     size_t num_elements,
-    unsigned element_byte_size,
+    size_t element_byte_size,
     SharingType is_shared,
     ArrayBufferContents::InitializationPolicy policy)
-    : holder_(base::AdoptRef(new DataHolder())) {
-  // Do not allow 32-bit overflow of the total size.
-  size_t total_size = num_elements * element_byte_size;
-  if (num_elements) {
-    if (total_size / num_elements != element_byte_size) {
-      return;
-    }
-  }
+    : ArrayBufferContents(
+          AllocateMemoryOrNull(num_elements * element_byte_size, policy),
+          num_elements * element_byte_size,
+          [](void* data, size_t, void*) { FreeMemory(data); },
+          is_shared) {}
 
-  holder_->AllocateNew(total_size, is_shared, policy);
-}
-
-ArrayBufferContents::ArrayBufferContents(DataHandle data, SharingType is_shared)
-    : holder_(base::AdoptRef(new DataHolder())) {
-  if (data) {
-    holder_->Adopt(std::move(data), is_shared);
+ArrayBufferContents::ArrayBufferContents(
+    std::shared_ptr<v8::BackingStore> backing_store) {
+  if (!backing_store || backing_store->Data()) {
+    backing_store_ = std::move(backing_store);
+    return;
+  }
+  // ArrayBufferContents has to guarantee that Data() provides a valid pointer,
+  // even when DataSize() is '0'. That's why we create a new BackingStore here.
+
+  // TODO(ahaas): Remove this code here once nullptr is a valid result for
+  // Data().
+  CHECK_EQ(backing_store->ByteLength(), 0u);
+  void* data = AllocateMemoryOrNull(0, kDontInitialize);
+  CHECK_NE(data, nullptr);
+  DataDeleter deleter = [](void* data, size_t, void*) { FreeMemory(data); };
+  if (!backing_store->IsShared()) {
+    backing_store_ =
+        v8::ArrayBuffer::NewBackingStore(data, 0, deleter, nullptr);
   } else {
-    // Allow null data if size is 0 bytes, make sure data is valid pointer.
-    // (PartitionAlloc guarantees valid pointer for size 0)
-    holder_->AllocateNew(0, is_shared, kZeroInitialize);
+    backing_store_ =
+        v8::SharedArrayBuffer::NewBackingStore(data, 0, deleter, nullptr);
   }
 }
 
-ArrayBufferContents::ArrayBufferContents(void* data,
-                                         size_t length,
-                                         DataDeleter deleter,
-                                         SharingType is_shared)
-    : holder_(base::AdoptRef(new DataHolder())) {
-  holder_->Adopt(DataHandle(data, length, deleter, nullptr), is_shared);
-}
-
 ArrayBufferContents::~ArrayBufferContents() = default;
 
 void ArrayBufferContents::Detach() {
-  holder_ = nullptr;
+  backing_store_.reset();
 }
 
 void ArrayBufferContents::Reset() {
-  holder_ = base::MakeRefCounted<DataHolder>();
+  backing_store_.reset();
 }
 
 void ArrayBufferContents::Transfer(ArrayBufferContents& other) {
   DCHECK(!IsShared());
-  DCHECK(!other.holder_->Data());
-  other.holder_ = holder_;
-  Detach();
+  DCHECK(!other.Data());
+  other.backing_store_ = std::move(backing_store_);
 }
 
 void ArrayBufferContents::ShareWith(ArrayBufferContents& other) {
   DCHECK(IsShared());
-  DCHECK(!other.holder_->Data());
-  other.holder_ = holder_;
+  DCHECK(!other.Data());
+  other.backing_store_ = backing_store_;
 }
 
 void ArrayBufferContents::ShareNonSharedForInternalUse(
     ArrayBufferContents& other) {
   DCHECK(!IsShared());
-  DCHECK(!other.holder_->Data());
-  DCHECK(holder_->Data());
-  other.holder_ = holder_;
+  DCHECK(!other.Data());
+  DCHECK(Data());
+  other.backing_store_ = backing_store_;
 }
 
 void ArrayBufferContents::CopyTo(ArrayBufferContents& other) {
-  DCHECK(!holder_->IsShared() && !other.holder_->IsShared());
-  other.holder_->CopyMemoryFrom(*holder_);
+  other = ArrayBufferContents(
+      DataLength(), 1, IsShared() ? kShared : kNotShared, kDontInitialize);
+  if (!IsValid() || !other.IsValid())
+    return;
+  memcpy(other.Data(), Data(), DataLength());
 }
 
 void* ArrayBufferContents::AllocateMemoryWithFlags(size_t size,
@@ -130,58 +144,4 @@ void ArrayBufferContents::FreeMemory(void* data) {
   WTF::Partitions::ArrayBufferPartition()->Free(data);
 }
 
-ArrayBufferContents::DataHandle ArrayBufferContents::CreateDataHandle(
-    size_t size,
-    InitializationPolicy policy) {
-  return DataHandle(
-      ArrayBufferContents::AllocateMemoryOrNull(size, policy), size,
-      [](void* buffer, size_t, void*) { FreeMemory(buffer); }, nullptr);
-}
-
-ArrayBufferContents::DataHolder::DataHolder()
-    : data_(
-          nullptr,
-          0,
-          [](void*, size_t, void*) {},
-          nullptr),
-      is_shared_(kNotShared),
-      has_registered_external_allocation_(false) {}
-
-ArrayBufferContents::DataHolder::~DataHolder() {
-  is_shared_ = kNotShared;
-}
-
-void ArrayBufferContents::DataHolder::AllocateNew(size_t length,
-                                                  SharingType is_shared,
-                                                  InitializationPolicy policy) {
-  DCHECK(!data_);
-  DCHECK(!has_registered_external_allocation_);
-
-  data_ = CreateDataHandle(length, policy);
-  if (!data_)
-    return;
-
-  is_shared_ = is_shared;
-}
-
-void ArrayBufferContents::DataHolder::Adopt(DataHandle data,
-                                            SharingType is_shared) {
-  DCHECK(!data_);
-  DCHECK(!has_registered_external_allocation_);
-
-  data_ = std::move(data);
-  is_shared_ = is_shared;
-}
-
-void ArrayBufferContents::DataHolder::CopyMemoryFrom(const DataHolder& source) {
-  DCHECK(!data_);
-  DCHECK(!has_registered_external_allocation_);
-
-  data_ = CreateDataHandle(source.DataLength(), kDontInitialize);
-  if (!data_)
-    return;
-
-  memcpy(data_.Data(), source.Data(), source.DataLength());
-}
-
 }  // namespace blink

third_party/blink/renderer/core/typed_arrays/array_buffer/array_buffer_contents.h

@@ -36,6 +36,7 @@
 #include "third_party/blink/renderer/platform/wtf/cross_thread_copier.h"
 #include "third_party/blink/renderer/platform/wtf/thread_safe_ref_counted.h"
 #include "third_party/blink/renderer/platform/wtf/wtf.h"
+#include "v8/include/v8.h"
 
 namespace blink {
 
@@ -43,65 +44,13 @@ class CORE_EXPORT ArrayBufferContents {
   DISALLOW_NEW();
 
  public:
-  using AdjustAmountOfExternalAllocatedMemoryFunction = void (*)(int64_t diff);
   // Types that need to be used when injecting external memory.
-  // DataHandle allows specifying a deleter which will be invoked when
-  // DataHandle instance goes out of scope. If the data memory is allocated
-  // using ArrayBufferContents::AllocateMemoryOrNull, it is necessary to specify
-  // ArrayBufferContents::FreeMemory as the DataDeleter. Most clients would want
-  // to use ArrayBufferContents::CreateDataHandle, which allocates memory and
-  // specifies the correct deleter.
+  // v8::BackingStore allows specifying a deleter which will be invoked when
+  // v8::BackingStore instance goes out of scope. If the data memory is
+  // allocated using ArrayBufferContents::AllocateMemoryOrNull, it is necessary
+  // to specify ArrayBufferContents::FreeMemory as the DataDeleter.
   using DataDeleter = void (*)(void* data, size_t length, void* info);
 
-  class DataHandle {
-    DISALLOW_COPY_AND_ASSIGN(DataHandle);
-
-   public:
-    DataHandle() {}
-
-    DataHandle(void* data,
-               size_t length,
-               DataDeleter deleter,
-               void* deleter_info)
-        : data_(data),
-          data_length_(data ? length : 0),
-          deleter_(deleter),
-          deleter_info_(deleter_info) {}
-    // Move constructor
-    DataHandle(DataHandle&& other) { *this = std::move(other); }
-    ~DataHandle() {
-      if (!data_)
-        return;
-      deleter_(data_, data_length_, deleter_info_);
-    }
-
-    // Move operator
-    DataHandle& operator=(DataHandle&& other) {
-      if (data_)
-        deleter_(data_, data_length_, deleter_info_);
-      data_ = other.data_;
-      data_length_ = other.data_length_;
-      deleter_ = other.deleter_;
-      deleter_info_ = other.deleter_info_;
-      other.data_ = nullptr;
-      return *this;
-    }
-
-    void reset() { *this = DataHandle(); }
-
-    void* Data() const { return data_; }
-    size_t DataLength() const { return data_length_; }
-
-    operator bool() const { return data_; }
-
-   private:
-    void* data_ = nullptr;
-    size_t data_length_ = 0;
-
-    DataDeleter deleter_ = nullptr;
-    void* deleter_info_ = nullptr;
-  };
-
   enum InitializationPolicy { kZeroInitialize, kDontInitialize };
 
   enum SharingType {
@@ -109,17 +58,17 @@ class CORE_EXPORT ArrayBufferContents {
     kShared,
   };
 
-  ArrayBufferContents();
+  ArrayBufferContents() = default;
   ArrayBufferContents(size_t num_elements,
-                      unsigned element_byte_size,
+                      size_t element_byte_size,
                       SharingType is_shared,
                       InitializationPolicy);
   ArrayBufferContents(void* data,
                       size_t length,
                       DataDeleter deleter,
                       SharingType is_shared);
-  ArrayBufferContents(DataHandle, SharingType is_shared);
   ArrayBufferContents(ArrayBufferContents&&) = default;
+  explicit ArrayBufferContents(std::shared_ptr<v8::BackingStore> backing_store);
 
   ~ArrayBufferContents();
 
@@ -138,10 +87,16 @@ class CORE_EXPORT ArrayBufferContents {
     DCHECK(IsShared());
     return DataMaybeShared();
   }
-  void* DataMaybeShared() const { return holder_ ? holder_->Data() : nullptr; }
-  size_t DataLength() const { return holder_ ? holder_->DataLength() : 0; }
-  bool IsShared() const { return holder_ ? holder_->IsShared() : false; }
-  bool IsValid() const { return holder_->Data(); }
+  void* DataMaybeShared() const {
+    return backing_store_ ? backing_store_->Data() : nullptr;
+  }
+  size_t DataLength() const {
+    return backing_store_ ? backing_store_->ByteLength() : 0;
+  }
+  bool IsShared() const {
+    return backing_store_ ? backing_store_->IsShared() : false;
+  }
+  bool IsValid() const { return backing_store_ && backing_store_->Data(); }
 
   void Transfer(ArrayBufferContents& other);
   void ShareWith(ArrayBufferContents& other);
@@ -150,7 +105,6 @@ class CORE_EXPORT ArrayBufferContents {
 
   static void* AllocateMemoryOrNull(size_t, InitializationPolicy);
   static void FreeMemory(void*);
-  static DataHandle CreateDataHandle(size_t, InitializationPolicy);
 
  private:
   static void* AllocateMemoryWithFlags(size_t, InitializationPolicy, int);
@@ -158,31 +112,7 @@ class CORE_EXPORT ArrayBufferContents {
   static void DefaultAdjustAmountOfExternalAllocatedMemoryFunction(
       int64_t diff);
 
-  class CORE_EXPORT DataHolder : public ThreadSafeRefCounted<DataHolder> {
-    DISALLOW_COPY_AND_ASSIGN(DataHolder);
-
-   public:
-    DataHolder();
-    ~DataHolder();
-
-    void AllocateNew(size_t length,
-                     SharingType is_shared,
-                     InitializationPolicy);
-    void Adopt(DataHandle, SharingType is_shared);
-    void CopyMemoryFrom(const DataHolder& source);
-
-    const void* Data() const { return data_.Data(); }
-    void* Data() { return data_.Data(); }
-    size_t DataLength() const { return data_.DataLength(); }
-    bool IsShared() const { return is_shared_ == kShared; }
-
-   private:
-    DataHandle data_;
-    SharingType is_shared_;
-    bool has_registered_external_allocation_;
-  };
-
-  scoped_refptr<DataHolder> holder_;
+  std::shared_ptr<v8::BackingStore> backing_store_;
 
   DISALLOW_COPY_AND_ASSIGN(ArrayBufferContents);
 };