From 7580ecb7a1e8efff7345f27288e77b794787ea22 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 2 Jun 2019 20:24:22 +0200 Subject: [PATCH 1/4] worker: add typechecking for postMessage transfer list If the transfer list argument is present, it should be an array. This commit adds typechecking to that effect. This aligns behaviour with browsers. --- src/node_messaging.cc | 7 +++++++ test/parallel/test-worker-message-port.js | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/src/node_messaging.cc b/src/node_messaging.cc index fa583a2570315b..64e402868907f4 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -711,6 +711,13 @@ void MessagePort::PostMessage(const FunctionCallbackInfo& args) { return THROW_ERR_MISSING_ARGS(env, "Not enough arguments to " "MessagePort.postMessage"); } + if (!args[1]->IsNullOrUndefined() && !args[1]->IsObject()) { + // Browsers also do not throw on `null` or objects, although it really + // should be an array or undefined, thus the mismatch between the checks + // above and the actual error message. + return THROW_ERR_INVALID_ARG_TYPE(env, + "Transfer list argument must be array or missing"); + } MessagePort* port = Unwrap(args.This()); // Even if the backing MessagePort object has already been deleted, we still diff --git a/test/parallel/test-worker-message-port.js b/test/parallel/test-worker-message-port.js index dce9da0b3063f5..1344379c9ac90d 100644 --- a/test/parallel/test-worker-message-port.js +++ b/test/parallel/test-worker-message-port.js @@ -70,6 +70,26 @@ const { MessageChannel, MessagePort } = require('worker_threads'); }); } +{ + const { port1, port2 } = new MessageChannel(); + port2.on('message', common.mustCall(4)); + port1.postMessage(1, null); + port1.postMessage(2, undefined); + port1.postMessage(3, []); + port1.postMessage(4, {}); + + const err = { + constructor: TypeError, + code: 'ERR_INVALID_ARG_TYPE', + message: 'Transfer list argument must be array or missing' + }; + + assert.throws(() => port1.postMessage(5, 0), err); + assert.throws(() => port1.postMessage(5, false), err); + assert.throws(() => port1.postMessage(5, 'X'), err); + assert.throws(() => port1.postMessage(5, Symbol('X')), err); +} + { assert.deepStrictEqual( Object.getOwnPropertyNames(MessagePort.prototype).sort(), From 5c9f1c69b29a026c89f1fd7ac0d9f64c4e1c185c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 9 Jun 2019 19:33:56 +0200 Subject: [PATCH 2/4] fixup! worker: add typechecking for postMessage transfer list Co-Authored-By: Rich Trott --- src/node_messaging.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_messaging.cc b/src/node_messaging.cc index 64e402868907f4..2395176d335555 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -716,7 +716,7 @@ void MessagePort::PostMessage(const FunctionCallbackInfo& args) { // should be an array or undefined, thus the mismatch between the checks // above and the actual error message. return THROW_ERR_INVALID_ARG_TYPE(env, - "Transfer list argument must be array or missing"); + "Optional transferList argument must be an array"); } MessagePort* port = Unwrap(args.This()); From 8aac5b20816d42a4935fb2f2d145963d5bf48eb9 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 9 Jun 2019 19:38:59 +0200 Subject: [PATCH 3/4] fixup! worker: add typechecking for postMessage transfer list --- src/node_messaging.cc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/node_messaging.cc b/src/node_messaging.cc index 2395176d335555..b705b6ad07108c 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -712,9 +712,11 @@ void MessagePort::PostMessage(const FunctionCallbackInfo& args) { "MessagePort.postMessage"); } if (!args[1]->IsNullOrUndefined() && !args[1]->IsObject()) { - // Browsers also do not throw on `null` or objects, although it really - // should be an array or undefined, thus the mismatch between the checks - // above and the actual error message. + // Browsers ignore null or undefined, and otherwise accept an array or an + // options object. + // TODO(addaleax): Add support for an options object and generic sequence + // support. + // Refs: https://github.com/nodejs/node/pull/28033#discussion_r289964991 return THROW_ERR_INVALID_ARG_TYPE(env, "Optional transferList argument must be an array"); } From ace5dff006f5fc4cc0cabd64a9116b240be55b49 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 9 Jun 2019 19:44:49 +0200 Subject: [PATCH 4/4] fixup! worker: add typechecking for postMessage transfer list --- test/parallel/test-worker-message-port.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-worker-message-port.js b/test/parallel/test-worker-message-port.js index 1344379c9ac90d..d1c58216fd9d00 100644 --- a/test/parallel/test-worker-message-port.js +++ b/test/parallel/test-worker-message-port.js @@ -81,13 +81,14 @@ const { MessageChannel, MessagePort } = require('worker_threads'); const err = { constructor: TypeError, code: 'ERR_INVALID_ARG_TYPE', - message: 'Transfer list argument must be array or missing' + message: 'Optional transferList argument must be an array' }; assert.throws(() => port1.postMessage(5, 0), err); assert.throws(() => port1.postMessage(5, false), err); assert.throws(() => port1.postMessage(5, 'X'), err); assert.throws(() => port1.postMessage(5, Symbol('X')), err); + port1.close(); } {