-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Initial pass at rewriting the structured clone algorithm #727
Conversation
I would love review from @dslomov and @ajklein. @heycam might also want to take a look since this removes any IDL annotations for transferable objects, relying on prose instead. Some xref'ing to ECMAScript might be broken/missing still. Would love to land #638 first since it puts down some more infrastructure for that. Summary of changes:
I haven't used the new internal methods/slots for ECMAScript objects per @domenic's feedback. The preference was to have one giant monkey patch over lots of small ones (he worded this differently, see #697 ). |
Hmm, this diff is unfortunate. Maybe it would be better with one commit that moves stuff around, then another that rewrites the structured clone section? |
<li><p>Let <var>transfer map</var> be the <i>transfer map</i> passed to the algorithm, if any, or | ||
the empty list otherwise.</p></li> | ||
<p>IDL-defined objects must define the [[Clone]] internal method such that it either throws or | ||
returns a copy of <b>this</b>, created in <var>targetRealm</var>.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy -> clone? Maybe add some sentence about how what "cloning" (or "copying") means is up to the object and what it thinks is best?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
The problem is that the second sentence is not a complete sentence. |
Last nits:
needs to become two steps so you can do
needs ! LGTM otherwise, feel free to squash and merge with those fixed. |
93d0a9a
to
6b2644d
Compare
@domenic why would OrdinaryGetOwnProperty need !? |
Because it returns a completion record and we need to show that it doesn't fail? I guess ES is inconsistent about this; only one of its call sites does the explicit assert. Probably all of them should though... |
It doesn't return a completion record though. It only ever returns a record or undefined, never a completion record. (I have a PR outstanding against ECMAScript to remove the existing ! usage.) |
Hmm I was kind of under the impression that per https://tc39.github.io/ecma262/#sec-implicit-completion-values all abstract operations returned completion records, but OK, I guess it makes sense for this one to not do so. |
Thanks (note that we have quite a bit of existing usage that does not use it either). |
This is based on https://github.com/dslomov/ecmascript-structured-clone with changes to account for changes in ECMAScript and feedback in #697. This fixes #697 and also fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=27031. This should make it much more explicit how structured cloning actually works and also provides clear hooks for other standards to use to make their objects cloneable and transferable.
6b2644d
to
bfb960c
Compare
\o/ |
Fixes whatwg#981. Was introduced by whatwg#727 while rewriting part of the algorithm.
Currently attempting to transfer non-transferable objects using the structured clone algorithm throws a `TypeError`, rather than a `DataCloneError` `DOMException` as per the spec. This seems like a holdover from the original definition of transferable objects as the `Transferable` WebIDL type, since transfer lists would be defined as `sequence<Transferable>` and so would throw a `TypeError` on conversion. whatwg/html#727 changed transfer lists to be `sequence<object>` and changed the `StructuredCloneWithTransfer` algorithm (now `StructuredSerializeWithTransfer`) to throw a `DataCloneError` `DOMException` if any element in the transfer list is not transferable. Bug: 816447 Change-Id: I9f4f88a2d7a593766f394256606a688f299c7b57
Currently attempting to transfer non-transferable objects using the structured clone algorithm throws a `TypeError`, rather than a `DataCloneError` `DOMException` as per the spec. This seems like a holdover from the original definition of transferable objects as the `Transferable` WebIDL type, since transfer lists would be defined as `sequence<Transferable>` and so would throw a `TypeError` on conversion. whatwg/html#727 changed transfer lists to be `sequence<object>` and changed the `StructuredCloneWithTransfer` algorithm (now `StructuredSerializeWithTransfer`) to throw a `DataCloneError` `DOMException` if any element in the transfer list is not transferable. Bug: 816447 Change-Id: I9f4f88a2d7a593766f394256606a688f299c7b57 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3229544 Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/heads/main@{#934398}
Currently attempting to transfer non-transferable objects using the structured clone algorithm throws a `TypeError`, rather than a `DataCloneError` `DOMException` as per the spec. This seems like a holdover from the original definition of transferable objects as the `Transferable` WebIDL type, since transfer lists would be defined as `sequence<Transferable>` and so would throw a `TypeError` on conversion. whatwg/html#727 changed transfer lists to be `sequence<object>` and changed the `StructuredCloneWithTransfer` algorithm (now `StructuredSerializeWithTransfer`) to throw a `DataCloneError` `DOMException` if any element in the transfer list is not transferable. Bug: 816447 Change-Id: I9f4f88a2d7a593766f394256606a688f299c7b57 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3229544 Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/heads/main@{#934398}
Currently attempting to transfer non-transferable objects using the structured clone algorithm throws a `TypeError`, rather than a `DataCloneError` `DOMException` as per the spec. This seems like a holdover from the original definition of transferable objects as the `Transferable` WebIDL type, since transfer lists would be defined as `sequence<Transferable>` and so would throw a `TypeError` on conversion. whatwg/html#727 changed transfer lists to be `sequence<object>` and changed the `StructuredCloneWithTransfer` algorithm (now `StructuredSerializeWithTransfer`) to throw a `DataCloneError` `DOMException` if any element in the transfer list is not transferable. Bug: 816447 Change-Id: I9f4f88a2d7a593766f394256606a688f299c7b57 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3229544 Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/heads/main@{#934398}
… non-transferable objects, a=testonly Automatic update from web-platform-tests Throw `DataCloneError` when transferring non-transferable objects Currently attempting to transfer non-transferable objects using the structured clone algorithm throws a `TypeError`, rather than a `DataCloneError` `DOMException` as per the spec. This seems like a holdover from the original definition of transferable objects as the `Transferable` WebIDL type, since transfer lists would be defined as `sequence<Transferable>` and so would throw a `TypeError` on conversion. whatwg/html#727 changed transfer lists to be `sequence<object>` and changed the `StructuredCloneWithTransfer` algorithm (now `StructuredSerializeWithTransfer`) to throw a `DataCloneError` `DOMException` if any element in the transfer list is not transferable. Bug: 816447 Change-Id: I9f4f88a2d7a593766f394256606a688f299c7b57 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3229544 Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/heads/main@{#934398} -- wpt-commits: 4287b41333755fc3a4909a06ecb0780623c637fc wpt-pr: 31328
… non-transferable objects, a=testonly Automatic update from web-platform-tests Throw `DataCloneError` when transferring non-transferable objects Currently attempting to transfer non-transferable objects using the structured clone algorithm throws a `TypeError`, rather than a `DataCloneError` `DOMException` as per the spec. This seems like a holdover from the original definition of transferable objects as the `Transferable` WebIDL type, since transfer lists would be defined as `sequence<Transferable>` and so would throw a `TypeError` on conversion. whatwg/html#727 changed transfer lists to be `sequence<object>` and changed the `StructuredCloneWithTransfer` algorithm (now `StructuredSerializeWithTransfer`) to throw a `DataCloneError` `DOMException` if any element in the transfer list is not transferable. Bug: 816447 Change-Id: I9f4f88a2d7a593766f394256606a688f299c7b57 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3229544 Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/heads/main@{#934398} -- wpt-commits: 4287b41333755fc3a4909a06ecb0780623c637fc wpt-pr: 31328
Currently attempting to transfer non-transferable objects using the structured clone algorithm throws a `TypeError`, rather than a `DataCloneError` `DOMException` as per the spec. This seems like a holdover from the original definition of transferable objects as the `Transferable` WebIDL type, since transfer lists would be defined as `sequence<Transferable>` and so would throw a `TypeError` on conversion. whatwg/html#727 changed transfer lists to be `sequence<object>` and changed the `StructuredCloneWithTransfer` algorithm (now `StructuredSerializeWithTransfer`) to throw a `DataCloneError` `DOMException` if any element in the transfer list is not transferable. Bug: 816447 Change-Id: I9f4f88a2d7a593766f394256606a688f299c7b57 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3229544 Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/heads/main@{#934398}
Currently attempting to transfer non-transferable objects using the structured clone algorithm throws a `TypeError`, rather than a `DataCloneError` `DOMException` as per the spec. This seems like a holdover from the original definition of transferable objects as the `Transferable` WebIDL type, since transfer lists would be defined as `sequence<Transferable>` and so would throw a `TypeError` on conversion. whatwg/html#727 changed transfer lists to be `sequence<object>` and changed the `StructuredCloneWithTransfer` algorithm (now `StructuredSerializeWithTransfer`) to throw a `DataCloneError` `DOMException` if any element in the transfer list is not transferable. Bug: 816447 Change-Id: I9f4f88a2d7a593766f394256606a688f299c7b57 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3229544 Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/heads/main@{#934398} NOKEYCHECK=True GitOrigin-RevId: ece66bb8e793ac03d11fe8f0b52f79bc1a030519
See https://html5.org/temp/structuredclone.html#safe-passing-of-structured-data for a more
reviewable version of this.
This is based on https://github.com/dslomov/ecmascript-structured-clone with changes to account for
changes in ECMAScript and feedback in #697.
This fixes #697 and also https://www.w3.org/Bugs/Public/show_bug.cgi?id=27031.