src: mark ArrayBuffers with free callbacks as untransferable

More precisely, make them untransferable if they were created through
*our* APIs, because those do not follow the improved free callback
mechanism that V8 uses now. All other ArrayBuffers can be transferred
between threads now, the assumption being that they were created in a
clean way that follows the V8 API on this.

This addresses a TODO comment.

Refs: #30339 (comment)

PR-URL: #30475
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>

Author: addaleax

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

@@ -305,11 +305,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);