src: migrate to new V8 ArrayBuffer API

ArrayBuffer without BackingStore will soon be deprecated. Fixes:#30529 PR-URL: #30782 Fixes: #30529 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs · Dec 12, 2019 · 4f523c2 · 4f523c2
1 parent 2dff8dd
commit 4f523c2
Show file tree

Hide file tree

Showing 10 changed files with 153 additions and 34 deletions.
diff --git a/src/env-inl.h b/src/env-inl.h
@@ -983,10 +983,20 @@ inline v8::MaybeLocal<v8::Object> AllocatedBuffer::ToBuffer() {
 inline v8::Local<v8::ArrayBuffer> 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<v8::ArrayBuffer::Allocator*>(deleter_data)
+        ->Free(data, length);
+  };
+  std::unique_ptr<v8::BackingStore> 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
@@ -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<v8::BackingStore> backing =
+      v8::ArrayBuffer::NewBackingStore(external_data,
+                                       byte_length,
+                                       [](void*, size_t, void*){},
+                                       nullptr);
   v8::Local<v8::ArrayBuffer> 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<bool> 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
@@ -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<char*>(obj_c.Data()));
+  std::shared_ptr<BackingStore> obj_backing = object->GetBackingStore();
+  CHECK_EQ(data_, static_cast<char*>(obj_backing->Data()));
   if (object->ByteLength() != 0)
     CHECK_NOT_NULL(data_);
 
@@ -406,7 +406,19 @@ MaybeLocal<Object> New(Environment* env,
     return Local<Object>();
   }
 
-  Local<ArrayBuffer> 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<BackingStore> backing =
+      ArrayBuffer::NewBackingStore(data,
+                                   length,
+                                   [](void*, size_t, void*){},
+                                   nullptr);
+  Local<ArrayBuffer> 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<Object> New(Environment* env,
     }
   }
 
-  Local<ArrayBuffer> 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<v8::ArrayBuffer::Allocator*>(deleter_data)
+        ->Free(data, length);
+  };
+  std::unique_ptr<v8::BackingStore> backing =
+      v8::ArrayBuffer::NewBackingStore(data,
+                                       length,
+                                       callback,
+                                       env->isolate()
+                                          ->GetArrayBufferAllocator());
+  Local<ArrayBuffer> ab = ArrayBuffer::New(env->isolate(),
+                                           std::move(backing));
+
   return Buffer::New(env, ab, 0, length).FromMaybe(Local<Object>());
 }
 
@@ -1181,8 +1203,16 @@ void Initialize(Local<Object> target,
   if (NodeArrayBufferAllocator* allocator =
           env->isolate_data()->node_allocator()) {
     uint32_t* zero_fill_field = allocator->zero_fill_field();
-    Local<ArrayBuffer> array_buffer = ArrayBuffer::New(
-        env->isolate(), zero_fill_field, sizeof(*zero_fill_field));
+    std::unique_ptr<BackingStore> backing =
+      ArrayBuffer::NewBackingStore(zero_fill_field,
+                                   sizeof(*zero_fill_field),
+                                   [](void*, size_t, void*){},
+                                   nullptr);
+    Local<ArrayBuffer> 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
@@ -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<BackingStore> backing =
+        ArrayBuffer::NewBackingStore(
+            reinterpret_cast<uint8_t*>(&js_fields_),
+            kSessionUint8FieldCount,
+            [](void*, size_t, void*){},
+            nullptr);
     Local<ArrayBuffer> ab =
-        ArrayBuffer::New(env->isolate(),
-                         reinterpret_cast<uint8_t*>(&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<Uint8Array> 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<ArrayBuffer> 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
@@ -989,6 +989,7 @@ class Http2Session : public AsyncWrap,
 
   // JS-accessible numeric fields, as indexed by SessionUint8Fields.
   SessionJSFields js_fields_ = {};
+  v8::Global<v8::ArrayBuffer> 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
@@ -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<Object> target,
   const size_t heap_statistics_buffer_byte_length =
       sizeof(*env->heap_statistics_buffer()) * kHeapStatisticsPropertiesCount;
 
+  std::unique_ptr<BackingStore> heap_statistics_backing =
+      ArrayBuffer::NewBackingStore(env->heap_statistics_buffer(),
+                                   heap_statistics_buffer_byte_length,
+                                   [](void*, size_t, void*){},
+                                   nullptr);
+  Local<ArrayBuffer> 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<Object> target,
       sizeof(*env->heap_code_statistics_buffer())
       * kHeapCodeStatisticsPropertiesCount;
 
+  std::unique_ptr<BackingStore> heap_code_statistics_backing =
+      ArrayBuffer::NewBackingStore(env->heap_code_statistics_buffer(),
+                                   heap_code_statistics_buffer_byte_length,
+                                   [](void*, size_t, void*){},
+                                   nullptr);
+  Local<ArrayBuffer> 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<Object> target,
       kHeapSpaceStatisticsPropertiesCount *
       number_of_heap_spaces;
 
+  std::unique_ptr<BackingStore> heap_space_statistics_backing =
+      ArrayBuffer::NewBackingStore(env->heap_space_statistics_buffer(),
+                                   heap_space_statistics_buffer_byte_length,
+                                   [](void*, size_t, void*){},
+                                   nullptr);
+  Local<ArrayBuffer> 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
@@ -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<Value>& args) {
 
 Local<Float64Array> Worker::GetResourceLimits(Isolate* isolate) const {
   Local<ArrayBuffer> 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
@@ -10,7 +10,6 @@ using v8::Object;
 using v8::Value;
 
 uint32_t free_call_count = 0;
-char data[] = "hello";
 
 void GetFreeCallCount(const FunctionCallbackInfo<Value>& args) {
   args.GetReturnValue().Set(free_call_count);
@@ -21,6 +20,9 @@ void Initialize(Local<Object> exports,
                 Local<Context> 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<Object> 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
@@ -1,6 +1,7 @@
 #define NAPI_EXPERIMENTAL
 #include <js_native_api.h>
 #include <string.h>
+#include <stdlib.h>
 #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
@@ -1,10 +1,10 @@
 #include <stdio.h>
+#include <stdlib.h>
 #include <node_api.h>
 #include <assert.h>
 #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,