The Wayback Machine - https://web.archive.org/web/20220507195949/https://github.com/denoland/deno/issues/14296
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

It's wrong to transform all exceptions thrown during structured cloning to DataCloneError exceptions #14296

Open
andreubotella opened this issue Apr 16, 2022 · 2 comments
Open

It's wrong to transform all exceptions thrown during structured cloning to DataCloneError exceptions #14296

andreubotella opened this issue Apr 16, 2022 · 2 comments
Labels
bug good first issue web

Comments

@andreubotella
Copy link

@andreubotella andreubotella commented Apr 16, 2022 •
edited

Deno's implementation of the structured clone algorithm is divided into a JS part, and a native part which doesn't have access to DOMException. Since serialization errors must be represented by throwing a DataCloneError DOMException, the native part throws a TypeError with the required message, and the JS part rethrows any exception thrown by the serialization as a DataCloneError.

As shown by web-platform-tests/wpt#33663, however, this behavior is wrong, because exceptions thrown from an object's property getters should be surfaced without changes:

const testObject = {
  get testProperty() {
    throw new Error();
  }
};
try {
  structuredClone(testObject);
} catch(err) {
  console.log(err instanceof DOMException);  // should be false; Deno prints true
}

One way to fix this would be to have an optional third parameter to the internal API Deno.core.serialize that would be a (string) => any callback, returning the exception to throw. Or possibly (string) => never, having the callback itself throw the exception.
@lucacasonato lucacasonato added bug web good first issue labels Apr 18, 2022
@karol-janik
Copy link

@karol-janik karol-janik commented Apr 19, 2022

I'd like to work on this!
@andreubotella
Copy link
Author

@andreubotella andreubotella commented Apr 19, 2022 •
edited

I'd like to work on this!

Deno's implementation of structured cloning is built on top of the Rust functions serialize and deserialize in core/bindings.rs, which get exposed in JS as Deno.core.{,de}serialize. You'd have to get a third JS-side argument to serialize (with args.get(2)), turn it into a v8::Local<v8::Function>, and pass it as a field to the SerializeDeserialize struct so you can get it from the throw_data_clone_error method.

The code sample in the OP tests that non-serialization errors are rethrown unchanged, but this code sample tests that serialization errors are thrown correctly:

try {
  structuredClone(() => {});  // functions are not serializable
} catch(err) {
  console.log(err instanceof DOMException);  // should print true
  console.log(err.name);  // "DataCloneError"
  console.log(err.message);  // "()=>{} could not be cloned"
}

Or alternatively, you can test that your changes don't break any correct behavior by running the web platform tests for structured cloning: ./tools/wpt.ts run -- html/webappapis/structured-clone.

Be sure to check out the rusty_v8 rustdoc. And feel free to ask for help if you're stuck or don't understand something.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug good first issue web
3 participants
@lucacasonato @andreubotella @karol-janik