Skip to content

Commit

Permalink
worker: handle detached MessagePort from a different context
Browse files Browse the repository at this point in the history
When `worker.moveMessagePortToContext` is used, the async handle
associated with the port, will be triggered more than needed (at least
one more time) and with null data. That can be avoided by simply
checking that the data is present and the port is not detached.

Fixes: #49075
Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com>
PR-URL: #49150
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
  • Loading branch information
juanarbol authored and targos committed Oct 23, 2023
1 parent 1464eba commit 87be790
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 2 deletions.
7 changes: 7 additions & 0 deletions src/node_messaging.cc
Original file line number Diff line number Diff line change
Expand Up @@ -789,6 +789,13 @@ MaybeLocal<Value> MessagePort::ReceiveMessage(Local<Context> context,

void MessagePort::OnMessage(MessageProcessingMode mode) {
Debug(this, "Running MessagePort::OnMessage()");
// Maybe the async handle was triggered empty or more than needed.
// The data_ could be freed or, the handle has been/is being closed.
// A possible case for this, is transfer the MessagePort to another
// context, it will call the constructor and trigger the async handle empty.
// Because all data was sent from the preivous context.
if (IsDetached()) return;

HandleScope handle_scope(env()->isolate());
Local<Context> context =
object(env()->isolate())->GetCreationContext().ToLocalChecked();
Expand Down
30 changes: 28 additions & 2 deletions test/parallel/test-worker-workerdata-messageport.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
'use strict';

require('../common');
const assert = require('assert');
const assert = require('node:assert');

const {
Worker, MessageChannel
} = require('worker_threads');
} = require('node:worker_threads');

const channel = new MessageChannel();
const workerData = { mesage: channel.port1 };
Expand Down Expand Up @@ -61,3 +61,29 @@ const meowScript = () => 'meow';
'listed in transferList'
});
}

{
// Should not crash when MessagePort is transferred to another context.
// https://github.com/nodejs/node/issues/49075
const channel = new MessageChannel();
new Worker(`
const { runInContext, createContext } = require('node:vm')
const { workerData } = require('worker_threads');
const context = createContext(Object.create(null));
context.messagePort = workerData.messagePort;
runInContext(
\`messagePort.postMessage("Meow")\`,
context,
{ displayErrors: true }
);
`, {
eval: true,
workerData: { messagePort: channel.port2 },
transferList: [channel.port2]
});
channel.port1.on(
'message',
(message) =>
assert.strictEqual(message, 'Meow')
);
}

0 comments on commit 87be790

Please sign in to comment.