src: mark ArrayBuffers with free callbacks as untransferable

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

@@ -2581,6 +2581,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

@@ -350,10 +350,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));
 }
 
 
@@ -371,6 +369,12 @@ 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);

src/node_messaging.cc

@@ -306,11 +306,17 @@ Maybe<bool> Message::Serialize(Environment* env,
       // raw data *and* an Isolate with a non-default ArrayBuffer allocator
       // is always going to outlive any Workers it creates, and so will its
       // allocator along with it.
-      // TODO(addaleax): Eventually remove the IsExternal() condition,
-      // see https://github.com/nodejs/node/pull/30339#issuecomment-552225353
+      if (!ab->IsDetachable()) continue;
+      // See https://github.com/nodejs/node/pull/30339#issuecomment-552225353
       // for details.
-      if (!ab->IsDetachable() || ab->IsExternal())
-        continue;
+      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,29 @@
+#include <node.h>
+#include <node_buffer.h>
+#include <v8.h>
+
+using v8::Context;
+using v8::Isolate;
+using v8::Local;
+using v8::Object;
+using v8::Value;
+
+char data[] = "hello";
+
+void Initialize(Local<Object> exports,
+                Local<Value> module,
+                Local<Context> context) {
+  Isolate* isolate = context->GetIsolate();
+  exports->Set(context,
+               v8::String::NewFromUtf8(
+                   isolate, "buffer", v8::NewStringType::kNormal)
+                   .ToLocalChecked(),
+               node::Buffer::New(
+                   isolate,
+                   data,
+                   sizeof(data),
+                   [](char* data, void* hint) {},
+                   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.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);