Skip to content

Commit

Permalink
lib: convert transfer sequence to array in js
Browse files Browse the repository at this point in the history
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: nodejs#55317
Fixes: nodejs#55280
Refs: nodejs#50330
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
  • Loading branch information
jazelly authored and tpoisseau committed Nov 21, 2024
1 parent b52a0a1 commit fc178ba
Show file tree
Hide file tree
Showing 7 changed files with 89 additions and 24 deletions.
7 changes: 5 additions & 2 deletions lib/internal/bootstrap/web/exposed-window-or-worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,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
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/perf/usertiming.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const {
},
} = require('internal/errors');

const { structuredClone } = internalBinding('messaging');
const { structuredClone } = require('internal/worker/js_transferable');
const {
lazyDOMException,
kEnumerableProperty,
Expand Down
12 changes: 12 additions & 0 deletions lib/internal/webidl.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,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;

Expand Down Expand Up @@ -189,6 +199,8 @@ converters.DOMString = function DOMString(V) {
return String(V);
};

converters['sequence<object>'] = createSequenceConverter(converters.object);

function codedTypeError(message, errorProperties = kEmptyObject) {
// eslint-disable-next-line no-restricted-syntax
const err = new TypeError(message);
Expand Down
3 changes: 1 addition & 2 deletions lib/internal/webstreams/readablestream.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ const {
kTransfer,
kTransferList,
markTransferMode,
structuredClone,
} = require('internal/worker/js_transferable');

const {
Expand All @@ -88,8 +89,6 @@ const {
kControllerErrorFunction,
} = require('internal/streams/utils');

const { structuredClone } = internalBinding('messaging');

const {
ArrayBufferViewGetBuffer,
ArrayBufferViewGetByteLength,
Expand Down
37 changes: 37 additions & 0 deletions lib/internal/worker/js_transferable.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -11,6 +18,7 @@ const {
} = internalBinding('symbols');
const {
setDeserializerCreateObjectFunction,
structuredClone: nativeStructuredClone,
} = internalBinding('messaging');
const {
privateSymbols: {
Expand Down Expand Up @@ -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<object>'](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,
Expand Down
29 changes: 11 additions & 18 deletions src/node_messaging.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1578,28 +1578,21 @@ static void StructuredClone(const FunctionCallbackInfo<Value>& 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> 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<Object> options = args[1].As<Object>();
Local<Value> transfer_list_v;
if (!options->Get(context, env->transfer_string())
.ToLocal(&transfer_list_v)) {
return;
}
Local<Object> options = args[1].As<Object>();
Local<Value> 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<Array> arr = transfer_list_v.As<Array>();
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;
}
}
Expand Down
23 changes: 22 additions & 1 deletion test/parallel/test-structuredClone-global.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -43,6 +43,27 @@ for (const Transferrable of [File, Blob]) {
assert.ok(extendedTransfer instanceof Transferrable);
}

// Transfer can be iterable
{
const value = {
a: new ReadableStream(),
b: new WritableStream(),
};
const cloned = structuredClone(value, {
transfer: {
*[Symbol.iterator]() {
for (const key in value) {
yield value[key];
}
}
}
});
for (const key in value) {
assert.ok(value[key].locked);
assert.ok(!cloned[key].locked);
}
}

{
// See: https://github.com/nodejs/node/issues/49940
const cloned = structuredClone({}, {
Expand Down

0 comments on commit fc178ba

Please sign in to comment.