-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Define serialization and deserialization steps for Error #4665
Conversation
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.
I don't see deserialization included in this PR yet. I assume this is a WIP?
As in the first comment this change is not ready for review yet. |
Although I haven't written tests yet, I think the change is ready for review. I will write tests probably tomorrow. cc: @domenic |
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.
LGTM with some nits that I touched up; nice work!
<ol> | ||
<li><p>Let <var>prototype</var> be "Error".</p></li> | ||
|
||
<li><p>Let <var>valueProto</var> be ! <var>value</var>.[[GetPrototypeOf]]().</p></li> |
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.
Because of the preconditions, I am 95% sure this is equivalent to your version, which checked [[Prototype]] directly each time. It's just a bit nicer to go through the semi-public API of [[GetPrototypeOf]](), which will return [[Prototype]].
I'll note that I'm not attached to the current design for getting the message, which emerged through conversations in #4665 (comment). We could, for example, also refuse to serialize the message if it's a non-string. Or we could always serialize the message, including any on the prototype chain, which would result in no-message errors coming out the other side with empty-string messages. I don't feel strongly; the current PR just represents something that seems vaguely reasonable. |
Tests are available at web-platform-tests/wpt#17095. |
This is tests for spec changes. - whatwg/html#4665 - whatwg/webidl#732
This is a reland of https://crrev.com/c/v8/v8/+/1692366. The original change was reverted because it broke some blink tests. This will be landed after suppressing them: https://crrev.com/c/chromium/src/+/1695541 Make native errors serializable. The implementation is mostly straightforward, but there is one exception: the stack property. Although the property is not specified, the spec for error cloning asks us to preserve the property if possible. This implementation serializes the property only when it is a string, and otherwise ignores it. Spec: whatwg/html#4665 Intent-to-Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/f8JngIi8qYs Bug: chromium:970079, v8:9462 Change-Id: Ibf012754f30237f6b5acf119ef834e73727a230f Cq-Include-Trybots: luci.v8.try:v8_linux_blink_rel Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1695202 Auto-Submit: Yutaka Hirano <yhirano@chromium.org> Commit-Queue: Simon Zünd <szuend@chromium.org> Reviewed-by: Simon Zünd <szuend@chromium.org> Cr-Commit-Position: refs/heads/master@{#62659}
Fix tests and expectations for Error structured cloning Spec: whatwg/html#4665 Intent-to-Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/f8JngIi8qYs Bug: 970079 Change-Id: I5f3eb0c5f1a2aee11f1ec668bd9bfe1e0f6ace2e
Fix tests and expectations for Error structured cloning Spec: whatwg/html#4665 Intent-to-Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/f8JngIi8qYs Bug: 970079 Change-Id: I5f3eb0c5f1a2aee11f1ec668bd9bfe1e0f6ace2e
Fix tests and expectations for Error structured cloning Spec: whatwg/html#4665 Intent-to-Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/f8JngIi8qYs Bug: 970079 Change-Id: I5f3eb0c5f1a2aee11f1ec668bd9bfe1e0f6ace2e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1703530 Commit-Queue: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Joshua Bell <jsbell@chromium.org> Reviewed-by: Majid Valipour <majidvp@chromium.org> Cr-Commit-Position: refs/heads/master@{#678728}
Fix tests and expectations for Error structured cloning Spec: whatwg/html#4665 Intent-to-Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/f8JngIi8qYs Bug: 970079 Change-Id: I5f3eb0c5f1a2aee11f1ec668bd9bfe1e0f6ace2e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1703530 Commit-Queue: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Joshua Bell <jsbell@chromium.org> Reviewed-by: Majid Valipour <majidvp@chromium.org> Cr-Commit-Position: refs/heads/master@{#678728}
Fix tests and expectations for Error structured cloning Spec: whatwg/html#4665 Intent-to-Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/f8JngIi8qYs Bug: 970079 Change-Id: I5f3eb0c5f1a2aee11f1ec668bd9bfe1e0f6ace2e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1703530 Commit-Queue: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Joshua Bell <jsbell@chromium.org> Reviewed-by: Majid Valipour <majidvp@chromium.org> Cr-Commit-Position: refs/heads/master@{#678728}
…tacks, a=testonly Automatic update from web-platform-tests Add optional test for cloning of error stacks Supplements web-platform-tests/wpt#17095. Follows whatwg/html#4665 and whatwg/webidl#732. -- wpt-commits: 1da6bed5d8c4c38200383b86928b7be68bfb87da wpt-pr: 17159 --HG-- rename : testing/web-platform/tests/html/infrastructure/safe-passing-of-structured-data/shared-array-buffers/resources/echo-iframe.html => testing/web-platform/tests/html/infrastructure/safe-passing-of-structured-data/resources/echo-iframe.html rename : testing/web-platform/tests/html/infrastructure/safe-passing-of-structured-data/shared-array-buffers/resources/echo-worker.js => testing/web-platform/tests/html/infrastructure/safe-passing-of-structured-data/resources/echo-worker.js
…=testonly Automatic update from web-platform-tests Add structured clone tests for errors This is tests for spec changes. - whatwg/html#4665 - whatwg/webidl#732 -- Add tests for cloning from another realm -- Test some stuff before cloning -- wpt-commits: 1b4fbe0c7049d89fd805dc157f68d82616f5d203, efc79eef01ffa65273ff01736d989ff5fa79f18d, 84af6c875d378944b39d895acdcfc170736b2d3d wpt-pr: 17095
…Error structured cloning, a=testonly Automatic update from web-platform-tests Fix and re-enable web tests affected by Error structured cloning Fix tests and expectations for Error structured cloning Spec: whatwg/html#4665 Intent-to-Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/f8JngIi8qYs Bug: 970079 Change-Id: I5f3eb0c5f1a2aee11f1ec668bd9bfe1e0f6ace2e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1703530 Commit-Queue: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Joshua Bell <jsbell@chromium.org> Reviewed-by: Majid Valipour <majidvp@chromium.org> Cr-Commit-Position: refs/heads/master@{#678728} -- wpt-commits: c1e9969044fed6bf1bd9d0a5cb1630845cde70fb wpt-pr: 17872
…tacks, a=testonly Automatic update from web-platform-tests Add optional test for cloning of error stacks Supplements web-platform-tests/wpt#17095. Follows whatwg/html#4665 and whatwg/webidl#732. -- wpt-commits: 1da6bed5d8c4c38200383b86928b7be68bfb87da wpt-pr: 17159
…=testonly Automatic update from web-platform-tests Add structured clone tests for errors This is tests for spec changes. - whatwg/html#4665 - whatwg/webidl#732 -- Add tests for cloning from another realm -- Test some stuff before cloning -- wpt-commits: 1b4fbe0c7049d89fd805dc157f68d82616f5d203, efc79eef01ffa65273ff01736d989ff5fa79f18d, 84af6c875d378944b39d895acdcfc170736b2d3d wpt-pr: 17095
…Error structured cloning, a=testonly Automatic update from web-platform-tests Fix and re-enable web tests affected by Error structured cloning Fix tests and expectations for Error structured cloning Spec: whatwg/html#4665 Intent-to-Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/f8JngIi8qYs Bug: 970079 Change-Id: I5f3eb0c5f1a2aee11f1ec668bd9bfe1e0f6ace2e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1703530 Commit-Queue: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Joshua Bell <jsbell@chromium.org> Reviewed-by: Majid Valipour <majidvp@chromium.org> Cr-Commit-Position: refs/heads/master@{#678728} -- wpt-commits: c1e9969044fed6bf1bd9d0a5cb1630845cde70fb wpt-pr: 17872
Supplements web-platform-tests#17095. Follows whatwg/html#4665 and whatwg/webidl#732.
This is tests for spec changes. - whatwg/html#4665 - whatwg/webidl#732
Fix tests and expectations for Error structured cloning Spec: whatwg/html#4665 Intent-to-Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/f8JngIi8qYs Bug: 970079 Change-Id: I5f3eb0c5f1a2aee11f1ec668bd9bfe1e0f6ace2e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1703530 Commit-Queue: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Joshua Bell <jsbell@chromium.org> Reviewed-by: Majid Valipour <majidvp@chromium.org> Cr-Commit-Position: refs/heads/master@{#678728}
…tacks, a=testonly Automatic update from web-platform-tests Add optional test for cloning of error stacks Supplements web-platform-tests/wpt#17095. Follows whatwg/html#4665 and whatwg/webidl#732. -- wpt-commits: 1da6bed5d8c4c38200383b86928b7be68bfb87da wpt-pr: 17159 UltraBlame original commit: d0647ab7df5d71c33b5c35b8b4ce69940975639e
…=testonly Automatic update from web-platform-tests Add structured clone tests for errors This is tests for spec changes. - whatwg/html#4665 - whatwg/webidl#732 -- Add tests for cloning from another realm -- Test some stuff before cloning -- wpt-commits: 1b4fbe0c7049d89fd805dc157f68d82616f5d203, efc79eef01ffa65273ff01736d989ff5fa79f18d, 84af6c875d378944b39d895acdcfc170736b2d3d wpt-pr: 17095 UltraBlame original commit: 696cf52f476f2f644137d19d9b9c4ec4dc400731
…Error structured cloning, a=testonly Automatic update from web-platform-tests Fix and re-enable web tests affected by Error structured cloning Fix tests and expectations for Error structured cloning Spec: whatwg/html#4665 Intent-to-Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/f8JngIi8qYs Bug: 970079 Change-Id: I5f3eb0c5f1a2aee11f1ec668bd9bfe1e0f6ace2e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1703530 Commit-Queue: Yutaka Hirano <yhiranochromium.org> Reviewed-by: Joshua Bell <jsbellchromium.org> Reviewed-by: Majid Valipour <majidvpchromium.org> Cr-Commit-Position: refs/heads/master{#678728} -- wpt-commits: c1e9969044fed6bf1bd9d0a5cb1630845cde70fb wpt-pr: 17872 UltraBlame original commit: 245fb2b7a2561a3a123abc430714e85f363dbf8b
…tacks, a=testonly Automatic update from web-platform-tests Add optional test for cloning of error stacks Supplements web-platform-tests/wpt#17095. Follows whatwg/html#4665 and whatwg/webidl#732. -- wpt-commits: 1da6bed5d8c4c38200383b86928b7be68bfb87da wpt-pr: 17159 UltraBlame original commit: d0647ab7df5d71c33b5c35b8b4ce69940975639e
…=testonly Automatic update from web-platform-tests Add structured clone tests for errors This is tests for spec changes. - whatwg/html#4665 - whatwg/webidl#732 -- Add tests for cloning from another realm -- Test some stuff before cloning -- wpt-commits: 1b4fbe0c7049d89fd805dc157f68d82616f5d203, efc79eef01ffa65273ff01736d989ff5fa79f18d, 84af6c875d378944b39d895acdcfc170736b2d3d wpt-pr: 17095 UltraBlame original commit: 696cf52f476f2f644137d19d9b9c4ec4dc400731
…Error structured cloning, a=testonly Automatic update from web-platform-tests Fix and re-enable web tests affected by Error structured cloning Fix tests and expectations for Error structured cloning Spec: whatwg/html#4665 Intent-to-Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/f8JngIi8qYs Bug: 970079 Change-Id: I5f3eb0c5f1a2aee11f1ec668bd9bfe1e0f6ace2e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1703530 Commit-Queue: Yutaka Hirano <yhiranochromium.org> Reviewed-by: Joshua Bell <jsbellchromium.org> Reviewed-by: Majid Valipour <majidvpchromium.org> Cr-Commit-Position: refs/heads/master{#678728} -- wpt-commits: c1e9969044fed6bf1bd9d0a5cb1630845cde70fb wpt-pr: 17872 UltraBlame original commit: 245fb2b7a2561a3a123abc430714e85f363dbf8b
…tacks, a=testonly Automatic update from web-platform-tests Add optional test for cloning of error stacks Supplements web-platform-tests/wpt#17095. Follows whatwg/html#4665 and whatwg/webidl#732. -- wpt-commits: 1da6bed5d8c4c38200383b86928b7be68bfb87da wpt-pr: 17159 UltraBlame original commit: d0647ab7df5d71c33b5c35b8b4ce69940975639e
…=testonly Automatic update from web-platform-tests Add structured clone tests for errors This is tests for spec changes. - whatwg/html#4665 - whatwg/webidl#732 -- Add tests for cloning from another realm -- Test some stuff before cloning -- wpt-commits: 1b4fbe0c7049d89fd805dc157f68d82616f5d203, efc79eef01ffa65273ff01736d989ff5fa79f18d, 84af6c875d378944b39d895acdcfc170736b2d3d wpt-pr: 17095 UltraBlame original commit: 696cf52f476f2f644137d19d9b9c4ec4dc400731
…Error structured cloning, a=testonly Automatic update from web-platform-tests Fix and re-enable web tests affected by Error structured cloning Fix tests and expectations for Error structured cloning Spec: whatwg/html#4665 Intent-to-Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/f8JngIi8qYs Bug: 970079 Change-Id: I5f3eb0c5f1a2aee11f1ec668bd9bfe1e0f6ace2e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1703530 Commit-Queue: Yutaka Hirano <yhiranochromium.org> Reviewed-by: Joshua Bell <jsbellchromium.org> Reviewed-by: Majid Valipour <majidvpchromium.org> Cr-Commit-Position: refs/heads/master{#678728} -- wpt-commits: c1e9969044fed6bf1bd9d0a5cb1630845cde70fb wpt-pr: 17872 UltraBlame original commit: 245fb2b7a2561a3a123abc430714e85f363dbf8b
Define serialization and deserialization steps for Error objects. This change doesn't cover DOMExceptions, which will be covered by whatwg/webidl#729.
Fixes #4268 in combination with whatwg/webidl#729.
(See WHATWG Working Mode: Changes for more details.)
/infrastructure.html ( diff )
/references.html ( diff )
/structured-data.html ( diff )