From 3051b5874fd8c10c67e510b3011bad86efe8436e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 19 Nov 2019 21:34:44 +0100 Subject: [PATCH 1/3] buffer: release buffers with free callbacks on env exit Invoke the free callback for a given `Buffer` if it was created with one, and mark the underlying `ArrayBuffer` as detached. This makes sure that the memory is released e.g. when addons inside Workers create such `Buffer`s. --- src/node_buffer.cc | 45 +++++++++++++++---- test/addons/worker-buffer-callback/binding.cc | 11 ++++- .../test-free-called.js | 17 +++++++ test/cctest/test_environment.cc | 32 +++++++++++++ 4 files changed, 95 insertions(+), 10 deletions(-) create mode 100644 test/addons/worker-buffer-callback/test-free-called.js diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 8641270eaecfcd..e5c4655b4ccdb9 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -53,6 +53,7 @@ using v8::Context; using v8::EscapableHandleScope; using v8::FunctionCallbackInfo; using v8::Global; +using v8::HandleScope; using v8::Int32; using v8::Integer; using v8::Isolate; @@ -73,8 +74,10 @@ namespace { class CallbackInfo { public: + ~CallbackInfo(); + static inline void Free(char* data, void* hint); - static inline CallbackInfo* New(Isolate* isolate, + static inline CallbackInfo* New(Environment* env, Local object, FreeCallback callback, char* data, @@ -84,9 +87,10 @@ class CallbackInfo { CallbackInfo& operator=(const CallbackInfo&) = delete; private: + static void CleanupHook(void* data); static void WeakCallback(const WeakCallbackInfo&); inline void WeakCallback(Isolate* isolate); - inline CallbackInfo(Isolate* isolate, + inline CallbackInfo(Environment* env, Local object, FreeCallback callback, char* data, @@ -95,6 +99,7 @@ class CallbackInfo { FreeCallback const callback_; char* const data_; void* const hint_; + Environment* const env_; }; @@ -103,31 +108,53 @@ void CallbackInfo::Free(char* data, void*) { } -CallbackInfo* CallbackInfo::New(Isolate* isolate, +CallbackInfo* CallbackInfo::New(Environment* env, Local object, FreeCallback callback, char* data, void* hint) { - return new CallbackInfo(isolate, object, callback, data, hint); + return new CallbackInfo(env, object, callback, data, hint); } -CallbackInfo::CallbackInfo(Isolate* isolate, +CallbackInfo::CallbackInfo(Environment* env, Local object, FreeCallback callback, char* data, void* hint) - : persistent_(isolate, object), + : persistent_(env->isolate(), object), callback_(callback), data_(data), - hint_(hint) { + hint_(hint), + env_(env) { ArrayBuffer::Contents obj_c = object->GetContents(); CHECK_EQ(data_, static_cast(obj_c.Data())); if (object->ByteLength() != 0) CHECK_NOT_NULL(data_); persistent_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter); - isolate->AdjustAmountOfExternalAllocatedMemory(sizeof(*this)); + env->AddCleanupHook(CleanupHook, this); + env->isolate()->AdjustAmountOfExternalAllocatedMemory(sizeof(*this)); +} + + +CallbackInfo::~CallbackInfo() { + persistent_.Reset(); + env_->RemoveCleanupHook(CleanupHook, this); +} + + +void CallbackInfo::CleanupHook(void* data) { + CallbackInfo* self = static_cast(data); + + { + HandleScope handle_scope(self->env_->isolate()); + Local ab = self->persistent_.Get(self->env_->isolate()); + CHECK(!ab.IsEmpty()); + ab->Detach(); + } + + self->WeakCallback(self->env_->isolate()); } @@ -388,7 +415,7 @@ MaybeLocal New(Environment* env, } MaybeLocal ui = Buffer::New(env, ab, 0, length); - CallbackInfo::New(env->isolate(), ab, callback, data, hint); + CallbackInfo::New(env, ab, callback, data, hint); if (ui.IsEmpty()) return MaybeLocal(); diff --git a/test/addons/worker-buffer-callback/binding.cc b/test/addons/worker-buffer-callback/binding.cc index a40876ebb523a6..1141c8a051e077 100644 --- a/test/addons/worker-buffer-callback/binding.cc +++ b/test/addons/worker-buffer-callback/binding.cc @@ -3,17 +3,24 @@ #include using v8::Context; +using v8::FunctionCallbackInfo; using v8::Isolate; using v8::Local; using v8::Object; using v8::Value; +uint32_t free_call_count = 0; char data[] = "hello"; +void GetFreeCallCount(const FunctionCallbackInfo& args) { + args.GetReturnValue().Set(free_call_count); +} + void Initialize(Local exports, Local module, Local context) { Isolate* isolate = context->GetIsolate(); + NODE_SET_METHOD(exports, "getFreeCallCount", GetFreeCallCount); exports->Set(context, v8::String::NewFromUtf8( isolate, "buffer", v8::NewStringType::kNormal) @@ -22,7 +29,9 @@ void Initialize(Local exports, isolate, data, sizeof(data), - [](char* data, void* hint) {}, + [](char* data, void* hint) { + free_call_count++; + }, nullptr).ToLocalChecked()).Check(); } diff --git a/test/addons/worker-buffer-callback/test-free-called.js b/test/addons/worker-buffer-callback/test-free-called.js new file mode 100644 index 00000000000000..2a3cc9e47c22ff --- /dev/null +++ b/test/addons/worker-buffer-callback/test-free-called.js @@ -0,0 +1,17 @@ +'use strict'; +const common = require('../../common'); +const path = require('path'); +const assert = require('assert'); +const { Worker } = require('worker_threads'); +const binding = path.resolve(__dirname, `./build/${common.buildType}/binding`); +const { getFreeCallCount } = require(binding); + +// Test that buffers allocated with a free callback through our APIs are +// released when a Worker owning it exits. + +const w = new Worker(`require(${JSON.stringify(binding)})`, { eval: true }); + +assert.strictEqual(getFreeCallCount(), 0); +w.on('exit', common.mustCall(() => { + assert.strictEqual(getFreeCallCount(), 1); +})); diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 0db2963acc9ba3..132f7b44f7db62 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -1,3 +1,4 @@ +#include "node_buffer.h" #include "node_internals.h" #include "libplatform/libplatform.h" @@ -208,3 +209,34 @@ TEST_F(EnvironmentTest, SetImmediateCleanup) { EXPECT_EQ(called, 1); EXPECT_EQ(called_unref, 0); } + +static char hello[] = "hello"; + +TEST_F(EnvironmentTest, BufferWithFreeCallbackIsDetached) { + // Test that a Buffer allocated with a free callback is detached after + // its callback has been called. + const v8::HandleScope handle_scope(isolate_); + const Argv argv; + + int callback_calls = 0; + + v8::Local ab; + { + Env env {handle_scope, argv}; + v8::Local buf_obj = node::Buffer::New( + isolate_, + hello, + sizeof(hello), + [](char* data, void* hint) { + CHECK_EQ(data, hello); + ++*static_cast(hint); + }, + &callback_calls).ToLocalChecked(); + CHECK(buf_obj->IsUint8Array()); + ab = buf_obj.As()->Buffer(); + CHECK_EQ(ab->ByteLength(), sizeof(hello)); + } + + CHECK_EQ(callback_calls, 1); + CHECK_EQ(ab->ByteLength(), 0); +} From 3bd4bbbc4528b018260f1b586f00494829776ca5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 19 Nov 2019 21:36:41 +0100 Subject: [PATCH 2/3] n-api: detach external ArrayBuffers on env exit Make sure that `ArrayBuffer`s created using `napi_create_external_arraybuffer` are rendered unusable after its memory has been released. --- src/js_native_api_v8.cc | 43 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 05712afde0020f..5506b2b4c6204b 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -186,8 +186,8 @@ inline static napi_status ConcludeDeferred(napi_env env, } // Wrapper around v8impl::Persistent that implements reference counting. -class Reference : private Finalizer, RefTracker { - private: +class Reference : protected Finalizer, RefTracker { + protected: Reference(napi_env env, v8::Local value, uint32_t initial_refcount, @@ -289,7 +289,7 @@ class Reference : private Finalizer, RefTracker { } } - private: + protected: void Finalize(bool is_env_teardown = false) override { if (_finalize_callback != nullptr) { _env->CallIntoModuleThrow([&](napi_env env) { @@ -310,6 +310,7 @@ class Reference : private Finalizer, RefTracker { } } + private: // The N-API finalizer callback may make calls into the engine. V8's heap is // not in a consistent state during the weak callback, and therefore it does // not support calls back into it. However, it provides a mechanism for adding @@ -335,6 +336,37 @@ class Reference : private Finalizer, RefTracker { bool _delete_self; }; +class ArrayBufferReference final : public Reference { + public: + // Same signatures for ctor and New() as Reference, except this only works + // with ArrayBuffers: + template + explicit ArrayBufferReference(napi_env env, + v8::Local value, + Args&&... args) + : Reference(env, value, std::forward(args)...) {} + + template + static ArrayBufferReference* New(napi_env env, + v8::Local value, + Args&&... args) { + return new ArrayBufferReference(env, value, std::forward(args)...); + } + + private: + void Finalize(bool is_env_teardown) override { + if (is_env_teardown) { + v8::HandleScope handle_scope(_env->isolate); + v8::Local ab = Get(); + CHECK(!ab.IsEmpty()); + CHECK(ab->IsArrayBuffer()); + ab.As()->Detach(); + } + + Reference::Finalize(is_env_teardown); + } +}; + enum UnwrapAction { KeepWrap, RemoveWrap @@ -2587,8 +2619,9 @@ napi_status napi_create_external_arraybuffer(napi_env env, if (finalize_cb != nullptr) { // Create a self-deleting weak reference that invokes the finalizer - // callback. - v8impl::Reference::New(env, + // callback and detaches the ArrayBuffer if it still exists on Environment + // teardown. + v8impl::ArrayBufferReference::New(env, buffer, 0, true, From e59af1b9cce44260aaf1c4cc0339fc706d9293c8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 19 Nov 2019 21:39:07 +0100 Subject: [PATCH 3/3] test: port worker + buffer test to N-API This ports `test/addons/worker-buffer-callback` to N-API, with the small exception of using external `ArrayBuffer`s rather than external Node.js `Buffer`s. --- .../test_worker_buffer_callback/binding.gyp | 8 ++++ .../test-free-called.js | 17 ++++++++ .../test_worker_buffer_callback/test.js | 15 +++++++ .../test_worker_buffer_callback.c | 43 +++++++++++++++++++ 4 files changed, 83 insertions(+) create mode 100644 test/node-api/test_worker_buffer_callback/binding.gyp create mode 100644 test/node-api/test_worker_buffer_callback/test-free-called.js create mode 100644 test/node-api/test_worker_buffer_callback/test.js create mode 100644 test/node-api/test_worker_buffer_callback/test_worker_buffer_callback.c diff --git a/test/node-api/test_worker_buffer_callback/binding.gyp b/test/node-api/test_worker_buffer_callback/binding.gyp new file mode 100644 index 00000000000000..7ab6381930da9c --- /dev/null +++ b/test/node-api/test_worker_buffer_callback/binding.gyp @@ -0,0 +1,8 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'sources': [ 'test_worker_buffer_callback.c' ] + } + ] +} diff --git a/test/node-api/test_worker_buffer_callback/test-free-called.js b/test/node-api/test_worker_buffer_callback/test-free-called.js new file mode 100644 index 00000000000000..2a3cc9e47c22ff --- /dev/null +++ b/test/node-api/test_worker_buffer_callback/test-free-called.js @@ -0,0 +1,17 @@ +'use strict'; +const common = require('../../common'); +const path = require('path'); +const assert = require('assert'); +const { Worker } = require('worker_threads'); +const binding = path.resolve(__dirname, `./build/${common.buildType}/binding`); +const { getFreeCallCount } = require(binding); + +// Test that buffers allocated with a free callback through our APIs are +// released when a Worker owning it exits. + +const w = new Worker(`require(${JSON.stringify(binding)})`, { eval: true }); + +assert.strictEqual(getFreeCallCount(), 0); +w.on('exit', common.mustCall(() => { + assert.strictEqual(getFreeCallCount(), 1); +})); diff --git a/test/node-api/test_worker_buffer_callback/test.js b/test/node-api/test_worker_buffer_callback/test.js new file mode 100644 index 00000000000000..4884a27d39e6a2 --- /dev/null +++ b/test/node-api/test_worker_buffer_callback/test.js @@ -0,0 +1,15 @@ +'use strict'; +const common = require('../../common'); +const assert = require('assert'); +const { MessageChannel } = require('worker_threads'); +const { buffer } = require(`./build/${common.buildType}/binding`); + +// Test that buffers allocated with a free callback through our APIs are not +// transferred. + +const { port1 } = new MessageChannel(); +const origByteLength = buffer.byteLength; +port1.postMessage(buffer, [buffer]); + +assert.strictEqual(buffer.byteLength, origByteLength); +assert.notStrictEqual(buffer.byteLength, 0); diff --git a/test/node-api/test_worker_buffer_callback/test_worker_buffer_callback.c b/test/node-api/test_worker_buffer_callback/test_worker_buffer_callback.c new file mode 100644 index 00000000000000..b911fd86380644 --- /dev/null +++ b/test/node-api/test_worker_buffer_callback/test_worker_buffer_callback.c @@ -0,0 +1,43 @@ +#include +#include +#include +#include "../../js-native-api/common.h" + +uint32_t free_call_count = 0; +char data[] = "hello"; + +napi_value GetFreeCallCount(napi_env env, napi_callback_info info) { + napi_value value; + NAPI_CALL(env, napi_create_uint32(env, free_call_count, &value)); + return value; +} + +static void finalize_cb(napi_env env, void* finalize_data, void* hint) { + assert(finalize_data == data); + free_call_count++; +} + +NAPI_MODULE_INIT() { + napi_property_descriptor properties[] = { + DECLARE_NAPI_PROPERTY("getFreeCallCount", GetFreeCallCount) + }; + + NAPI_CALL(env, napi_define_properties( + env, exports, sizeof(properties) / sizeof(*properties), properties)); + + // This is a slight variation on the non-N-API test: We create an ArrayBuffer + // rather than a Node.js Buffer, since testing the latter would only test + // the same code paths and not the ones specific to N-API. + napi_value buffer; + NAPI_CALL(env, napi_create_external_arraybuffer( + env, + data, + sizeof(data), + finalize_cb, + NULL, + &buffer)); + + NAPI_CALL(env, napi_set_named_property(env, exports, "buffer", buffer)); + + return exports; +}