From 303a9a3d06bfb5558326e78a93501b706501cd6c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 2 Jun 2019 19:42:22 +0200 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/28032 Reviewed-By: Rich Trott Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: James M Snell --- doc/api/errors.md | 8 +++++ src/node_errors.h | 4 ++- src/node_messaging.cc | 32 +++++++------------ src/node_messaging.h | 4 +-- .../test-worker-message-port-constructor.js | 27 ++++++++++++++++ .../parallel/test-worker-message-port-move.js | 7 ---- 6 files changed, 52 insertions(+), 30 deletions(-) create mode 100644 test/parallel/test-worker-message-port-constructor.js diff --git a/doc/api/errors.md b/doc/api/errors.md index 131773143ceca7..994634c8067c31 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -703,6 +703,14 @@ non-writable `stdout` or `stderr` stream. A constructor for a class was called without `new`. + +### ERR_CONSTRUCT_CALL_INVALID + + +A class constructor was called that is not callable. + ### ERR_CPU_USAGE diff --git a/src/node_errors.h b/src/node_errors.h index 0dad93f31fa33e..689911f9963a55 100644 --- a/src/node_errors.h +++ b/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") \ diff --git a/src/node_messaging.cc b/src/node_messaging.cc index 79fa510ed70ab1..7a0f2db8830978 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -529,16 +529,11 @@ void MessagePort::Close(v8::Local close_callback) { } void MessagePort::New(const FunctionCallbackInfo& 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 = args.This()->CreationContext(); - Context::Scope context_scope(context); - - new MessagePort(env, context, args.This()); + THROW_ERR_CONSTRUCT_CALL_INVALID(env); } MessagePort* MessagePort::New( @@ -546,16 +541,14 @@ MessagePort* MessagePort::New( Local context, std::unique_ptr data) { Context::Scope context_scope(context); - Local ctor; - if (!GetMessagePortConstructor(env, context).ToLocal(&ctor)) - return nullptr; + Local ctor_templ = GetMessagePortConstructorTemplate(env); // Construct a new instance, then assign the listener instance and possibly // the MessagePortData to it. Local instance; - if (!ctor->NewInstance(context).ToLocal(&instance)) + if (!ctor_templ->InstanceTemplate()->NewInstance(context).ToLocal(&instance)) return nullptr; - MessagePort* port = Unwrap(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 GetMessagePortConstructor( - Environment* env, Local context) { +Local 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 templ = env->message_port_constructor_template(); if (!templ.IsEmpty()) - return templ->GetFunction(context); + return templ; Isolate* isolate = env->isolate(); @@ -859,7 +851,7 @@ MaybeLocal GetMessagePortConstructor( env->set_message_event_object_template(e); } - return GetMessagePortConstructor(env, context); + return GetMessagePortConstructorTemplate(env); } namespace { @@ -902,8 +894,8 @@ static void InitMessaging(Local 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. diff --git a/src/node_messaging.h b/src/node_messaging.h index 9f7929aa1c4a05..d9f25a95d76fc9 100644 --- a/src/node_messaging.h +++ b/src/node_messaging.h @@ -211,8 +211,8 @@ class MessagePort : public HandleWrap { friend class MessagePortData; }; -v8::MaybeLocal GetMessagePortConstructor( - Environment* env, v8::Local context); +v8::Local GetMessagePortConstructorTemplate( + Environment* env); } // namespace worker } // namespace node diff --git a/test/parallel/test-worker-message-port-constructor.js b/test/parallel/test-worker-message-port-constructor.js new file mode 100644 index 00000000000000..55bb61ecb3544c --- /dev/null +++ b/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' +}); diff --git a/test/parallel/test-worker-message-port-move.js b/test/parallel/test-worker-message-port-move.js index 65082139776683..7e5d0243fa8b96 100644 --- a/test/parallel/test-worker-message-port-move.js +++ b/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) => {