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.

Sign up for GitHub

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

url: conform structuredClone url serialization #47214

Closed
wants to merge 1 commit into from
Closed

url: conform structuredClone url serialization #47214

wants to merge 1 commit into from
+34 −3

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Mar 22, 2023 •
edited
Loading

Enabled 2 more web platform tests for URL and URLSearchParams for disallowing them to be called in structuredClone. cc @nodejs/url

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/url

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Mar 22, 2023
@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 22, 2023
@targos
Copy link
Member

targos commented Mar 22, 2023

I think this should be semver-major (with something in the doc/history).
I know that the clone doesn't produce something useful, but this change can break working code that clones deep objects (and doesn't care about URL objects in there)

targos
targos requested changes Mar 22, 2023
View reviewed changes
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isURL isn't precise enough

@anonrig
Copy link
Member Author

anonrig commented Mar 22, 2023

isURL isn't precise enough

@targos The documentation refers to the original issue. Do you have any recommendations?

Using a symbol or instanceof would not be able to recognize URL objects
coming from other implementations (e.g. in Electron), so instead we are
checking some well known properties for a lack of a better test.

@targos
Copy link
Member

targos commented Mar 22, 2023

I don't have a suggestion right now.
Also I think your implementation here actually only catches urls passed directly to the structuredClone function, and not in objects.
OTOH structuredClone({test: new URL('https://example.com')}) throws in a browser.

@legendecas
Copy link
Member

We can tag URL objects as platform objects so that they can be rejected by v8::Serializer with node's SerializerDelegate. Managed to come up with a demo: https://github.com/legendecas/node/tree/platform-object-brand (note: it still fails constructor prototype idl checks, and needs more tweaking).

@legendecas
Copy link
Member

A better solution can be delegating custom JS object serialization with v8's value serializer: https://chromium-review.googlesource.com/c/v8/v8/+/4385565 so that we don't have to alter the prototype chain of URL.

@anonrig
Copy link
Member Author

anonrig commented Apr 6, 2023

A better solution can be delegating custom JS object serialization with v8's value serializer: https://chromium-review.googlesource.com/c/v8/v8/+/4385565 so that we don't have to alter the prototype chain of URL.

@legendecas Thank you for working on this. I really like this approach. The previous approach of tagging each prototype with webidl would degregate the performance a lot.

@legendecas legendecas mentioned this pull request Apr 10, 2023
Merged
@anonrig
Copy link
Member Author

anonrig commented Apr 13, 2023

I'm closing this pull request in favor of @legendecas's pull request. Thanks!

@anonrig anonrig closed this Apr 13, 2023
nodejs-github-bot pushed a commit that referenced this pull request Jul 15, 2023
@legendecas
lib: mark URL/URLSearchParams as uncloneable and untransferable
c5b0b89 
Mark URL/URLSearchParams as uncloneable and untransferable to reject
them in `structuredClone` and `port.postMessage`.

PR-URL: #47497
Refs: #47214
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@legendecas @Ceres6
lib: mark URL/URLSearchParams as uncloneable and untransferable
2425f2a 
Mark URL/URLSearchParams as uncloneable and untransferable to reject
them in `structuredClone` and `port.postMessage`.

PR-URL: nodejs#47497
Refs: nodejs#47214
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@legendecas @Ceres6
lib: mark URL/URLSearchParams as uncloneable and untransferable
06a0358 
Mark URL/URLSearchParams as uncloneable and untransferable to reject
them in `structuredClone` and `port.postMessage`.

PR-URL: nodejs#47497
Refs: nodejs#47214
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos requested changes

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@anonrig @nodejs-github-bot @targos @legendecas