From c5a4534d5cbec234264c66b506e9661e270a7f5c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 28 Apr 2020 15:29:48 +0200 Subject: [PATCH] deps: V8: backport 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 PR-URL: https://github.com/nodejs/node/pull/33125 Reviewed-By: Colin Ihrig Reviewed-By: Michaël Zasso Reviewed-By: Juan José Arboleda --- 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 2ebabf8f80b38b..5e4f9b6dba1dce 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.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 index 8d604ba35d8d06..c9a5c2e4c8ec20 100644 --- 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 index e8e026f156053d..eeddbcca1003be 100644 --- 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 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_HoldAllocatorAlive_AfterIsolateShutdown) { std::shared_ptr allocator = std::make_shared();