Skip to content

Commit

Permalink
deps: V8: backport e29c62b74854
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
addaleax authored and targos committed May 4, 2020
1 parent 89ed7a5 commit c5a4534
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 2 deletions.
2 changes: 1 addition & 1 deletion common.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -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 #####

Expand Down
5 changes: 4 additions & 1 deletion deps/v8/src/objects/backing-store.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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_);
Expand Down
40 changes: 40 additions & 0 deletions deps/v8/test/cctest/test-api-array-buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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>();
Expand Down

0 comments on commit c5a4534

Please sign in to comment.