From ca73f52d7362b26f3d9ccb609202e31a4161fadf Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Sat, 6 May 2023 17:00:50 +0800 Subject: [PATCH] src,lib: make JSTransferables based on private symbols Serializes and transfers platform objects implemented as a JS class based on private symbols instead of V8 object internal slots. This avoids the need to alter the prototype chains and mixins to make the JS class to be transferable. --- lib/internal/abort_controller.js | 19 +- lib/internal/blob.js | 16 +- lib/internal/blocklist.js | 10 +- lib/internal/crypto/keys.js | 8 +- lib/internal/crypto/x509.js | 10 +- lib/internal/event_target.js | 25 --- lib/internal/fs/promises.js | 7 +- lib/internal/histogram.js | 12 +- lib/internal/perf/event_loop_delay.js | 7 +- lib/internal/socketaddress.js | 12 +- lib/internal/test/transfer.js | 5 +- lib/internal/webstreams/readablestream.js | 13 +- lib/internal/webstreams/transfer.js | 14 +- lib/internal/webstreams/transformstream.js | 11 +- lib/internal/webstreams/writablestream.js | 11 +- lib/internal/worker/js_transferable.js | 41 ++-- src/env_properties.h | 3 + src/node_messaging.cc | 175 ++++++++++++++---- src/node_messaging.h | 18 +- src/node_util.cc | 14 ++ ....js => test-messaging-marktransfermode.js} | 4 - .../test-whatwg-webstreams-transfer.js | 12 +- ...rt-jstransferable-nested-untransferable.js | 6 +- 23 files changed, 282 insertions(+), 171 deletions(-) rename test/parallel/{test-messaging-maketransferable.js => test-messaging-marktransfermode.js} (82%) diff --git a/lib/internal/abort_controller.js b/lib/internal/abort_controller.js index 2c1f43354f9f7c..23e7ec7c5c7608 100644 --- a/lib/internal/abort_controller.js +++ b/lib/internal/abort_controller.js @@ -62,9 +62,9 @@ const { } = internalBinding('symbols'); let _MessageChannel; -let makeTransferable; +let markTransferMode; -// Loading the MessageChannel and makeTransferable have to be done lazily +// Loading the MessageChannel and markTransferable have to be done lazily // because otherwise we'll end up with a require cycle that ends up with // an incomplete initialization of abort_controller. @@ -73,10 +73,10 @@ function lazyMessageChannel() { return new _MessageChannel(); } -function lazyMakeTransferable(obj) { - makeTransferable ??= - require('internal/worker/js_transferable').makeTransferable; - return makeTransferable(obj); +function lazyMarkTransferMode(obj, cloneable, transferable) { + markTransferMode ??= + require('internal/worker/js_transferable').markTransferMode; + return markTransferMode(obj, cloneable, transferable); } const clearTimeoutRegistry = new SafeFinalizationRegistry(clearTimeout); @@ -301,7 +301,9 @@ function createAbortSignal(init = kEmptyObject) { ObjectSetPrototypeOf(signal, AbortSignal.prototype); signal[kAborted] = aborted; signal[kReason] = reason; - return transferable ? lazyMakeTransferable(signal) : signal; + if (transferable) + lazyMarkTransferMode(signal, false, true); + return signal; } function abortSignal(signal, reason) { @@ -353,7 +355,8 @@ class AbortController { function transferableAbortSignal(signal) { if (signal?.[kAborted] === undefined) throw new ERR_INVALID_ARG_TYPE('signal', 'AbortSignal', signal); - return lazyMakeTransferable(signal); + lazyMarkTransferMode(signal, false, true); + return signal; } /** diff --git a/lib/internal/blob.js b/lib/internal/blob.js index ee8e1c75819ab9..b2eed6d126f335 100644 --- a/lib/internal/blob.js +++ b/lib/internal/blob.js @@ -32,7 +32,7 @@ const { const { URL } = require('internal/url'); const { - makeTransferable, + markTransferMode, kClone, kDeserialize, } = require('internal/worker/js_transferable'); @@ -136,6 +136,8 @@ class Blob { * @constructs {Blob} */ constructor(sources = [], options) { + markTransferMode(this, true, false); + if (sources === null || typeof sources[SymbolIterator] !== 'function' || typeof sources === 'string') { @@ -167,9 +169,6 @@ class Blob { type = `${type}`; this[kType] = RegExpPrototypeExec(disallowedTypeCharacters, type) !== null ? '' : StringPrototypeToLowerCase(type); - - // eslint-disable-next-line no-constructor-return - return makeTransferable(this); } [kInspect](depth, options) { @@ -374,16 +373,19 @@ class Blob { } function ClonedBlob() { - return makeTransferable(ReflectConstruct(function() {}, [], Blob)); + return ReflectConstruct(function() { + markTransferMode(this, true, false); + }, [], Blob); } ClonedBlob.prototype[kDeserialize] = () => {}; function createBlob(handle, length, type = '') { - return makeTransferable(ReflectConstruct(function() { + return ReflectConstruct(function() { + markTransferMode(this, true, false); this[kHandle] = handle; this[kType] = type; this[kLength] = length; - }, [], Blob)); + }, [], Blob); } ObjectDefineProperty(Blob.prototype, SymbolToStringTag, { diff --git a/lib/internal/blocklist.js b/lib/internal/blocklist.js index c0bce00321fc8b..d4eb35c9a70cb8 100644 --- a/lib/internal/blocklist.js +++ b/lib/internal/blocklist.js @@ -20,7 +20,7 @@ const { } = require('internal/socketaddress'); const { - JSTransferable, + markTransferMode, kClone, kDeserialize, } = require('internal/worker/js_transferable'); @@ -36,9 +36,9 @@ const { const { validateInt32, validateString } = require('internal/validators'); -class BlockList extends JSTransferable { +class BlockList { constructor() { - super(); + markTransferMode(this, true, false); this[kHandle] = new BlockListHandle(); this[kHandle][owner_symbol] = this; } @@ -148,9 +148,9 @@ class BlockList extends JSTransferable { } } -class InternalBlockList extends JSTransferable { +class InternalBlockList { constructor(handle) { - super(); + markTransferMode(this, true, false); this[kHandle] = handle; if (handle !== undefined) handle[owner_symbol] = this; diff --git a/lib/internal/crypto/keys.js b/lib/internal/crypto/keys.js index 4b06810a8729c4..1e49bd62473a1b 100644 --- a/lib/internal/crypto/keys.js +++ b/lib/internal/crypto/keys.js @@ -57,7 +57,7 @@ const { } = require('internal/util/types'); const { - makeTransferable, + markTransferMode, kClone, kDeserialize, } = require('internal/worker/js_transferable'); @@ -706,7 +706,7 @@ ObjectDefineProperties(CryptoKey.prototype, { // All internal code must use new InternalCryptoKey to create // CryptoKey instances. The CryptoKey class is exposed to end // user code but is not permitted to be constructed directly. -// Using makeTransferable also allows the CryptoKey to be +// Using markTransferMode also allows the CryptoKey to be // cloned to Workers. class InternalCryptoKey { constructor( @@ -714,6 +714,7 @@ class InternalCryptoKey { algorithm, keyUsages, extractable) { + markTransferMode(this, true, false); // Using symbol properties here currently instead of private // properties because (for now) the performance penalty of // private fields is still too high. @@ -721,9 +722,6 @@ class InternalCryptoKey { this[kAlgorithm] = algorithm; this[kExtractable] = extractable; this[kKeyUsages] = keyUsages; - - // eslint-disable-next-line no-constructor-return - return makeTransferable(this); } [kClone]() { diff --git a/lib/internal/crypto/x509.js b/lib/internal/crypto/x509.js index 0e9d4e87506329..30005390a4571e 100644 --- a/lib/internal/crypto/x509.js +++ b/lib/internal/crypto/x509.js @@ -48,7 +48,7 @@ const { } = require('internal/errors'); const { - JSTransferable, + markTransferMode, kClone, kDeserialize, } = require('internal/worker/js_transferable'); @@ -94,16 +94,16 @@ function getFlags(options = kEmptyObject) { return flags; } -class InternalX509Certificate extends JSTransferable { +class InternalX509Certificate { [kInternalState] = new SafeMap(); constructor(handle) { - super(); + markTransferMode(this, true, false); this[kHandle] = handle; } } -class X509Certificate extends JSTransferable { +class X509Certificate { [kInternalState] = new SafeMap(); constructor(buffer) { @@ -115,7 +115,7 @@ class X509Certificate extends JSTransferable { ['string', 'Buffer', 'TypedArray', 'DataView'], buffer); } - super(); + markTransferMode(this, true, false); this[kHandle] = parseX509(buffer); } diff --git a/lib/internal/event_target.js b/lib/internal/event_target.js index 61aa31afa4449e..277927bac6398a 100644 --- a/lib/internal/event_target.js +++ b/lib/internal/event_target.js @@ -10,11 +10,7 @@ const { ObjectDefineProperties, ObjectDefineProperty, ObjectGetOwnPropertyDescriptor, - ObjectGetOwnPropertyDescriptors, - ObjectSetPrototypeOf, - ObjectValues, ReflectApply, - SafeArrayIterator, SafeFinalizationRegistry, SafeMap, SafeWeakMap, @@ -1097,30 +1093,9 @@ function defineEventHandler(emitter, name, event = name) { }); } -const EventEmitterMixin = (Superclass) => { - class MixedEventEmitter extends Superclass { - constructor(...args) { - args = new SafeArrayIterator(args); - super(...args); - FunctionPrototypeCall(EventEmitter, this); - } - } - const protoProps = ObjectGetOwnPropertyDescriptors(EventEmitter.prototype); - delete protoProps.constructor; - const propertiesValues = ObjectValues(protoProps); - for (let i = 0; i < propertiesValues.length; i++) { - // We want to use null-prototype objects to not rely on globally mutable - // %Object.prototype%. - ObjectSetPrototypeOf(propertiesValues[i], null); - } - ObjectDefineProperties(MixedEventEmitter.prototype, protoProps); - return MixedEventEmitter; -}; - module.exports = { Event, CustomEvent, - EventEmitterMixin, EventTarget, NodeEventTarget, defineEventHandler, diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index caea9592e14fd7..fafa1e2efc8542 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -89,7 +89,7 @@ const { lazyDOMException, promisify, } = require('internal/util'); -const { EventEmitterMixin } = require('internal/event_target'); +const EventEmitter = require('events'); const { StringDecoder } = require('string_decoder'); const { kFSWatchStart, watch } = require('internal/fs/watchers'); const nonNativeWatcher = require('internal/fs/recursive_watch'); @@ -109,7 +109,7 @@ const kLocked = Symbol('kLocked'); const { kUsePromises } = binding; const { Interface } = require('internal/readline/interface'); const { - JSTransferable, kDeserialize, kTransfer, kTransferList, + kDeserialize, kTransfer, kTransferList, markTransferMode, } = require('internal/worker/js_transferable'); const getDirectoryEntriesPromise = promisify(getDirents); @@ -129,12 +129,13 @@ function lazyFsStreams() { return fsStreams ??= require('internal/fs/streams'); } -class FileHandle extends EventEmitterMixin(JSTransferable) { +class FileHandle extends EventEmitter { /** * @param {InternalFSBinding.FileHandle | undefined} filehandle */ constructor(filehandle) { super(); + markTransferMode(this, false, true); this[kHandle] = filehandle; this[kFd] = filehandle ? filehandle.fd : -1; diff --git a/lib/internal/histogram.js b/lib/internal/histogram.js index 37bde8195e4d34..079fbce50d54e3 100644 --- a/lib/internal/histogram.js +++ b/lib/internal/histogram.js @@ -45,7 +45,7 @@ const kRecordable = Symbol('kRecordable'); const { kClone, kDeserialize, - makeTransferable, + markTransferMode, } = require('internal/worker/js_transferable'); function isHistogram(object) { @@ -319,21 +319,23 @@ class RecordableHistogram extends Histogram { } function internalHistogram(handle) { - return makeTransferable(ReflectConstruct( + return ReflectConstruct( function() { + markTransferMode(this, true, false); this[kHandle] = handle; this[kMap] = new SafeMap(); - }, [], Histogram)); + }, [], Histogram); } internalHistogram.prototype[kDeserialize] = () => {}; function internalRecordableHistogram(handle) { - return makeTransferable(ReflectConstruct( + return ReflectConstruct( function() { + markTransferMode(this, true, false); this[kHandle] = handle; this[kMap] = new SafeMap(); this[kRecordable] = true; - }, [], RecordableHistogram)); + }, [], RecordableHistogram); } internalRecordableHistogram.prototype[kDeserialize] = () => {}; diff --git a/lib/internal/perf/event_loop_delay.js b/lib/internal/perf/event_loop_delay.js index 1c1d657a9a0e61..8281ea105f4083 100644 --- a/lib/internal/perf/event_loop_delay.js +++ b/lib/internal/perf/event_loop_delay.js @@ -32,7 +32,7 @@ const { } = require('internal/util'); const { - makeTransferable, + markTransferMode, } = require('internal/worker/js_transferable'); const kEnabled = Symbol('kEnabled'); @@ -79,12 +79,13 @@ function monitorEventLoopDelay(options = kEmptyObject) { const { resolution = 10 } = options; validateInteger(resolution, 'options.resolution', 1); - return makeTransferable(ReflectConstruct( + return ReflectConstruct( function() { + markTransferMode(this, true, false); this[kEnabled] = false; this[kHandle] = createELDHistogram(resolution); this[kMap] = new SafeMap(); - }, [], ELDHistogram)); + }, [], ELDHistogram); } module.exports = monitorEventLoopDelay; diff --git a/lib/internal/socketaddress.js b/lib/internal/socketaddress.js index 2fd524bc7237f0..e432c8a7d7593a 100644 --- a/lib/internal/socketaddress.js +++ b/lib/internal/socketaddress.js @@ -32,7 +32,7 @@ const { const { inspect } = require('internal/util/inspect'); const { - JSTransferable, + markTransferMode, kClone, kDeserialize, } = require('internal/worker/js_transferable'); @@ -40,13 +40,14 @@ const { const kHandle = Symbol('kHandle'); const kDetail = Symbol('kDetail'); -class SocketAddress extends JSTransferable { +class SocketAddress { static isSocketAddress(value) { return value?.[kHandle] !== undefined; } constructor(options = kEmptyObject) { - super(); + markTransferMode(this, true, false); + validateObject(options, 'options'); let { family = 'ipv4' } = options; const { @@ -139,9 +140,10 @@ class SocketAddress extends JSTransferable { } } -class InternalSocketAddress extends JSTransferable { +class InternalSocketAddress { constructor(handle) { - super(); + markTransferMode(this, true, false); + this[kHandle] = handle; this[kDetail] = this[kHandle]?.detail({ address: undefined, diff --git a/lib/internal/test/transfer.js b/lib/internal/test/transfer.js index 635f3033e76642..72361fb22199ef 100644 --- a/lib/internal/test/transfer.js +++ b/lib/internal/test/transfer.js @@ -1,7 +1,7 @@ 'use strict'; const { - makeTransferable, + markTransferMode, kClone, kDeserialize, } = require('internal/worker/js_transferable'); @@ -23,8 +23,7 @@ class E { class F extends E { constructor(b) { super(b); - /* eslint-disable-next-line no-constructor-return */ - return makeTransferable(this); + markTransferMode(this, true, false); } [kClone]() { diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index 0b8b8ac1ef584d..b2f66157d7d29e 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -71,7 +71,7 @@ const { kDeserialize, kTransfer, kTransferList, - makeTransferable, + markTransferMode, } = require('internal/worker/js_transferable'); const { @@ -244,6 +244,7 @@ class ReadableStream { * @param {QueuingStrategy} [strategy] */ constructor(source = {}, strategy = kEmptyObject) { + markTransferMode(this, false, true); if (source === null) throw new ERR_INVALID_ARG_VALUE('source', 'Object', source); this[kState] = { @@ -289,9 +290,6 @@ class ReadableStream { extractHighWaterMark(highWaterMark, 1), extractSizeAlgorithm(size)); } - - // eslint-disable-next-line no-constructor-return - return makeTransferable(this); } get [kIsDisturbed]() { @@ -643,8 +641,9 @@ ObjectDefineProperties(ReadableStream.prototype, { }); function TransferredReadableStream() { - return makeTransferable(ReflectConstruct( + return ReflectConstruct( function() { + markTransferMode(this, false, true); this[kType] = 'ReadableStream'; this[kState] = { disturbed: false, @@ -659,7 +658,7 @@ function TransferredReadableStream() { }; this[kIsClosedPromise] = createDeferredPromise(); }, - [], ReadableStream)); + [], ReadableStream); } TransferredReadableStream.prototype[kDeserialize] = () => {}; @@ -1207,6 +1206,7 @@ function createReadableByteStreamController() { function createTeeReadableStream(start, pull, cancel) { return ReflectConstruct( function() { + markTransferMode(this, false, true); this[kType] = 'ReadableStream'; this[kState] = { disturbed: false, @@ -1229,7 +1229,6 @@ function createTeeReadableStream(start, pull, cancel) { }), 1, () => 1); - return makeTransferable(this); }, [], ReadableStream, ); } diff --git a/lib/internal/webstreams/transfer.js b/lib/internal/webstreams/transfer.js index 367daf4bc17339..136b0d81a99464 100644 --- a/lib/internal/webstreams/transfer.js +++ b/lib/internal/webstreams/transfer.js @@ -35,7 +35,7 @@ const { const assert = require('internal/assert'); const { - makeTransferable, + markTransferMode, kClone, kDeserialize, } = require('internal/worker/js_transferable'); @@ -49,13 +49,12 @@ const { class CloneableDOMException extends DOMException { constructor(message, name) { super(message, name); + markTransferMode(this, true, false); this[kDeserialize]({ message: this.message, name: this.name, code: this.code, }); - // eslint-disable-next-line no-constructor-return - return makeTransferable(this); } [kClone]() { @@ -95,11 +94,10 @@ class CloneableDOMException extends DOMException { } function InternalCloneableDOMException() { - return makeTransferable( - ReflectConstruct( - CloneableDOMException, - [], - DOMException)); + return ReflectConstruct( + CloneableDOMException, + [], + DOMException); } InternalCloneableDOMException[kDeserialize] = () => {}; diff --git a/lib/internal/webstreams/transformstream.js b/lib/internal/webstreams/transformstream.js index 3a7fbebc042a22..c5b2aa90ffae5f 100644 --- a/lib/internal/webstreams/transformstream.js +++ b/lib/internal/webstreams/transformstream.js @@ -34,7 +34,7 @@ const { kDeserialize, kTransfer, kTransferList, - makeTransferable, + markTransferMode, } = require('internal/worker/js_transferable'); const { @@ -120,6 +120,7 @@ class TransformStream { transformer = null, writableStrategy = kEmptyObject, readableStrategy = kEmptyObject) { + markTransferMode(this, false, true); const readableType = transformer?.readableType; const writableType = transformer?.writableType; const start = transformer?.start; @@ -170,9 +171,6 @@ class TransformStream { } else { startPromise.resolve(); } - - // eslint-disable-next-line no-constructor-return - return makeTransferable(this); } /** @@ -247,8 +245,9 @@ ObjectDefineProperties(TransformStream.prototype, { }); function TransferredTransformStream() { - return makeTransferable(ReflectConstruct( + return ReflectConstruct( function() { + markTransferMode(this, false, true); this[kType] = 'TransformStream'; this[kState] = { readable: undefined, @@ -262,7 +261,7 @@ function TransferredTransformStream() { controller: undefined, }; }, - [], TransformStream)); + [], TransformStream); } TransferredTransformStream.prototype[kDeserialize] = () => {}; diff --git a/lib/internal/webstreams/writablestream.js b/lib/internal/webstreams/writablestream.js index e04ff381c51cd4..654cb21457883b 100644 --- a/lib/internal/webstreams/writablestream.js +++ b/lib/internal/webstreams/writablestream.js @@ -46,7 +46,7 @@ const { kDeserialize, kTransfer, kTransferList, - makeTransferable, + markTransferMode, } = require('internal/worker/js_transferable'); const { @@ -155,6 +155,7 @@ class WritableStream { * @param {QueuingStrategy} [strategy] */ constructor(sink = null, strategy = kEmptyObject) { + markTransferMode(this, false, true); const type = sink?.type; if (type !== undefined) throw new ERR_INVALID_ARG_VALUE.RangeError('type', type); @@ -210,9 +211,6 @@ class WritableStream { sink, highWaterMark, size); - - // eslint-disable-next-line no-constructor-return - return makeTransferable(this); } /** @@ -329,8 +327,9 @@ ObjectDefineProperties(WritableStream.prototype, { }); function TransferredWritableStream() { - return makeTransferable(ReflectConstruct( + return ReflectConstruct( function() { + markTransferMode(this, false, true); this[kType] = 'WritableStream'; this[kState] = { close: createDeferredPromise(), @@ -374,7 +373,7 @@ function TransferredWritableStream() { this[kIsClosedPromise] = createDeferredPromise(); this[kControllerErrorFunction] = () => {}; }, - [], WritableStream)); + [], WritableStream); } TransferredWritableStream.prototype[kDeserialize] = () => {}; diff --git a/lib/internal/worker/js_transferable.js b/lib/internal/worker/js_transferable.js index 41ef278b33174d..1adb8beea56e99 100644 --- a/lib/internal/worker/js_transferable.js +++ b/lib/internal/worker/js_transferable.js @@ -1,12 +1,6 @@ 'use strict'; const { Error, - ObjectDefineProperties, - ObjectGetOwnPropertyDescriptors, - ObjectGetPrototypeOf, - ObjectSetPrototypeOf, - ObjectValues, - ReflectConstruct, StringPrototypeSplit, } = primordials; const { @@ -16,9 +10,18 @@ const { messaging_transfer_list_symbol, } = internalBinding('symbols'); const { - JSTransferable, setDeserializerCreateObjectFunction, } = internalBinding('messaging'); +const { + privateSymbols: { + transfer_mode_private_symbol, + }, + constants: { + kDisallowCloneAndTransfer, + kTransferable, + kCloneable, + }, +} = internalBinding('util'); function setup() { // Register the handler that will be used when deserializing JS-based objects @@ -39,26 +42,18 @@ function setup() { }); } -function makeTransferable(obj) { - // If the object is already transferable, skip all this. - if (obj instanceof JSTransferable) return obj; - const inst = ReflectConstruct(JSTransferable, [], obj.constructor); - const properties = ObjectGetOwnPropertyDescriptors(obj); - const propertiesValues = ObjectValues(properties); - for (let i = 0; i < propertiesValues.length; i++) { - // We want to use null-prototype objects to not rely on globally mutable - // %Object.prototype%. - ObjectSetPrototypeOf(propertiesValues[i], null); - } - ObjectDefineProperties(inst, properties); - ObjectSetPrototypeOf(inst, ObjectGetPrototypeOf(obj)); - return inst; +function markTransferMode(obj, cloneable = false, transferable = false) { + if ((typeof obj !== 'object' && typeof obj !== 'function') || obj === null) + return; // This object is a primitive and therefore already untransferable. + let mode = kDisallowCloneAndTransfer; + if (cloneable) mode |= kCloneable; + if (transferable) mode |= kTransferable; + obj[transfer_mode_private_symbol] = mode; } module.exports = { - makeTransferable, + markTransferMode, setup, - JSTransferable, kClone: messaging_clone_symbol, kDeserialize: messaging_deserialize_symbol, kTransfer: messaging_transfer_symbol, diff --git a/src/env_properties.h b/src/env_properties.h index 3ef589397dbdcc..49c63caf3decbf 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -21,6 +21,8 @@ V(arrow_message_private_symbol, "node:arrowMessage") \ V(contextify_context_private_symbol, "node:contextify:context") \ V(decorated_private_symbol, "node:decorated") \ + V(transfer_mode_private_symbol, "node:transfer_mode") \ + V(js_transferable_wrapper_private_symbol, "node:js_transferable_wrapper") \ V(napi_type_tag, "node:napi:type_tag") \ V(napi_wrapper, "node:napi:wrapper") \ V(untransferable_object_private_symbol, "node:untransferableObject") \ @@ -356,6 +358,7 @@ V(http2ping_constructor_template, v8::ObjectTemplate) \ V(i18n_converter_template, v8::ObjectTemplate) \ V(intervalhistogram_constructor_template, v8::FunctionTemplate) \ + V(js_transferable_constructor_template, v8::FunctionTemplate) \ V(libuv_stream_wrap_ctor_template, v8::FunctionTemplate) \ V(message_port_constructor_template, v8::FunctionTemplate) \ V(microtask_queue_ctor_template, v8::FunctionTemplate) \ diff --git a/src/node_messaging.cc b/src/node_messaging.cc index b108b907e7af7f..37d6de94287bee 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -75,7 +75,8 @@ class DeserializerDelegate : public ValueDeserializer::Delegate { const std::vector>& shared_array_buffers, const std::vector& wasm_modules, const std::optional& shared_value_conveyor) - : host_objects_(host_objects), + : env_(env), + host_objects_(host_objects), shared_array_buffers_(shared_array_buffers), wasm_modules_(wasm_modules), shared_value_conveyor_(shared_value_conveyor) {} @@ -87,7 +88,12 @@ class DeserializerDelegate : public ValueDeserializer::Delegate { return MaybeLocal(); if (id != kNormalObject) { CHECK_LT(id, host_objects_.size()); - return host_objects_[id]->object(isolate); + Local object = host_objects_[id]->object(isolate); + if (env_->js_transferable_constructor_template()->HasInstance(object)) { + return Unwrap(object)->target(); + } else { + return object; + } } EscapableHandleScope scope(isolate); Local context = isolate->GetCurrentContext(); @@ -119,6 +125,7 @@ class DeserializerDelegate : public ValueDeserializer::Delegate { ValueDeserializer* deserializer = nullptr; private: + Environment* env_; const std::vector>& host_objects_; const std::vector>& shared_array_buffers_; const std::vector& wasm_modules_; @@ -297,12 +304,33 @@ class SerializerDelegate : public ValueSerializer::Delegate { ThrowDataCloneException(context_, message); } + bool HasCustomHostObject(Isolate* isolate) override { return true; } + + Maybe IsHostObject(Isolate* isolate, Local object) override { + if (BaseObject::IsBaseObject(object)) { + return Just(true); + } + + return JSTransferable::IsJSTransferable(env_, context_, object); + } + Maybe WriteHostObject(Isolate* isolate, Local object) override { if (BaseObject::IsBaseObject(object)) { return WriteHostObject( BaseObjectPtr { Unwrap(object) }); } + bool is_js_transferable; + if (!JSTransferable::IsJSTransferable(env_, context_, object) + .To(&is_js_transferable)) { + return Nothing(); + } + if (is_js_transferable) { + JSTransferable* js_transferable = JSTransferable::Wrap(env_, object); + if (!js_transferable) return Nothing(); + return WriteHostObject(BaseObjectPtr{js_transferable}); + } + // Convert process.env to a regular object. auto env_proxy_ctor_template = env_->env_proxy_ctor_template(); if (!env_proxy_ctor_template.IsEmpty() && @@ -498,8 +526,7 @@ Maybe Message::Serialize(Environment* env, array_buffers.push_back(ab); serializer.TransferArrayBuffer(id, ab); continue; - } else if (entry->IsObject() && - BaseObject::IsBaseObject(entry.As())) { + } else if (entry->IsObject()) { // Check if the source MessagePort is being transferred. if (!source_port.IsEmpty() && entry == source_port) { ThrowDataCloneException( @@ -508,8 +535,26 @@ Maybe Message::Serialize(Environment* env, "Transfer list contains source port")); return Nothing(); } - BaseObjectPtr host_object { - Unwrap(entry.As()) }; + BaseObjectPtr host_object; + if (BaseObject::IsBaseObject(entry.As())) { + host_object = + BaseObjectPtr{Unwrap(entry.As())}; + } else { + bool is_js_transferable; + if (!JSTransferable::IsJSTransferable(env, context, entry.As()) + .To(&is_js_transferable)) { + return Nothing(); + } + if (!is_js_transferable) { + THROW_ERR_INVALID_TRANSFER_OBJECT(env); + return Nothing(); + } + JSTransferable* js_transferable = + JSTransferable::Wrap(env, entry.As()); + if (!js_transferable) return Nothing(); + host_object = BaseObjectPtr{js_transferable}; + } + if (env->message_port_constructor_template()->HasInstance(entry) && (!host_object || static_cast(host_object.get())->IsDetached())) { @@ -1163,28 +1208,74 @@ Local GetMessagePortConstructorTemplate(Environment* env) { return GetMessagePortConstructorTemplate(env); } -JSTransferable::JSTransferable(Environment* env, Local obj) +// static +JSTransferable* JSTransferable::Wrap(Environment* env, Local target) { + Local context = env->context(); + Local wrapper_val; + if (!target + ->GetPrivate(context, env->js_transferable_wrapper_private_symbol()) + .ToLocal(&wrapper_val)) { + return nullptr; + } + DCHECK(wrapper_val->IsObject() || wrapper_val->IsUndefined()); + JSTransferable* wrapper; + if (wrapper_val->IsObject()) { + wrapper = Unwrap(wrapper_val); + } else { + Local wrapper_obj = env->js_transferable_constructor_template() + ->GetFunction(context) + .ToLocalChecked() + ->NewInstance(context) + .ToLocalChecked(); + wrapper = new JSTransferable(env, wrapper_obj, target); + if (target + ->SetPrivate(context, + env->js_transferable_wrapper_private_symbol(), + wrapper_obj) + .IsNothing()) { + return nullptr; + } + } + return wrapper; +} + +// static +Maybe JSTransferable::IsJSTransferable(Environment* env, + v8::Local context, + v8::Local object) { + bool has_transfer_mode; + if (!object->HasPrivate(context, env->transfer_mode_private_symbol()) + .To(&has_transfer_mode)) { + return Nothing(); + } + return Just(has_transfer_mode); +} + +JSTransferable::JSTransferable(Environment* env, + Local obj, + Local target) : BaseObject(env, obj) { MakeWeak(); + target_.Reset(env->isolate(), target); + target_.SetWeak(); } -void JSTransferable::New(const FunctionCallbackInfo& args) { - CHECK(args.IsConstructCall()); - new JSTransferable(Environment::GetCurrent(args), args.This()); +Local JSTransferable::target() const { + return target_.Get(env()->isolate()); } uint32_t JSTransferable::GetTransferMode() const { - // Implement `kClone in this ? kCloneable : kTransferable`. HandleScope handle_scope(env()->isolate()); errors::TryCatchScope ignore_exceptions(env()); - bool has_clone; - if (!object()->Has(env()->context(), - env()->messaging_clone_symbol()).To(&has_clone)) { + Local transfer_mode_val; + if (!target() + ->GetPrivate(env()->context(), env()->transfer_mode_private_symbol()) + .ToLocal(&transfer_mode_val) && + !transfer_mode_val->IsUint32()) { return TransferMode::kDisallowCloneAndTransfer; } - - return has_clone ? TransferMode::kCloneable : TransferMode::kTransferable; + return transfer_mode_val.As()->Value(); } std::unique_ptr JSTransferable::TransferForMessaging() { @@ -1208,7 +1299,7 @@ std::unique_ptr JSTransferable::TransferOrClone() const { : env()->messaging_transfer_symbol(); Local method; - if (!object()->Get(context, method_name).ToLocal(&method)) { + if (!target()->Get(context, method_name).ToLocal(&method)) { return {}; } if (!method->IsFunction()) { @@ -1216,7 +1307,7 @@ std::unique_ptr JSTransferable::TransferOrClone() const { } Local result_v; if (!method.As() - ->Call(context, object(), 0, nullptr) + ->Call(context, target(), 0, nullptr) .ToLocal(&result_v)) { return {}; } @@ -1246,14 +1337,15 @@ JSTransferable::NestedTransferables() const { Local method_name = env()->messaging_transfer_list_symbol(); Local method; - if (!object()->Get(context, method_name).ToLocal(&method)) { + if (!target()->Get(context, method_name).ToLocal(&method)) { return Nothing(); } if (!method->IsFunction()) return Just(BaseObjectList {}); Local list_v; - if (!method.As()->Call( - context, object(), 0, nullptr).ToLocal(&list_v)) { + if (!method.As() + ->Call(context, target(), 0, nullptr) + .ToLocal(&list_v)) { return Nothing(); } if (!list_v->IsArray()) return Just(BaseObjectList {}); @@ -1264,8 +1356,25 @@ JSTransferable::NestedTransferables() const { Local value; if (!list->Get(context, i).ToLocal(&value)) return Nothing(); - if (value->IsObject() && BaseObject::IsBaseObject(value.As())) + if (!value->IsObject()) { + continue; + } + if (BaseObject::IsBaseObject(value.As())) { ret.emplace_back(Unwrap(value)); + continue; + } + bool is_js_transferable; + if (!JSTransferable::IsJSTransferable(env(), context, value.As()) + .To(&is_js_transferable)) { + return Nothing(); + } + if (!is_js_transferable) { + continue; + } + JSTransferable* js_transferable = + JSTransferable::Wrap(env(), value.As()); + if (!js_transferable) return Nothing(); + ret.emplace_back(js_transferable); } return Just(ret); } @@ -1280,12 +1389,12 @@ Maybe JSTransferable::FinalizeTransferRead( Local method_name = env()->messaging_deserialize_symbol(); Local method; - if (!object()->Get(context, method_name).ToLocal(&method)) { + if (!target()->Get(context, method_name).ToLocal(&method)) { return Nothing(); } if (!method->IsFunction()) return Just(true); - if (method.As()->Call(context, object(), 1, &data).IsEmpty()) { + if (method.As()->Call(context, target(), 1, &data).IsEmpty()) { return Nothing(); } return Just(true); @@ -1319,11 +1428,18 @@ BaseObjectPtr JSTransferable::Data::Deserialize( if (!env->messaging_deserialize_create_object() ->Call(context, Null(env->isolate()), 1, &info) .ToLocal(&ret) || - !ret->IsObject() || !BaseObject::IsBaseObject(ret.As())) { + !ret->IsObject()) { return {}; } - return BaseObjectPtr { Unwrap(ret) }; + bool is_js_transferable; + if (!JSTransferable::IsJSTransferable(env, context, ret.As()) + .To(&is_js_transferable)) { + return {}; + } + JSTransferable* js_transferable = JSTransferable::Wrap(env, ret.As()); + if (!js_transferable) return {}; + return BaseObjectPtr{js_transferable}; } Maybe JSTransferable::Data::FinalizeTransferWrite( @@ -1498,13 +1614,11 @@ static void InitMessaging(Local target, } { - Local t = - NewFunctionTemplate(isolate, JSTransferable::New); + Local t = FunctionTemplate::New(isolate); t->InstanceTemplate()->SetInternalFieldCount( JSTransferable::kInternalFieldCount); t->SetClassName(OneByteString(isolate, "JSTransferable")); - SetConstructorFunction( - context, target, "JSTransferable", t, SetConstructorFunctionFlag::NONE); + env->isolate_data()->set_js_transferable_constructor_template(t); } SetConstructorFunction(context, @@ -1541,7 +1655,6 @@ static void InitMessaging(Local target, static void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(MessageChannel); registry->Register(BroadcastChannel); - registry->Register(JSTransferable::New); registry->Register(MessagePort::New); registry->Register(MessagePort::PostMessage); registry->Register(MessagePort::Start); diff --git a/src/node_messaging.h b/src/node_messaging.h index 01a1f2cf9228a5..5d2d2bbce2161a 100644 --- a/src/node_messaging.h +++ b/src/node_messaging.h @@ -319,13 +319,15 @@ class MessagePort : public HandleWrap { friend class MessagePortData; }; -// Provide a base class from which JS classes that should be transferable or -// cloneable by postMessage() can inherit. +// Provide a wrapper class created when a built-in JS classes that being +// transferable or cloneable by postMessage(). // See e.g. FileHandle in internal/fs/promises.js for an example. class JSTransferable : public BaseObject { public: - JSTransferable(Environment* env, v8::Local obj); - static void New(const v8::FunctionCallbackInfo& args); + static JSTransferable* Wrap(Environment* env, v8::Local target); + static v8::Maybe IsJSTransferable(Environment* env, + v8::Local context, + v8::Local object); uint32_t GetTransferMode() const override; std::unique_ptr TransferForMessaging() override; @@ -340,10 +342,18 @@ class JSTransferable : public BaseObject { SET_MEMORY_INFO_NAME(JSTransferable) SET_SELF_SIZE(JSTransferable) + v8::Local target() const; + private: + JSTransferable(Environment* env, + v8::Local obj, + v8::Local target); + template std::unique_ptr TransferOrClone() const; + v8::Global target_; + class Data : public TransferData { public: Data(std::string&& deserialize_info, v8::Global&& data); diff --git a/src/node_util.cc b/src/node_util.cc index c434f201e592a4..c3dc06316bce42 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -433,6 +433,20 @@ void Initialize(Local target, V(SKIP_SYMBOLS); #undef V +#define V(name) \ + constants \ + ->Set( \ + context, \ + FIXED_ONE_BYTE_STRING(isolate, #name), \ + Integer::New(isolate, \ + static_cast(BaseObject::TransferMode::name))) \ + .Check(); + + V(kDisallowCloneAndTransfer); + V(kTransferable); + V(kCloneable); +#undef V + target->Set(context, env->constants_string(), constants).Check(); } diff --git a/test/parallel/test-messaging-maketransferable.js b/test/parallel/test-messaging-marktransfermode.js similarity index 82% rename from test/parallel/test-messaging-maketransferable.js rename to test/parallel/test-messaging-marktransfermode.js index 07b1081045d99f..20084b6c9c2078 100644 --- a/test/parallel/test-messaging-maketransferable.js +++ b/test/parallel/test-messaging-marktransfermode.js @@ -4,9 +4,6 @@ const common = require('../common'); const assert = require('assert'); -const { - JSTransferable, -} = require('internal/worker/js_transferable'); const { E, F } = require('internal/test/transfer'); // Tests that F is transferable even tho it does not directly, @@ -17,7 +14,6 @@ const mc = new MessageChannel(); mc.port1.onmessageerror = common.mustNotCall(); mc.port1.onmessage = common.mustCall(({ data }) => { - assert(!(data instanceof JSTransferable)); assert(data instanceof F); assert(data instanceof E); assert.strictEqual(data.b, 1); diff --git a/test/parallel/test-whatwg-webstreams-transfer.js b/test/parallel/test-whatwg-webstreams-transfer.js index 6abc2fe2a87f91..f8783259422890 100644 --- a/test/parallel/test-whatwg-webstreams-transfer.js +++ b/test/parallel/test-whatwg-webstreams-transfer.js @@ -31,7 +31,7 @@ const { } = require('internal/webstreams/util'); const { - makeTransferable, + markTransferMode, kClone, kTransfer, kDeserialize, @@ -324,7 +324,7 @@ const theData = 'hello'; port2.postMessage(readable, [readable]); - const notActuallyTransferable = makeTransferable({ + const notActuallyTransferable = { [kClone]() { return { data: {}, @@ -332,7 +332,8 @@ const theData = 'hello'; }; }, [kDeserialize]: common.mustNotCall(), - }); + }; + markTransferMode(notActuallyTransferable, true, false); controller.enqueue(notActuallyTransferable); } @@ -351,7 +352,7 @@ const theData = 'hello'; const writable = new WritableStream(source); - const notActuallyTransferable = makeTransferable({ + const notActuallyTransferable = { [kClone]() { return { data: {}, @@ -359,7 +360,8 @@ const theData = 'hello'; }; }, [kDeserialize]: common.mustNotCall(), - }); + }; + markTransferMode(notActuallyTransferable, true, false); port1.onmessage = common.mustCall(({ data }) => { const writer = data.getWriter(); diff --git a/test/parallel/test-worker-message-port-jstransferable-nested-untransferable.js b/test/parallel/test-worker-message-port-jstransferable-nested-untransferable.js index 621c42be5149cd..2f94fccd394452 100644 --- a/test/parallel/test-worker-message-port-jstransferable-nested-untransferable.js +++ b/test/parallel/test-worker-message-port-jstransferable-nested-untransferable.js @@ -3,16 +3,16 @@ const common = require('../common'); const assert = require('assert'); const { - JSTransferable, kTransfer, kTransferList + markTransferMode, kTransfer, kTransferList } = require('internal/worker/js_transferable'); const { MessageChannel } = require('worker_threads'); // Transferring a JSTransferable that refers to another, untransferable, value // in its transfer list should not crash hard. -class OuterTransferable extends JSTransferable { +class OuterTransferable { constructor() { - super(); + markTransferMode(this, false, true); // Create a detached MessagePort at this.inner const c = new MessageChannel(); this.inner = c.port1;