[v12.x] backport Buffer cleanup changes

src/env.h

@@ -151,6 +151,7 @@ constexpr size_t kFsStatsBufferLength =
 // "node:" prefix to avoid name clashes with third-party code.
 #define PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V)                              \
   V(alpn_buffer_private_symbol, "node:alpnBuffer")                            \
+  V(arraybuffer_untransferable_private_symbol, "node:untransferableBuffer")   \
   V(arrow_message_private_symbol, "node:arrowMessage")                        \
   V(contextify_context_private_symbol, "node:contextify:context")             \
   V(contextify_global_private_symbol, "node:contextify:global")               \

src/js_native_api_v8.cc

@@ -2614,6 +2614,8 @@ napi_status napi_create_external_arraybuffer(napi_env env,
   v8::Isolate* isolate = env->isolate;
   v8::Local<v8::ArrayBuffer> buffer =
       v8::ArrayBuffer::New(isolate, external_data, byte_length);
+  v8::Maybe<bool> marked = env->mark_arraybuffer_as_untransferable(buffer);
+  CHECK_MAYBE_NOTHING(env, marked, napi_generic_failure);
 
   if (finalize_cb != nullptr) {
     // Create a self-deleting weak reference that invokes the finalizer

src/js_native_api_v8.h

@@ -39,6 +39,10 @@ struct napi_env__ {
   inline void Unref() { if ( --refs == 0) delete this; }
 
   virtual bool can_call_into_js() const { return true; }
+  virtual v8::Maybe<bool> mark_arraybuffer_as_untransferable(
+      v8::Local<v8::ArrayBuffer> ab) const {
+    return v8::Just(true);
+  }
 
   template <typename T, typename U>
   void CallIntoModule(T&& call, U&& handle_exception) {

src/node_api.cc

@@ -24,6 +24,14 @@ struct node_napi_env__ : public napi_env__ {
   bool can_call_into_js() const override {
     return node_env()->can_call_into_js();
   }
+
+  v8::Maybe<bool> mark_arraybuffer_as_untransferable(
+      v8::Local<v8::ArrayBuffer> ab) const {
+    return ab->SetPrivate(
+        context(),
+        node_env()->arraybuffer_untransferable_private_symbol(),
+        v8::True(isolate));
+  }
 };
 
 typedef node_napi_env__* node_napi_env;

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());
 }
 
 
@@ -364,10 +391,8 @@ MaybeLocal<Object> New(Isolate* isolate,
     THROW_ERR_BUFFER_CONTEXT_NOT_AVAILABLE(isolate);
     return MaybeLocal<Object>();
   }
-  Local<Object> obj;
-  if (Buffer::New(env, data, length, callback, hint).ToLocal(&obj))
-    return handle_scope.Escape(obj);
-  return Local<Object>();
+  return handle_scope.EscapeMaybe(
+      Buffer::New(env, data, length, callback, hint));
 }
 
 
@@ -385,9 +410,15 @@ MaybeLocal<Object> New(Environment* env,
   }
 
   Local<ArrayBuffer> ab = ArrayBuffer::New(env->isolate(), data, length);
+  if (ab->SetPrivate(env->context(),
+                     env->arraybuffer_untransferable_private_symbol(),
+                     True(env->isolate())).IsNothing()) {
+    callback(data, hint);
+    return Local<Object>();
+  }
   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>();

src/node_messaging.cc

@@ -329,6 +329,16 @@ Maybe<bool> Message::Serialize(Environment* env,
           !env->isolate_data()->uses_node_allocator()) {
         continue;
       }
+      // See https://github.com/nodejs/node/pull/30339#issuecomment-552225353
+      // for details.
+      bool untransferrable;
+      if (!ab->HasPrivate(
+              context,
+              env->arraybuffer_untransferable_private_symbol())
+              .To(&untransferrable)) {
+        return Nothing<bool>();
+      }
+      if (untransferrable) continue;
       if (std::find(array_buffers.begin(), array_buffers.end(), ab) !=
           array_buffers.end()) {
         ThrowDataCloneException(

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

@@ -0,0 +1,38 @@
+#include <node.h>
+#include <node_buffer.h>
+#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)
+                   .ToLocalChecked(),
+               node::Buffer::New(
+                   isolate,
+                   data,
+                   sizeof(data),
+                   [](char* data, void* hint) {
+                     free_call_count++;
+                   },
+                   nullptr).ToLocalChecked()).Check();
+}
+
+NODE_MODULE_CONTEXT_AWARE(NODE_GYP_MODULE_NAME, Initialize)

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

@@ -0,0 +1,9 @@
+{
+  'targets': [
+    {
+      'target_name': 'binding',
+      'sources': [ 'binding.cc' ],
+      'includes': ['../common.gypi'],
+    }
+  ]
+}

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/addons/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
+// transfered.
+
+const { port1 } = new MessageChannel();
+const origByteLength = buffer.byteLength;
+port1.postMessage(buffer, [buffer.buffer]);
+
+assert.strictEqual(buffer.byteLength, origByteLength);
+assert.notStrictEqual(buffer.byteLength, 0);

test/cctest/test_environment.cc

@@ -1,3 +1,4 @@
+#include "node_buffer.h"
 #include "node_internals.h"
 #include "libplatform/libplatform.h"
 
@@ -185,3 +186,34 @@ static void at_exit_js(void* arg) {
   assert(obj->IsObject());
   called_at_exit_js = true;
 }
+
+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);
+}