From 850a80b8a7b2fc8e1fcc3956c2860c3042576975 Mon Sep 17 00:00:00 2001 From: Thang Tran Date: Tue, 3 Dec 2019 23:22:08 +0100 Subject: [PATCH] src: migrate to new V8 ArrayBuffer API ArrayBuffer without BackingStore will soon be deprecated. Fixes:https://github.com/nodejs/node/issues/30529 --- src/env-inl.h | 16 ++++-- src/js_native_api_v8.cc | 12 ++++- src/node_buffer.cc | 52 +++++++++++++++---- src/node_http2.cc | 23 ++++++-- src/node_http2.h | 1 + src/node_v8.cc | 49 +++++++++++++---- src/node_worker.cc | 2 + test/addons/worker-buffer-callback/binding.cc | 5 +- .../test_typedarray/test_typedarray.c | 20 +++++-- .../test_worker_buffer_callback.c | 7 ++- 10 files changed, 153 insertions(+), 34 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index d75b4ea743c4f4..91a69ddd8f3a10 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -983,10 +983,20 @@ inline v8::MaybeLocal AllocatedBuffer::ToBuffer() { inline v8::Local AllocatedBuffer::ToArrayBuffer() { CHECK_NOT_NULL(env_); uv_buf_t buf = release(); + auto callback = [](void* data, size_t length, void* deleter_data){ + CHECK_NOT_NULL(deleter_data); + + static_cast(deleter_data) + ->Free(data, length); + }; + std::unique_ptr backing = + v8::ArrayBuffer::NewBackingStore(buf.base, + buf.len, + callback, + env_->isolate() + ->GetArrayBufferAllocator()); return v8::ArrayBuffer::New(env_->isolate(), - buf.base, - buf.len, - v8::ArrayBufferCreationMode::kInternalized); + std::move(backing)); } inline void Environment::ThrowError(const char* errmsg) { diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 5506b2b4c6204b..52e2d7f582ea3e 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -2612,8 +2612,18 @@ napi_status napi_create_external_arraybuffer(napi_env env, CHECK_ARG(env, result); v8::Isolate* isolate = env->isolate; + // The buffer will be freed with v8impl::ArrayBufferReference::New() + // below, hence this BackingStore does not need to free the buffer. + std::unique_ptr backing = + v8::ArrayBuffer::NewBackingStore(external_data, + byte_length, + [](void*, size_t, void*){}, + nullptr); v8::Local buffer = - v8::ArrayBuffer::New(isolate, external_data, byte_length); + v8::ArrayBuffer::New(isolate, std::move(backing)); + // TODO(thangktran): drop this check when V8 is pumped to 8.0 . + if (!buffer->IsExternal()) + buffer->Externalize(buffer->GetBackingStore()); v8::Maybe marked = env->mark_arraybuffer_as_untransferable(buffer); CHECK_MAYBE_NOTHING(env, marked, napi_generic_failure); diff --git a/src/node_buffer.cc b/src/node_buffer.cc index e5c4655b4ccdb9..c04be68d1b0cbe 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -47,8 +47,8 @@ namespace node { namespace Buffer { using v8::ArrayBuffer; -using v8::ArrayBufferCreationMode; using v8::ArrayBufferView; +using v8::BackingStore; using v8::Context; using v8::EscapableHandleScope; using v8::FunctionCallbackInfo; @@ -127,8 +127,8 @@ CallbackInfo::CallbackInfo(Environment* env, data_(data), hint_(hint), env_(env) { - ArrayBuffer::Contents obj_c = object->GetContents(); - CHECK_EQ(data_, static_cast(obj_c.Data())); + std::shared_ptr obj_backing = object->GetBackingStore(); + CHECK_EQ(data_, static_cast(obj_backing->Data())); if (object->ByteLength() != 0) CHECK_NOT_NULL(data_); @@ -406,7 +406,19 @@ MaybeLocal New(Environment* env, return Local(); } - Local ab = ArrayBuffer::New(env->isolate(), data, length); + + // The buffer will be released by a CallbackInfo::New() below, + // hence this BackingStore callback is empty. + std::unique_ptr backing = + ArrayBuffer::NewBackingStore(data, + length, + [](void*, size_t, void*){}, + nullptr); + Local ab = ArrayBuffer::New(env->isolate(), + std::move(backing)); + // TODO(thangktran): drop this check when V8 is pumped to 8.0 . + if (!ab->IsExternal()) + ab->Externalize(ab->GetBackingStore()); if (ab->SetPrivate(env->context(), env->arraybuffer_untransferable_private_symbol(), True(env->isolate())).IsNothing()) { @@ -465,11 +477,21 @@ MaybeLocal New(Environment* env, } } - Local ab = - ArrayBuffer::New(env->isolate(), - data, - length, - ArrayBufferCreationMode::kInternalized); + auto callback = [](void* data, size_t length, void* deleter_data){ + CHECK_NOT_NULL(deleter_data); + + static_cast(deleter_data) + ->Free(data, length); + }; + std::unique_ptr backing = + v8::ArrayBuffer::NewBackingStore(data, + length, + callback, + env->isolate() + ->GetArrayBufferAllocator()); + Local ab = ArrayBuffer::New(env->isolate(), + std::move(backing)); + return Buffer::New(env, ab, 0, length).FromMaybe(Local()); } @@ -1181,8 +1203,16 @@ void Initialize(Local target, if (NodeArrayBufferAllocator* allocator = env->isolate_data()->node_allocator()) { uint32_t* zero_fill_field = allocator->zero_fill_field(); - Local array_buffer = ArrayBuffer::New( - env->isolate(), zero_fill_field, sizeof(*zero_fill_field)); + std::unique_ptr backing = + ArrayBuffer::NewBackingStore(zero_fill_field, + sizeof(*zero_fill_field), + [](void*, size_t, void*){}, + nullptr); + Local array_buffer = + ArrayBuffer::New(env->isolate(), std::move(backing)); + // TODO(thangktran): drop this check when V8 is pumped to 8.0 . + if (!array_buffer->IsExternal()) + array_buffer->Externalize(array_buffer->GetBackingStore()); CHECK(target ->Set(env->context(), FIXED_ONE_BYTE_STRING(env->isolate(), "zeroFill"), diff --git a/src/node_http2.cc b/src/node_http2.cc index 760533b605497b..db3bd035b34444 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -16,6 +16,7 @@ namespace node { using v8::ArrayBuffer; using v8::ArrayBufferView; +using v8::BackingStore; using v8::Boolean; using v8::Context; using v8::Float64Array; @@ -566,10 +567,18 @@ Http2Session::Http2Session(Environment* env, { // Make the js_fields_ property accessible to JS land. + std::unique_ptr backing = + ArrayBuffer::NewBackingStore( + reinterpret_cast(&js_fields_), + kSessionUint8FieldCount, + [](void*, size_t, void*){}, + nullptr); Local ab = - ArrayBuffer::New(env->isolate(), - reinterpret_cast(&js_fields_), - kSessionUint8FieldCount); + ArrayBuffer::New(env->isolate(), std::move(backing)); + // TODO(thangktran): drop this check when V8 is pumped to 8.0 . + if (!ab->IsExternal()) + ab->Externalize(ab->GetBackingStore()); + js_fields_ab_.Reset(env->isolate(), ab); Local uint8_arr = Uint8Array::New(ab, 0, kSessionUint8FieldCount); USE(wrap->Set(env->context(), env->fields_string(), uint8_arr)); @@ -581,6 +590,14 @@ Http2Session::~Http2Session() { Debug(this, "freeing nghttp2 session"); nghttp2_session_del(session_); CHECK_EQ(current_nghttp2_memory_, 0); + HandleScope handle_scope(env()->isolate()); + // Detach js_fields_ab_ to avoid having problem when new Http2Session + // instances are created on the same location of previous + // instances. This in turn will call ArrayBuffer::NewBackingStore() + // multiple times with the same buffer address and causing error. + // Ref: https://github.com/nodejs/node/pull/30782 + Local ab = js_fields_ab_.Get(env()->isolate()); + ab->Detach(); } std::string Http2Session::diagnostic_name() const { diff --git a/src/node_http2.h b/src/node_http2.h index 07febd7da40a9b..264b112fec7649 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -989,6 +989,7 @@ class Http2Session : public AsyncWrap, // JS-accessible numeric fields, as indexed by SessionUint8Fields. SessionJSFields js_fields_ = {}; + v8::Global js_fields_ab_; // The session type: client or server nghttp2_session_type session_type_; diff --git a/src/node_v8.cc b/src/node_v8.cc index ed2e71de1069bb..1f4ef0e35f5cd1 100644 --- a/src/node_v8.cc +++ b/src/node_v8.cc @@ -28,6 +28,7 @@ namespace node { using v8::Array; using v8::ArrayBuffer; +using v8::BackingStore; using v8::Context; using v8::FunctionCallbackInfo; using v8::HeapCodeStatistics; @@ -162,12 +163,22 @@ void Initialize(Local target, const size_t heap_statistics_buffer_byte_length = sizeof(*env->heap_statistics_buffer()) * kHeapStatisticsPropertiesCount; + std::unique_ptr heap_statistics_backing = + ArrayBuffer::NewBackingStore(env->heap_statistics_buffer(), + heap_statistics_buffer_byte_length, + [](void*, size_t, void*){}, + nullptr); + Local heap_statistics_ab = + ArrayBuffer::New(env->isolate(), + std::move(heap_statistics_backing)); + // TODO(thangktran): drop this check when V8 is pumped to 8.0 . + if (!heap_statistics_ab->IsExternal()) + heap_statistics_ab->Externalize( + heap_statistics_ab->GetBackingStore()); target->Set(env->context(), FIXED_ONE_BYTE_STRING(env->isolate(), "heapStatisticsArrayBuffer"), - ArrayBuffer::New(env->isolate(), - env->heap_statistics_buffer(), - heap_statistics_buffer_byte_length)).Check(); + heap_statistics_ab).Check(); #define V(i, _, name) \ target->Set(env->context(), \ @@ -189,12 +200,22 @@ void Initialize(Local target, sizeof(*env->heap_code_statistics_buffer()) * kHeapCodeStatisticsPropertiesCount; + std::unique_ptr heap_code_statistics_backing = + ArrayBuffer::NewBackingStore(env->heap_code_statistics_buffer(), + heap_code_statistics_buffer_byte_length, + [](void*, size_t, void*){}, + nullptr); + Local heap_code_statistics_ab = + ArrayBuffer::New(env->isolate(), + std::move(heap_code_statistics_backing)); + // TODO(thangktran): drop this check when V8 is pumped to 8.0 . + if (!heap_code_statistics_ab->IsExternal()) + heap_code_statistics_ab->Externalize( + heap_code_statistics_ab->GetBackingStore()); target->Set(env->context(), FIXED_ONE_BYTE_STRING(env->isolate(), "heapCodeStatisticsArrayBuffer"), - ArrayBuffer::New(env->isolate(), - env->heap_code_statistics_buffer(), - heap_code_statistics_buffer_byte_length)) + heap_code_statistics_ab) .Check(); #define V(i, _, name) \ @@ -244,12 +265,22 @@ void Initialize(Local target, kHeapSpaceStatisticsPropertiesCount * number_of_heap_spaces; + std::unique_ptr heap_space_statistics_backing = + ArrayBuffer::NewBackingStore(env->heap_space_statistics_buffer(), + heap_space_statistics_buffer_byte_length, + [](void*, size_t, void*){}, + nullptr); + Local heap_space_statistics_ab = + ArrayBuffer::New(env->isolate(), + std::move(heap_space_statistics_backing)); + // TODO(thangktran): drop this check when V8 is pumped to 8.0 . + if (!heap_space_statistics_ab->IsExternal()) + heap_space_statistics_ab->Externalize( + heap_space_statistics_ab->GetBackingStore()); target->Set(env->context(), FIXED_ONE_BYTE_STRING(env->isolate(), "heapSpaceStatisticsArrayBuffer"), - ArrayBuffer::New(env->isolate(), - env->heap_space_statistics_buffer(), - heap_space_statistics_buffer_byte_length)) + heap_space_statistics_ab) .Check(); #define V(i, _, name) \ diff --git a/src/node_worker.cc b/src/node_worker.cc index 9976ec3f0ac089..a7449e44118a59 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -19,6 +19,7 @@ using node::kDisallowedInEnvironment; using v8::Array; using v8::ArrayBuffer; +using v8::BackingStore; using v8::Boolean; using v8::Context; using v8::Float64Array; @@ -622,6 +623,7 @@ void Worker::GetResourceLimits(const FunctionCallbackInfo& args) { Local Worker::GetResourceLimits(Isolate* isolate) const { Local ab = ArrayBuffer::New(isolate, sizeof(resource_limits_)); + memcpy(ab->GetBackingStore()->Data(), resource_limits_, sizeof(resource_limits_)); diff --git a/test/addons/worker-buffer-callback/binding.cc b/test/addons/worker-buffer-callback/binding.cc index 1141c8a051e077..f600f410c1ed18 100644 --- a/test/addons/worker-buffer-callback/binding.cc +++ b/test/addons/worker-buffer-callback/binding.cc @@ -10,7 +10,6 @@ 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); @@ -21,6 +20,9 @@ void Initialize(Local exports, Local context) { Isolate* isolate = context->GetIsolate(); NODE_SET_METHOD(exports, "getFreeCallCount", GetFreeCallCount); + + char* data = new char; + exports->Set(context, v8::String::NewFromUtf8( isolate, "buffer", v8::NewStringType::kNormal) @@ -30,6 +32,7 @@ void Initialize(Local exports, data, sizeof(data), [](char* data, void* hint) { + delete data; free_call_count++; }, nullptr).ToLocalChecked()).Check(); diff --git a/test/js-native-api/test_typedarray/test_typedarray.c b/test/js-native-api/test_typedarray/test_typedarray.c index d8b64d06c0c57b..66b1f9019c04ea 100644 --- a/test/js-native-api/test_typedarray/test_typedarray.c +++ b/test/js-native-api/test_typedarray/test_typedarray.c @@ -1,6 +1,7 @@ #define NAPI_EXPERIMENTAL #include #include +#include #include "../common.h" static napi_value Multiply(napi_env env, napi_callback_info info) { @@ -74,22 +75,33 @@ static napi_value Multiply(napi_env env, napi_callback_info info) { return output_array; } +static void FinalizeCallback(napi_env env, + void* finalize_data, + void* finalize_hint) +{ + free(finalize_data); +} + static napi_value External(napi_env env, napi_callback_info info) { - static int8_t externalData[] = {0, 1, 2}; + const uint8_t nElem = 3; + int8_t* externalData = malloc(nElem*sizeof(int8_t)); + externalData[0] = 0; + externalData[1] = 1; + externalData[2] = 2; napi_value output_buffer; NAPI_CALL(env, napi_create_external_arraybuffer( env, externalData, - sizeof(externalData), - NULL, // finalize_callback + nElem*sizeof(int8_t), + FinalizeCallback, NULL, // finalize_hint &output_buffer)); napi_value output_array; NAPI_CALL(env, napi_create_typedarray(env, napi_int8_array, - sizeof(externalData) / sizeof(int8_t), + nElem, output_buffer, 0, &output_array)); 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 index b911fd86380644..3275551aee526a 100644 --- 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 @@ -1,10 +1,10 @@ #include +#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; @@ -13,7 +13,7 @@ napi_value GetFreeCallCount(napi_env env, napi_callback_info info) { } static void finalize_cb(napi_env env, void* finalize_data, void* hint) { - assert(finalize_data == data); + free(finalize_data); free_call_count++; } @@ -29,6 +29,9 @@ NAPI_MODULE_INIT() { // 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; + + char* data = malloc(sizeof(char)); + NAPI_CALL(env, napi_create_external_arraybuffer( env, data,