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: https://github.com/nodejs/node/pull/30339#issuecomment-552225353

PR-URL: https://github.com/nodejs/node/pull/30475
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>

6 files changed, 33 insertions, 8 deletions

diff --git a/src/env.h b/src/env.h
index 2df49a24a1..495d92471a 100644
--- a/src/env.h
+++ b/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")               \
diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc
index ef1edb92ee..6484afaaac 100644
--- a/src/js_native_api_v8.cc
+++ b/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
diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h
index 2e0a7a1d6a..534b09851f 100644
--- a/src/js_native_api_v8.h
+++ b/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) {
diff --git a/src/node_api.cc b/src/node_api.cc
index 95664e9c7a..b293272b91 100644
--- a/src/node_api.cc
+++ b/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;
diff --git a/src/node_buffer.cc b/src/node_buffer.cc
index 2dbdd61923..3cd047c0e7 100644
--- a/src/node_buffer.cc
+++ b/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);
diff --git a/src/node_messaging.cc b/src/node_messaging.cc
index c54958a4cf..1028da69b4 100644
--- a/src/node_messaging.cc
+++ b/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(