deps: V8: backport e29c62b74854

addaleax · targos · commit c5a4534d5cbe · 2020-05-04T14:23:53.000+02:00
Original commit message: [arraybuffer] Clean up BackingStore even if it pointer to nullptr For a zero-length BackingStore allocation, it is valid for the underlying memory to be a null pointer. However, some cleanup is still necessary, since the BackingStore may hold a reference to the allocator itself, which needs to be released when destroying the `BackingStore` instance. Change-Id: I1f168079d39e4592d2fde31fbe5f705586690e85 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2169646 Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Commit-Queue: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#67420} Refs: v8/v8@e29c62b PR-URL: #33125 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
diff --git a/common.gypi b/common.gypi
@@ -36,7 +36,7 @@
 
     # Reset this number to 0 on major V8 upgrades.
     # Increment by one for each non-official patch applied to deps/v8.
-    'v8_embedder_string': '-node.32',
+    'v8_embedder_string': '-node.33',
 
     ##### V8 defaults for Node.js #####
 
diff --git a/deps/v8/src/objects/backing-store.cc b/deps/v8/src/objects/backing-store.cc
@@ -164,7 +164,10 @@ void BackingStore::Clear() {
 BackingStore::~BackingStore() {
   GlobalBackingStoreRegistry::Unregister(this);
 
-  if (buffer_start_ == nullptr) return;  // nothing to deallocate
+  if (buffer_start_ == nullptr) {
+    Clear();
+    return;
+  }
 
   if (is_wasm_memory_) {
     DCHECK(free_on_destruct_);
diff --git a/deps/v8/test/cctest/test-api-array-buffer.cc b/deps/v8/test/cctest/test-api-array-buffer.cc
@@ -729,6 +729,46 @@ TEST(BackingStore_HoldAllocatorAlive_UntilIsolateShutdown) {
   CHECK(allocator_weak.expired());
 }
 
+class NullptrAllocator final : public v8::ArrayBuffer::Allocator {
+ public:
+  void* Allocate(size_t length) override {
+    CHECK_EQ(length, 0);
+    return nullptr;
+  }
+  void* AllocateUninitialized(size_t length) override {
+    CHECK_EQ(length, 0);
+    return nullptr;
+  }
+  void Free(void* data, size_t length) override { CHECK_EQ(data, nullptr); }
+};
+
+TEST(BackingStore_ReleaseAllocator_NullptrBackingStore) {
+  std::shared_ptr<NullptrAllocator> allocator =
+      std::make_shared<NullptrAllocator>();
+  std::weak_ptr<NullptrAllocator> allocator_weak(allocator);
+
+  v8::Isolate::CreateParams create_params;
+  create_params.array_buffer_allocator_shared = allocator;
+  v8::Isolate* isolate = v8::Isolate::New(create_params);
+  isolate->Enter();
+
+  allocator.reset();
+  create_params.array_buffer_allocator_shared.reset();
+  CHECK(!allocator_weak.expired());
+
+  {
+    std::shared_ptr<v8::BackingStore> backing_store =
+        v8::ArrayBuffer::NewBackingStore(isolate, 0);
+    // This should release a reference to the allocator, even though the
+    // buffer is empty/nullptr.
+    backing_store.reset();
+  }
+
+  isolate->Exit();
+  isolate->Dispose();
+  CHECK(allocator_weak.expired());
+}
+
 TEST(BackingStore_HoldAllocatorAlive_AfterIsolateShutdown) {
   std::shared_ptr<DummyAllocator> allocator =
       std::make_shared<DummyAllocator>();
