From 11b506001eb3370f8a33a1c129e36692c73235b1 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 23 Sep 2018 19:10:54 +0200 Subject: [PATCH 1/6] worker: hide MessagePort init function behind symbol This reduces unintended exposure of internals. --- lib/internal/worker.js | 5 +++-- src/env.h | 2 +- src/node_messaging.cc | 2 +- src/node_worker.cc | 4 ++++ 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/internal/worker.js b/lib/internal/worker.js index 4f797dd9e0094c..b5dab224211b13 100644 --- a/lib/internal/worker.js +++ b/lib/internal/worker.js @@ -24,7 +24,8 @@ util.inherits(MessagePort, EventEmitter); const { Worker: WorkerImpl, getEnvMessagePort, - threadId + threadId, + oninit: oninit_symbol } = internalBinding('worker'); const isMainThread = threadId === 0; @@ -93,7 +94,7 @@ function oninit() { setupPortReferencing(this, this, 'message'); } -Object.defineProperty(MessagePort.prototype, 'oninit', { +Object.defineProperty(MessagePort.prototype, oninit_symbol, { enumerable: true, writable: false, value: oninit diff --git a/src/env.h b/src/env.h index aafb3f6e0c7988..491ba396ab8376 100644 --- a/src/env.h +++ b/src/env.h @@ -113,6 +113,7 @@ struct PackageConfig { #define PER_ISOLATE_SYMBOL_PROPERTIES(V) \ V(handle_onclose_symbol, "handle_onclose") \ V(owner_symbol, "owner") \ + V(oninit_symbol, "oninit") \ // Strings are per-isolate primitives but Environment proxies them // for the sake of convenience. Strings should be ASCII-only. @@ -219,7 +220,6 @@ struct PackageConfig { V(onhandshakedone_string, "onhandshakedone") \ V(onhandshakestart_string, "onhandshakestart") \ V(onheaders_string, "onheaders") \ - V(oninit_string, "oninit") \ V(onmessage_string, "onmessage") \ V(onnewsession_string, "onnewsession") \ V(onocspresponse_string, "onocspresponse") \ diff --git a/src/node_messaging.cc b/src/node_messaging.cc index 20e0c7673b8fa9..e7b44823ecd773 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -421,7 +421,7 @@ MessagePort::MessagePort(Environment* env, async()->data = static_cast(this); Local fn; - if (!wrap->Get(context, env->oninit_string()).ToLocal(&fn)) + if (!wrap->Get(context, env->oninit_symbol()).ToLocal(&fn)) return; if (fn->IsFunction()) { diff --git a/src/node_worker.cc b/src/node_worker.cc index 675c3e03a056e6..14651dd5f4d4ac 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -502,6 +502,10 @@ void InitWorker(Local target, thread_id_string, Number::New(env->isolate(), static_cast(env->thread_id()))).FromJust(); + Local oninit_string = FIXED_ONE_BYTE_STRING(env->isolate(), "oninit"); + target->Set(env->context(), + oninit_string, + env->oninit_symbol()).FromJust(); } } // anonymous namespace From b327b9b5bcbc9c03582365747b75ffda70d07cdf Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 23 Sep 2018 19:14:57 +0200 Subject: [PATCH 2/6] worker: reduce `MessagePort` prototype to documented API `MessagePort` is special because it has to be a C++ API that is exposed to userland. Therefore, there is a number of internal methods on its native prototype; this commit reduces this set of methods to only what is documented in the API. --- lib/internal/worker.js | 36 +++++++++++++++-------- test/parallel/test-heapdump-worker.js | 3 +- test/parallel/test-worker-message-port.js | 9 ++++++ 3 files changed, 33 insertions(+), 15 deletions(-) diff --git a/lib/internal/worker.js b/lib/internal/worker.js index b5dab224211b13..91398686ce2abf 100644 --- a/lib/internal/worker.js +++ b/lib/internal/worker.js @@ -19,8 +19,6 @@ const { clearAsyncIdStack } = require('internal/async_hooks'); const { serializeError, deserializeError } = require('internal/error-serdes'); const { pathToFileURL } = require('url'); -util.inherits(MessagePort, EventEmitter); - const { Worker: WorkerImpl, getEnvMessagePort, @@ -58,6 +56,22 @@ const messageTypes = { LOAD_SCRIPT: 'loadScript' }; +// We have to mess with the MessagePort prototype a bit, so that a) we can make +// it inherit from EventEmitter, even though it is a C++ class, and b) we do +// not provide methods that are not present in the Browser and not documented +// on our side (e.g. hasRef). +// Save a copy of the original set of methods as a shallow clone. +const MessagePortPrototype = Object.create( + Object.getPrototypeOf(MessagePort.prototype), + Object.getOwnPropertyDescriptors(MessagePort.prototype)); +// Set up the new inheritance chain. +Object.setPrototypeOf(MessagePort, EventEmitter); +Object.setPrototypeOf(MessagePort.prototype, EventEmitter.prototype); +// Finally, purge methods we don't want to be public. +delete MessagePort.prototype.stop; +delete MessagePort.prototype.drain; +delete MessagePort.prototype.hasRef; + // A communication channel consisting of a handle (that wraps around an // uv_async_t) which can receive information from other threads and emits // .onmessage events, and a function used for sending data to a MessagePort @@ -81,10 +95,10 @@ Object.defineProperty(MessagePort.prototype, 'onmessage', { this[kOnMessageListener] = value; if (typeof value === 'function') { this.ref(); - this.start(); + MessagePortPrototype.start.call(this); } else { this.unref(); - this.stop(); + MessagePortPrototype.stop.call(this); } } }); @@ -117,16 +131,12 @@ Object.defineProperty(MessagePort.prototype, handle_onclose, { value: onclose }); -const originalClose = MessagePort.prototype.close; MessagePort.prototype.close = function(cb) { if (typeof cb === 'function') this.once('close', cb); - originalClose.call(this); + MessagePortPrototype.close.call(this); }; -const drainMessagePort = MessagePort.prototype.drain; -delete MessagePort.prototype.drain; - Object.defineProperty(MessagePort.prototype, util.inspect.custom, { enumerable: false, writable: false, @@ -135,7 +145,7 @@ Object.defineProperty(MessagePort.prototype, util.inspect.custom, { try { // This may throw when `this` does not refer to a native object, // e.g. when accessing the prototype directly. - ref = this.hasRef(); + ref = MessagePortPrototype.hasRef.call(this); } catch { return this; } return Object.assign(Object.create(MessagePort.prototype), ref === undefined ? { @@ -157,12 +167,12 @@ function setupPortReferencing(port, eventEmitter, eventName) { eventEmitter.on('newListener', (name) => { if (name === eventName && eventEmitter.listenerCount(eventName) === 0) { port.ref(); - port.start(); + MessagePortPrototype.start.call(port); } }); eventEmitter.on('removeListener', (name) => { if (name === eventName && eventEmitter.listenerCount(eventName) === 0) { - port.stop(); + MessagePortPrototype.stop.call(port); port.unref(); } }); @@ -304,7 +314,7 @@ class Worker extends EventEmitter { [kOnExit](code) { debug(`[${threadId}] hears end event for Worker ${this.threadId}`); - drainMessagePort.call(this[kPublicPort]); + MessagePortPrototype.drain.call(this[kPublicPort]); this[kDispose](); this.emit('exit', code); this.removeAllListeners(); diff --git a/test/parallel/test-heapdump-worker.js b/test/parallel/test-heapdump-worker.js index b7b5c2f756717c..44a50dd66b5fca 100644 --- a/test/parallel/test-heapdump-worker.js +++ b/test/parallel/test-heapdump-worker.js @@ -19,8 +19,7 @@ validateSnapshotNodes('Worker', [ validateSnapshotNodes('MessagePort', [ { children: [ - { name: 'MessagePortData' }, - { name: 'MessagePort' } + { name: 'MessagePortData' } ] } ], { loose: true }); diff --git a/test/parallel/test-worker-message-port.js b/test/parallel/test-worker-message-port.js index 2d321611ec7758..349cff8c40ef8d 100644 --- a/test/parallel/test-worker-message-port.js +++ b/test/parallel/test-worker-message-port.js @@ -69,3 +69,12 @@ const { MessageChannel, MessagePort } = require('worker_threads'); }); }); } + +{ + assert.deepStrictEqual( + Object.getOwnPropertyNames(MessagePort.prototype).sort(), + [ + 'ref', 'unref', 'start', 'close', 'postMessage', 'onmessage', + 'constructor' + ].sort()); +} From 7551a047dc1beaa80bd64e4779088e7b2582ae95 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 23 Sep 2018 20:00:30 +0200 Subject: [PATCH 3/6] fixup! worker: reduce `MessagePort` prototype to documented API --- lib/internal/worker.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/internal/worker.js b/lib/internal/worker.js index 91398686ce2abf..950c7e6ba8be33 100644 --- a/lib/internal/worker.js +++ b/lib/internal/worker.js @@ -71,6 +71,7 @@ Object.setPrototypeOf(MessagePort.prototype, EventEmitter.prototype); delete MessagePort.prototype.stop; delete MessagePort.prototype.drain; delete MessagePort.prototype.hasRef; +delete MessagePort.prototype.getAsyncId; // A communication channel consisting of a handle (that wraps around an // uv_async_t) which can receive information from other threads and emits From fb840e7b64c5f44f70fa0f8bf485c9ca49618ca5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 23 Sep 2018 20:15:31 +0200 Subject: [PATCH 4/6] fixup! worker: reduce `MessagePort` prototype to documented API --- lib/internal/worker.js | 10 ++++++---- src/node_worker.cc | 4 ---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/lib/internal/worker.js b/lib/internal/worker.js index 950c7e6ba8be33..3409b72bfaa7a9 100644 --- a/lib/internal/worker.js +++ b/lib/internal/worker.js @@ -14,7 +14,10 @@ const { const { internalBinding } = require('internal/bootstrap/loaders'); const { MessagePort, MessageChannel } = internalBinding('messaging'); -const { handle_onclose } = internalBinding('symbols'); +const { + handle_onclose: handleOnCloseSymbol, + oninit: onInitSymbol +} = internalBinding('symbols'); const { clearAsyncIdStack } = require('internal/async_hooks'); const { serializeError, deserializeError } = require('internal/error-serdes'); const { pathToFileURL } = require('url'); @@ -23,7 +26,6 @@ const { Worker: WorkerImpl, getEnvMessagePort, threadId, - oninit: oninit_symbol } = internalBinding('worker'); const isMainThread = threadId === 0; @@ -109,7 +111,7 @@ function oninit() { setupPortReferencing(this, this, 'message'); } -Object.defineProperty(MessagePort.prototype, oninit_symbol, { +Object.defineProperty(MessagePort.prototype, onInitSymbol, { enumerable: true, writable: false, value: oninit @@ -126,7 +128,7 @@ function onclose() { this.emit('close'); } -Object.defineProperty(MessagePort.prototype, handle_onclose, { +Object.defineProperty(MessagePort.prototype, handleOnCloseSymbol, { enumerable: false, writable: false, value: onclose diff --git a/src/node_worker.cc b/src/node_worker.cc index 14651dd5f4d4ac..675c3e03a056e6 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -502,10 +502,6 @@ void InitWorker(Local target, thread_id_string, Number::New(env->isolate(), static_cast(env->thread_id()))).FromJust(); - Local oninit_string = FIXED_ONE_BYTE_STRING(env->isolate(), "oninit"); - target->Set(env->context(), - oninit_string, - env->oninit_symbol()).FromJust(); } } // anonymous namespace From 255db77abd34da33dce78a2dd59855e01c5950cb Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 23 Sep 2018 21:12:02 +0200 Subject: [PATCH 5/6] fixup! fixup! worker: reduce `MessagePort` prototype to documented API --- lib/internal/worker.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/worker.js b/lib/internal/worker.js index 3409b72bfaa7a9..ceec8469b19a9a 100644 --- a/lib/internal/worker.js +++ b/lib/internal/worker.js @@ -25,7 +25,7 @@ const { pathToFileURL } = require('url'); const { Worker: WorkerImpl, getEnvMessagePort, - threadId, + threadId } = internalBinding('worker'); const isMainThread = threadId === 0; From e2e2d67493d006c6e2fbfbab3788437639b3f587 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 23 Sep 2018 22:38:33 +0200 Subject: [PATCH 6/6] fixup! worker: reduce `MessagePort` prototype to documented API --- test/parallel/test-worker-message-port.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-worker-message-port.js b/test/parallel/test-worker-message-port.js index 349cff8c40ef8d..995868e9a770e0 100644 --- a/test/parallel/test-worker-message-port.js +++ b/test/parallel/test-worker-message-port.js @@ -74,7 +74,7 @@ const { MessageChannel, MessagePort } = require('worker_threads'); assert.deepStrictEqual( Object.getOwnPropertyNames(MessagePort.prototype).sort(), [ - 'ref', 'unref', 'start', 'close', 'postMessage', 'onmessage', - 'constructor' - ].sort()); + 'close', 'constructor', 'onmessage', 'postMessage', 'ref', 'start', + 'unref' + ]); }