nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.32k stars 29.46k forks source link

Transfer should not be null or undefined in `structuredClone` #55280

Closed jazelly closed 1 week ago

jazelly commented 2 weeks ago

Version

22.9.0

Platform

N/A

Subsystem

No response

What steps will reproduce the bug?

// transfer should not be null
structuredClone(undefined, { transfer: null });

How often does it reproduce? Is there a required condition?

always

What is the expected behavior? Why is that the expected behavior?

It should throw TypeError like other runtimes.

What do you see instead?

it passed

Additional information

I didn't find any requirements on type checking, but seems like other runtimes unanimously perform the webidl sequence conversion for transfer. IMO, we should do the same.

There are two ways to do this.

One is quick win by updating the check here https://github.com/nodejs/node/blob/a301596c41c29bcea0b9803f41750f741f9f49be/src/node_messaging.cc#L1032

The other one is to perform WebIDL conversion at JS layer before hitting the native StructuredClone. I understand the current implementation has been completely moved to cc, so I am not sure if that approach would be more preferable, but it does benefit us from reducing the C++ to JS overhead inside this function https://github.com/nodejs/node/blob/a301596c41c29bcea0b9803f41750f741f9f49be/src/node_messaging.cc#L1045

jazelly commented 2 weeks ago

FWIW, on chrome or deno, you can see their attempt to webidl conversion. Here is the output from chrome

VM468:1 Uncaught TypeError: Failed to execute 'structuredClone' on 'Window': Failed to read the 'transfer' property from 'StructuredSerializeOptions': The provided value cannot be converted to a sequence.
    at <anonymous>:1:1