worker: make MessagePort constructor non-callable

Refactor the C++ code for creating `MessagePort`s to skip calling the
constructor and instead directly instantiating the `InstanceTemplate`,
and always throw an error from the `MessagePort` constructor.

This aligns behaviour with the web, and creating single `MessagePort`s
does not make sense anyway.

PR-URL: #28032
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Author: addaleax

doc/api/errors.md

@@ -703,6 +703,14 @@ non-writable `stdout` or `stderr` stream.
 
 A constructor for a class was called without `new`.
 
+<a id="ERR_CONSTRUCT_CALL_INVALID"></a>
+### ERR_CONSTRUCT_CALL_INVALID
+<!--
+added: REPLACEME
+-->
+
+A class constructor was called that is not callable.
+
 <a id="ERR_CPU_USAGE"></a>
 ### ERR_CPU_USAGE
 

src/node_errors.h

@@ -54,7 +54,8 @@ void FatalException(v8::Isolate* isolate,
   V(ERR_BUFFER_CONTEXT_NOT_AVAILABLE, Error)                                 \
   V(ERR_BUFFER_OUT_OF_BOUNDS, RangeError)                                    \
   V(ERR_BUFFER_TOO_LARGE, Error)                                             \
-  V(ERR_CONSTRUCT_CALL_REQUIRED, Error)                                      \
+  V(ERR_CONSTRUCT_CALL_REQUIRED, TypeError)                                  \
+  V(ERR_CONSTRUCT_CALL_INVALID, TypeError)                                   \
   V(ERR_INVALID_ARG_VALUE, TypeError)                                        \
   V(ERR_INVALID_ARG_TYPE, TypeError)                                         \
   V(ERR_INVALID_MODULE_SPECIFIER, TypeError)                                 \
@@ -99,6 +100,7 @@ void FatalException(v8::Isolate* isolate,
 #define PREDEFINED_ERROR_MESSAGES(V)                                         \
   V(ERR_BUFFER_CONTEXT_NOT_AVAILABLE,                                        \
     "Buffer is not available for the current Context")                       \
+  V(ERR_CONSTRUCT_CALL_INVALID, "Constructor cannot be called")              \
   V(ERR_CONSTRUCT_CALL_REQUIRED, "Cannot call constructor without `new`")    \
   V(ERR_INVALID_TRANSFER_OBJECT, "Found invalid object in transferList")     \
   V(ERR_MEMORY_ALLOCATION_FAILED, "Failed to allocate memory")               \

src/node_messaging.cc

@@ -529,33 +529,26 @@ void MessagePort::Close(v8::Local<v8::Value> close_callback) {
 }
 
 void MessagePort::New(const FunctionCallbackInfo<Value>& args) {
+  // This constructor just throws an error. Unfortunately, we can’t use V8’s
+  // ConstructorBehavior::kThrow, as that also removes the prototype from the
+  // class (i.e. makes it behave like an arrow function).
   Environment* env = Environment::GetCurrent(args);
-  if (!args.IsConstructCall()) {
-    THROW_ERR_CONSTRUCT_CALL_REQUIRED(env);
-    return;
-  }
-
-  Local<Context> context = args.This()->CreationContext();
-  Context::Scope context_scope(context);
-
-  new MessagePort(env, context, args.This());
+  THROW_ERR_CONSTRUCT_CALL_INVALID(env);
 }
 
 MessagePort* MessagePort::New(
     Environment* env,
     Local<Context> context,
     std::unique_ptr<MessagePortData> data) {
   Context::Scope context_scope(context);
-  Local<Function> ctor;
-  if (!GetMessagePortConstructor(env, context).ToLocal(&ctor))
-    return nullptr;
+  Local<FunctionTemplate> ctor_templ = GetMessagePortConstructorTemplate(env);
 
   // Construct a new instance, then assign the listener instance and possibly
   // the MessagePortData to it.
   Local<Object> instance;
-  if (!ctor->NewInstance(context).ToLocal(&instance))
+  if (!ctor_templ->InstanceTemplate()->NewInstance(context).ToLocal(&instance))
     return nullptr;
-  MessagePort* port = Unwrap<MessagePort>(instance);
+  MessagePort* port = new MessagePort(env, context, instance);
   CHECK_NOT_NULL(port);
   if (data) {
     port->Detach();
@@ -830,13 +823,12 @@ void MessagePort::Entangle(MessagePort* a, MessagePortData* b) {
   MessagePortData::Entangle(a->data_.get(), b);
 }
 
-MaybeLocal<Function> GetMessagePortConstructor(
-    Environment* env, Local<Context> context) {
+Local<FunctionTemplate> GetMessagePortConstructorTemplate(Environment* env) {
   // Factor generating the MessagePort JS constructor into its own piece
   // of code, because it is needed early on in the child environment setup.
   Local<FunctionTemplate> templ = env->message_port_constructor_template();
   if (!templ.IsEmpty())
-    return templ->GetFunction(context);
+    return templ;
 
   Isolate* isolate = env->isolate();
 
@@ -859,7 +851,7 @@ MaybeLocal<Function> GetMessagePortConstructor(
     env->set_message_event_object_template(e);
   }
 
-  return GetMessagePortConstructor(env, context);
+  return GetMessagePortConstructorTemplate(env);
 }
 
 namespace {
@@ -902,8 +894,8 @@ static void InitMessaging(Local<Object> target,
 
   target->Set(context,
               env->message_port_constructor_string(),
-              GetMessagePortConstructor(env, context).ToLocalChecked())
-                  .Check();
+              GetMessagePortConstructorTemplate(env)
+                  ->GetFunction(context).ToLocalChecked()).Check();
 
   // These are not methods on the MessagePort prototype, because
   // the browser equivalents do not provide them.

src/node_messaging.h

@@ -211,8 +211,8 @@ class MessagePort : public HandleWrap {
   friend class MessagePortData;
 };
 
-v8::MaybeLocal<v8::Function> GetMessagePortConstructor(
-    Environment* env, v8::Local<v8::Context> context);
+v8::Local<v8::FunctionTemplate> GetMessagePortConstructorTemplate(
+    Environment* env);
 
 }  // namespace worker
 }  // namespace node

test/parallel/test-worker-message-port-constructor.js

@@ -0,0 +1,27 @@
+'use strict';
+require('../common');
+const assert = require('assert');
+
+const { MessageChannel, MessagePort } = require('worker_threads');
+
+// Make sure that `MessagePort` is the constructor for MessagePort instances,
+// but not callable.
+const { port1 } = new MessageChannel();
+
+assert(port1 instanceof MessagePort);
+assert.strictEqual(port1.constructor, MessagePort);
+
+assert.throws(() => MessagePort(), {
+  constructor: TypeError,
+  code: 'ERR_CONSTRUCT_CALL_INVALID'
+});
+
+assert.throws(() => new MessagePort(), {
+  constructor: TypeError,
+  code: 'ERR_CONSTRUCT_CALL_INVALID'
+});
+
+assert.throws(() => MessageChannel(), {
+  constructor: TypeError,
+  code: 'ERR_CONSTRUCT_CALL_REQUIRED'
+});

test/parallel/test-worker-message-port-move.js

@@ -53,13 +53,6 @@ vm.runInContext('(' + function() {
     }
     assert(threw);
   }
-
-  {
-    const newDummyPort = new (port.constructor)();
-    assert(!(newDummyPort instanceof MessagePort));
-    assert(newDummyPort.close instanceof Function);
-    newDummyPort.close();
-  }
 } + ')()', context);
 
 port2.on('message', common.mustCall((msg) => {