From a8e4e98eb6220f7eaedab9de48ff3766e0dc9d6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Tue, 5 May 2020 09:26:57 +0200 Subject: [PATCH] deps: V8: cherry-pick e29c62b74854 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Commit-Queue: Ulan Degenbaev Cr-Commit-Position: refs/heads/master@{#67420} Refs: https://github.com/v8/v8/commit/e29c62b7485462c1ce8f4129b26e7f7a4d4b193c Backport-PR-URL: https://github.com/nodejs/node/pull/33376 PR-URL: https://github.com/nodejs/node/pull/32831 Reviewed-By: Anna Henningsen Reviewed-By: Michaƫl Zasso Reviewed-By: Jiawen Geng Reviewed-By: Colin Ihrig --- common.gypi | 2 +- deps/v8/src/objects/backing-store.cc | 5 ++- deps/v8/test/cctest/test-api-array-buffer.cc | 40 ++++++++++++++++++++ 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/common.gypi b/common.gypi index 0d092cbd7c69a9..49a0097ce3ad6b 100644 --- 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.11', + 'v8_embedder_string': '-node.12', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/src/objects/backing-store.cc b/deps/v8/src/objects/backing-store.cc index 52ab0085f7c75a..a1db32f5dce471 100644 --- a/deps/v8/src/objects/backing-store.cc +++ b/deps/v8/src/objects/backing-store.cc @@ -165,7 +165,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 index b15fe801511e03..4c1b87797e7515 100644 --- a/deps/v8/test/cctest/test-api-array-buffer.cc +++ b/deps/v8/test/cctest/test-api-array-buffer.cc @@ -799,6 +799,46 @@ TEST(BackingStore_HoldAllocatorAlive_AfterIsolateShutdown) { 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 allocator = + std::make_shared(); + std::weak_ptr 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 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_ReallocateExpand) { LocalContext env; v8::Isolate* isolate = env->GetIsolate();