buffer,n-api: properly handle Environment cleanup

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<v8::Value> 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 <typename... Args>
+  explicit ArrayBufferReference(napi_env env,
+                                v8::Local<v8::ArrayBuffer> value,
+                                Args&&... args)
+    : Reference(env, value, std::forward<Args>(args)...) {}
+
+  template <typename... Args>
+  static ArrayBufferReference* New(napi_env env,
+                                   v8::Local<v8::ArrayBuffer> value,
+                                   Args&&... args) {
+    return new ArrayBufferReference(env, value, std::forward<Args>(args)...);
+  }
+
+ private:
+  void Finalize(bool is_env_teardown) override {
+    if (is_env_teardown) {
+      v8::HandleScope handle_scope(_env->isolate);
+      v8::Local<v8::Value> ab = Get();
+      CHECK(!ab.IsEmpty());
+      CHECK(ab->IsArrayBuffer());
+      ab.As<v8::ArrayBuffer>()->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,

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<ArrayBuffer> 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<CallbackInfo>&);
   inline void WeakCallback(Isolate* isolate);
-  inline CallbackInfo(Isolate* isolate,
+  inline CallbackInfo(Environment* env,
                       Local<ArrayBuffer> 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<ArrayBuffer> 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<ArrayBuffer> 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<char*>(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<CallbackInfo*>(data);
+
+  {
+    HandleScope handle_scope(self->env_->isolate());
+    Local<ArrayBuffer> ab = self->persistent_.Get(self->env_->isolate());
+    CHECK(!ab.IsEmpty());
+    ab->Detach();
+  }
+
+  self->WeakCallback(self->env_->isolate());
 }
 
 
@@ -388,7 +415,7 @@ MaybeLocal<Object> New(Environment* env,
   }
   MaybeLocal<Uint8Array> 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<Object>();

test/addons/worker-buffer-callback/binding.cc

@@ -3,17 +3,24 @@
 #include <v8.h>
 
 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<Value>& args) {
+  args.GetReturnValue().Set(free_call_count);
+}
+
 void Initialize(Local<Object> exports,
                 Local<Value> module,
                 Local<Context> 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<Object> exports,
                    isolate,
                    data,
                    sizeof(data),
-                   [](char* data, void* hint) {},
+                   [](char* data, void* hint) {
+                     free_call_count++;
+                   },
                    nullptr).ToLocalChecked()).Check();
 }
 

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);
+}));

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<v8::ArrayBuffer> ab;
+  {
+    Env env {handle_scope, argv};
+    v8::Local<v8::Object> buf_obj = node::Buffer::New(
+        isolate_,
+        hello,
+        sizeof(hello),
+        [](char* data, void* hint) {
+          CHECK_EQ(data, hello);
+          ++*static_cast<int*>(hint);
+        },
+        &callback_calls).ToLocalChecked();
+    CHECK(buf_obj->IsUint8Array());
+    ab = buf_obj.As<v8::Uint8Array>()->Buffer();
+    CHECK_EQ(ab->ByteLength(), sizeof(hello));
+  }
+
+  CHECK_EQ(callback_calls, 1);
+  CHECK_EQ(ab->ByteLength(), 0);
+}

test/node-api/test_worker_buffer_callback/binding.gyp

@@ -0,0 +1,8 @@
+{
+  'targets': [
+    {
+      'target_name': 'binding',
+      'sources': [ 'test_worker_buffer_callback.c' ]
+    }
+  ]
+}

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);
+}));

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);

test/node-api/test_worker_buffer_callback/test_worker_buffer_callback.c

@@ -0,0 +1,43 @@
+#include <stdio.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;
+  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;
+}