From d48c5da0393666edecfc7a728b95fb0f8126732b Mon Sep 17 00:00:00 2001 From: Jason Zhang Date: Mon, 14 Oct 2024 04:41:21 +1030 Subject: [PATCH] lib: convert transfer sequence to array in js This commit lets `tranfer` passed to `structuredClone` get validated at JS layer by doing webidl conversion. This avoids the C++ to JS function call overhead in the native implementaiton of `structuredClone` PR-URL: https://github.com/nodejs/node/pull/55317 Fixes: https://github.com/nodejs/node/issues/55280 Refs: https://github.com/nodejs/node/pull/50330 Reviewed-By: Joyee Cheung Reviewed-By: Matteo Collina Reviewed-By: James M Snell Reviewed-By: Matthew Aitken --- .../bootstrap/web/exposed-window-or-worker.js | 7 +++- lib/internal/perf/usertiming.js | 2 +- lib/internal/webidl.js | 12 ++++++ lib/internal/webstreams/readablestream.js | 3 +- lib/internal/worker/js_transferable.js | 37 +++++++++++++++++++ src/node_messaging.cc | 29 ++++++--------- test/parallel/test-structuredClone-global.js | 2 +- 7 files changed, 68 insertions(+), 24 deletions(-) diff --git a/lib/internal/bootstrap/web/exposed-window-or-worker.js b/lib/internal/bootstrap/web/exposed-window-or-worker.js index a46036764dc7f6..80f32629e45252 100644 --- a/lib/internal/bootstrap/web/exposed-window-or-worker.js +++ b/lib/internal/bootstrap/web/exposed-window-or-worker.js @@ -32,8 +32,11 @@ const { } = require('internal/process/task_queues'); defineOperation(globalThis, 'queueMicrotask', queueMicrotask); -const { structuredClone } = internalBinding('messaging'); -defineOperation(globalThis, 'structuredClone', structuredClone); +defineLazyProperties( + globalThis, + 'internal/worker/js_transferable', + ['structuredClone'], +); defineLazyProperties(globalThis, 'buffer', ['atob', 'btoa']); // https://html.spec.whatwg.org/multipage/web-messaging.html#broadcasting-to-other-browsing-contexts diff --git a/lib/internal/perf/usertiming.js b/lib/internal/perf/usertiming.js index dfbbcbaf36c538..b2783799067dde 100644 --- a/lib/internal/perf/usertiming.js +++ b/lib/internal/perf/usertiming.js @@ -31,7 +31,7 @@ const { }, } = require('internal/errors'); -const { structuredClone } = internalBinding('messaging'); +const { structuredClone } = require('internal/worker/js_transferable'); const { lazyDOMException, kEnumerableProperty, diff --git a/lib/internal/webidl.js b/lib/internal/webidl.js index 4b689cf5eb24b2..1dfe965dd75182 100644 --- a/lib/internal/webidl.js +++ b/lib/internal/webidl.js @@ -36,6 +36,16 @@ converters.any = (V) => { return V; }; +converters.object = (V, opts = kEmptyObject) => { + if (type(V) !== 'Object') { + throw makeException( + 'is not an object', + kEmptyObject, + ); + } + return V; +}; + // https://webidl.spec.whatwg.org/#abstract-opdef-integerpart const integerPart = MathTrunc; @@ -187,6 +197,8 @@ converters.DOMString = function DOMString(V) { return String(V); }; +converters['sequence'] = createSequenceConverter(converters.object); + function codedTypeError(message, errorProperties = kEmptyObject) { // eslint-disable-next-line no-restricted-syntax const err = new TypeError(message); diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index 7279ff3de4b28b..cbe8256dd9c4dd 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -74,6 +74,7 @@ const { kTransfer, kTransferList, markTransferMode, + structuredClone, } = require('internal/worker/js_transferable'); const { @@ -88,8 +89,6 @@ const { kControllerErrorFunction, } = require('internal/streams/utils'); -const { structuredClone } = internalBinding('messaging'); - const { ArrayBufferViewGetBuffer, ArrayBufferViewGetByteLength, diff --git a/lib/internal/worker/js_transferable.js b/lib/internal/worker/js_transferable.js index 083702149f29d9..58e377b87a9d11 100644 --- a/lib/internal/worker/js_transferable.js +++ b/lib/internal/worker/js_transferable.js @@ -3,6 +3,13 @@ const { Error, StringPrototypeSplit, } = primordials; +const { + codes: { + ERR_INVALID_ARG_TYPE, + ERR_MISSING_ARGS, + }, +} = require('internal/errors'); +const webidl = require('internal/webidl'); const { messaging_deserialize_symbol, messaging_transfer_symbol, @@ -11,6 +18,7 @@ const { } = internalBinding('symbols'); const { setDeserializerCreateObjectFunction, + structuredClone: nativeStructuredClone, } = internalBinding('messaging'); const { privateSymbols: { @@ -90,9 +98,38 @@ function markTransferMode(obj, cloneable = false, transferable = false) { obj[transfer_mode_private_symbol] = mode; } +function structuredClone(value, options) { + if (arguments.length === 0) { + throw new ERR_MISSING_ARGS('The value argument must be specified'); + } + + // TODO(jazelly): implement generic webidl dictionary converter + const prefix = 'Options'; + const optionsType = webidl.type(options); + if (optionsType !== 'Undefined' && optionsType !== 'Null' && optionsType !== 'Object') { + throw new ERR_INVALID_ARG_TYPE( + prefix, + ['object', 'null', 'undefined'], + options, + ); + } + const key = 'transfer'; + const idlOptions = { __proto__: null, [key]: [] }; + if (options != null && key in options && options[key] !== undefined) { + idlOptions[key] = webidl.converters['sequence'](options[key], { + __proto__: null, + context: 'Transfer', + }); + } + + const serializedData = nativeStructuredClone(value, idlOptions); + return serializedData; +} + module.exports = { markTransferMode, setup, + structuredClone, kClone: messaging_clone_symbol, kDeserialize: messaging_deserialize_symbol, kTransfer: messaging_transfer_symbol, diff --git a/src/node_messaging.cc b/src/node_messaging.cc index a27faf92c172c7..7447d7c6b8a712 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -1578,28 +1578,21 @@ static void StructuredClone(const FunctionCallbackInfo& args) { Realm* realm = Realm::GetCurrent(context); Environment* env = realm->env(); - if (args.Length() == 0) { - return THROW_ERR_MISSING_ARGS(env, "The value argument must be specified"); - } - Local value = args[0]; TransferList transfer_list; - if (!args[1]->IsNullOrUndefined()) { - if (!args[1]->IsObject()) { - return THROW_ERR_INVALID_ARG_TYPE( - env, "The options argument must be either an object or undefined"); - } - Local options = args[1].As(); - Local transfer_list_v; - if (!options->Get(context, env->transfer_string()) - .ToLocal(&transfer_list_v)) { - return; - } + Local options = args[1].As(); + Local transfer_list_v; + if (!options->Get(context, env->transfer_string()) + .ToLocal(&transfer_list_v)) { + return; + } - // TODO(joyeecheung): implement this in JS land to avoid the C++ -> JS - // cost to convert a sequence into an array. - if (!GetTransferList(env, context, transfer_list_v, &transfer_list)) { + Local arr = transfer_list_v.As(); + size_t length = arr->Length(); + transfer_list.AllocateSufficientStorage(length); + for (size_t i = 0; i < length; i++) { + if (!arr->Get(context, i).ToLocal(&transfer_list[i])) { return; } } diff --git a/test/parallel/test-structuredClone-global.js b/test/parallel/test-structuredClone-global.js index 50fcf6047828a8..4192e3699e4469 100644 --- a/test/parallel/test-structuredClone-global.js +++ b/test/parallel/test-structuredClone-global.js @@ -8,12 +8,12 @@ assert.throws(() => structuredClone(undefined, ''), { code: 'ERR_INVALID_ARG_TYP assert.throws(() => structuredClone(undefined, 1), { code: 'ERR_INVALID_ARG_TYPE' }); assert.throws(() => structuredClone(undefined, { transfer: 1 }), { code: 'ERR_INVALID_ARG_TYPE' }); assert.throws(() => structuredClone(undefined, { transfer: '' }), { code: 'ERR_INVALID_ARG_TYPE' }); +assert.throws(() => structuredClone(undefined, { transfer: null }), { code: 'ERR_INVALID_ARG_TYPE' }); // Options can be null or undefined. assert.strictEqual(structuredClone(undefined), undefined); assert.strictEqual(structuredClone(undefined, null), undefined); // Transfer can be null or undefined. -assert.strictEqual(structuredClone(undefined, { transfer: null }), undefined); assert.strictEqual(structuredClone(undefined, { }), undefined); // Transferables or its subclasses should be received with its closest transferable superclass