Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

worker: add typechecking for postMessage transfer list #28033

Closed
wants to merge 4 commits into from
Closed

worker: add typechecking for postMessage transfer list #28033

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7580ecb
worker: add typechecking for postMessage transfer list
addaleax Jun 2, 2019
5c9f1c6
fixup! worker: add typechecking for postMessage transfer list
addaleax Jun 9, 2019
8aac5b2
fixup! worker: add typechecking for postMessage transfer list
addaleax Jun 9, 2019
ace5dff
fixup! worker: add typechecking for postMessage transfer list
addaleax Jun 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/node_messaging.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,13 @@ void MessagePort::PostMessage(const FunctionCallbackInfo<Value>& 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for this is due to the fact that browsers have an overload that accepts an option bag in addition to just a transfer list. See https://html.spec.whatwg.org/multipage/web-messaging.html#messageport. How browsers resolve the overloading is roughly something like this

  1. If second arg is undefined or null then ignore.
  2. If second arg is not an object (i.e., also not a function), then throw.
  3. Try to get the Symbol.iterator property of the second arg.
    1. If that throws, then rethrow the exception.
    2. If that returns anything other than undefined or function, also throw an exception.
    3. If it returns a function, then treat this as an iterable and use the equivalent of Array.from to convert it to a list.
    4. Otherwise (it returns undefined), then treat the second arg as an option bag. Try to get the transferList property and then convert it to a list through something like Array.from if present.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TimothyGu I’m adding a TODO comment in this PR and merge this with the current state, if that’s okay with you

Copy link
Member

@TimothyGu TimothyGu Jun 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sure. You might encounter it as we try to pass more WPTs, so hope it'd be helpful.

return THROW_ERR_INVALID_ARG_TYPE(env,
"Transfer list argument must be array or missing");
addaleax marked this conversation as resolved.
Show resolved Hide resolved
}

MessagePort* port = Unwrap<MessagePort>(args.This());
// Even if the backing MessagePort object has already been deleted, we still
Expand Down
20 changes: 20 additions & 0 deletions test/parallel/test-worker-message-port.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down