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

Expand JS error serialization #5749

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Expand JS error serialization #5749

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
110 changes: 65 additions & 45 deletions source
View file
Open in desktop
Expand Up @@ -2764,19 +2764,15 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute
<dfn>@@isConcatSpreadable</dfn>,
<dfn>@@toPrimitive</dfn>, and
<dfn>@@toStringTag</dfn></li>
<li><dfn data-x-href="https://tc39.es/ecma262/#sec-native-error-types-used-in-this-standard"><var>NativeError</var></dfn></li>
<li><dfn data-x-href="https://tc39.es/ecma262/#sec-well-known-intrinsic-objects">Well-Known Intrinsic Objects</dfn>, including
<dfn data-x-href="https://tc39.es/ecma262/#sec-properties-of-the-array-prototype-object">%Array.prototype%</dfn>,
<dfn data-x-href="https://tc39.es/ecma262/#sec-properties-of-the-error-prototype-object">%Error.prototype%</dfn>,
<dfn>%EvalError.prototype%</dfn>,
<dfn data-x-href="https://tc39.es/ecma262/#sec-properties-of-the-aggregate-error-prototype-objects">%AggregateError.prototype%</dfn>,
<dfn data-x-href="https://tc39.es/ecma262/#sec-properties-of-the-function-prototype-object">%Function.prototype%</dfn>,
<dfn data-x-href="https://tc39.es/ecma262/#sec-json.parse">%JSON.parse%</dfn>,
<dfn data-x-href="https://tc39.es/ecma262/#sec-properties-of-the-object-prototype-object">%Object.prototype%</dfn>,
<dfn data-x-href="https://tc39.es/ecma262/#sec-object.prototype.valueof">%Object.prototype.valueOf%</dfn>,
<dfn>%RangeError.prototype%</dfn>,
<dfn>%ReferenceError.prototype%</dfn>,
<dfn>%SyntaxError.prototype%</dfn>,
<dfn>%TypeError.prototype%</dfn>, and
<dfn>%URIError.prototype%</dfn></li>
<dfn data-x-href="https://tc39.es/ecma262/#sec-object.prototype.valueof">%Object.prototype.valueOf%</dfn>.</li>

<li>The <dfn data-x="js-prod-FunctionBody" data-x-href="https://tc39.es/ecma262/#prod-FunctionBody"><i>FunctionBody</i></dfn> production</li>
<li>The <dfn data-x="js-prod-Module" data-x-href="https://tc39.es/ecma262/#prod-Module"><i>Module</i></dfn> production</li>
Expand Down Expand Up @@ -2890,6 +2886,14 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute
<li>The <dfn data-x="js-HostGetSupportedImportAssertions" data-x-href="https://tc39.es/proposal-import-assertions/#sec-hostgetsupportedimportassertions">HostGetSupportedImportAssertions</dfn> abstract operation</li>
</ul>

<p>User agents that support JavaScript must also implement the <cite>Error Cause</cite>
proposal. The following terms are defined there, and used in this specification: <ref
spec=JSERRORCAUSE></p>

<ul class="brief">
<li><dfn data-x-href="https://tc39.es/proposal-error-cause/#sec-createnonenumerabledatapropertyorthrow">CreateNonEnumerableDataPropertyOrThrow</dfn></li>
</ul>

<p>The following terms are defined in the <cite>JSON modules</cite> proposal and used in this
specification: <ref spec=JSJSONMODULES></p>

Expand All @@ -2908,6 +2912,7 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute

<ul class="brief">
<li><dfn data-x-href="https://webassembly.github.io/spec/js-api/#module"><code>WebAssembly.Module</code></dfn></li>
<li><dfn data-x-href="https://webassembly.github.io/spec/js-api/#error-objects">WebAssembly Error class</dfn></li>
</ul>
</dd>

Expand Down Expand Up @@ -8325,26 +8330,35 @@ interface <dfn interface>DOMStringList</dfn> {
<li><p>Let <var>name</var> be ? <span data-x="js-Get">Get</span>(<var>value</var>,
"name").</p></li>

<li><p>If <var>name</var> is not one of "Error", "EvalError", "RangeError", "ReferenceError",
"SyntaxError", "TypeError", or "URIError", then set <var>name</var> to "Error".</p></li>
<li><p>If <var>name</var> is neither "AggregateError", nor one of the
<span><var>NativeError</var></span> names, nor one of the <span>WebAssembly Error class</span>
names, then set <var>name</var> to "Error".</p></li>

<li><p>Let <var>message</var> be ? <span>StructuredSerializeInternal</span>(? <span
data-x="js-Get">Get</span>(<var>value</var>, "message"), <var>forStorage</var>,
<var>memory</var>).</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

TBH I'm not sure how much this message behavior change is useful here, I'd be slightly happier if this is split to a separate PR/discussion.

Given that we lose random property e.g. err.foo, I think it's not too inconsistent to lose some information here (as both are set outside of the constructor in this case).

Copy link
Member

Choose a reason for hiding this comment

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

Are you suggesting we change what we do for objects containing [[ErrorData]] today?

Copy link
Member

Choose a reason for hiding this comment

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

Not really, just saying we don't keep all manually-set properties too hard. Right?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should be consistent across error objects on copying message. And we copy other fields such as stack too, so that doesn't seem like something we should be changing here.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't toString() behavior what the existing spec says and browsers do currently?

And we copy other fields such as stack too,

Hmm, quickly checked and it seems Gecko somehow decided not to copy the stack property but to copy the internal stack data. @evilpie, do you remember the reasoning behind that decision?

Copy link

Choose a reason for hiding this comment

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

We need to copy the internal stack so that it works properly with all other consumers in Gecko, which don't want to reparse the stack from a string. It's also required for hiding cross-origin stack frames in content and still having them visible for more privileged code.

Copy link
Member

Choose a reason for hiding this comment

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

I see now about message, my bad. I'm not sure I care strongly, as long as it's copied somehow.

I think copying the internal stack data is correct. We tend to prefer serializing internal slots over public data.

Copy link
Member

@saschanaz saschanaz Jun 5, 2023 •
edited

Choose a reason for hiding this comment

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

Oh, and I just noticed that the PR does not use Get() for stack but only for cause and message, which sounds correct 👍 (with the corresponding WPT here for cross origin case.)


<li><p>Let <var>cause</var> be ? <span>StructuredSerializeInternal</span>(? <span
data-x="js-Get">Get</span>(<var>value</var>, "cause"), <var>forStorage</var>,
<var>memory</var>).</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

cause should be conditionally installed as noted below. Firefox adds "hasCause" for this purpose, maybe the same can be done here.

(But it's never been crystal clear to me how it's supposed to be different to not have the property and to have undefined here, maybe some relevant content in MDN would be helpful.)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's different because you can throw undefined, and then undefined might be the cause, and that's different from an error with no cause.


<li><p>Let <var>valueMessageDesc</var> be ? <var>value</var>.[[GetOwnProperty]]("<code
data-x="">message</code>").</p></li>
<li><p>Let <var>errors</var> be undefined.</p></li>

<li><p>Let <var>message</var> be undefined if
<span>IsDataDescriptor</span>(<var>valueMessageDesc</var>) is false, and
? <span>ToString</span>(<var>valueMessageDesc</var>.[[Value]]) otherwise.</p></li>
<li><p>If <var>name</var> is "AggregateError", then set <var>errors</var> to ?
<span>StructuredSerializeInternal</span>(? <span data-x="js-Get">Get</span>(<var>value</var>,
"errors"), <var>forStorage</var>, <var>memory</var>).</p></li>
Copy link

@zloirock zloirock Nov 4, 2021 •
edited

Choose a reason for hiding this comment

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

Maybe it's better to handle it after caching copy in the memory for avoiding problems with circular dependencies?

Copy link

Choose a reason for hiding this comment

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

const e = AggregateError([]);
e.errors.push(e);
structuredClone(e);


<li><p>Set <var>serialized</var> to { [[Type]]: "Error", [[Name]]: <var>name</var>,
[[Message]]: <var>message</var> }.</p></li>
[[Message]]: <var>message</var>, [[Cause]]: <var>cause</var>, [[Errors]]: <var>errors</var>
}.</p></li>

<li>
<p>User agents should attach a serialized representation of any interesting accompanying
data which are not yet specified, notably the <code data-x="">stack</code> property, to
<p>User agents should attach a serialized representation of any interesting accompanying data
which are not yet specified, notably the <code data-x="">stack</code> property, to
<var>serialized</var>.</p>

<p class="note">See the <cite>Error Stacks</cite> proposal for in-progress work on
specifying this data. <ref spec=JSERRORSTACKS></p>
<p class="note">See the <cite>Error Stacks</cite> proposal for in-progress work on specifying
this data. <ref spec=JSERRORSTACKS></p>
</li>
</ol>
</li>
Expand Down Expand Up @@ -8520,8 +8534,8 @@ interface <dfn interface>DOMStringList</dfn> {
<p>If ! <span>HasOwnProperty</span>(<var>value</var>, <var>key</var>) is true, then:</p>

<ol>
<li><p>Let <var>inputValue</var> be ? <var>value</var>.[[Get]](<var>key</var>,
<var>value</var>).</p></li>
<li><p>Let <var>inputValue</var> be ? <span data-x="js-Get">Get</span>(<var>value</var>,
<var>key</var>).</p></li>

<li><p>Let <var>outputValue</var> be ?
<span>StructuredSerializeInternal</span>(<var>inputValue</var>, <var>forStorage</var>,
Expand Down Expand Up @@ -8737,38 +8751,41 @@ o.myself = o;</code></pre>
<p>Otherwise, if <var>serialized</var>.[[Type]] is "Error", then:</p>

<ol>
<li><p>Let <var>prototype</var> be <span>%Error.prototype%</span>.</p></li>

<li><p>If <var>serialized</var>.[[Name]] is "EvalError", then set <var>prototype</var> to
<span>%EvalError.prototype%</span>.</p></li>
<li><p>Let <var>name</var> be <var>serialized</var>.[[Name]].</p></li>

<li><p>If <var>serialized</var>.[[Name]] is "RangeError", then set <var>prototype</var>
to <span>%RangeError.prototype%</span>.</p></li>
<li><p>Let <var>prototype</var> be
<var>targetRealm</var>.[[Intrinsics]].[[%<var>name</var>%.prototype]], or the equivalent if
<var>name</var> is a <span>WebAssembly Error class</span> name.</p></li>

<li><p>If <var>serialized</var>.[[Name]] is "ReferenceError", then set
<var>prototype</var> to <span>%ReferenceError.prototype%</span>.</p></li>
<li><p>Set <var>value</var> to ! <span>ObjectCreate</span>(<var>prototype</var>, «
[[ErrorData]] »).</p></li>

<li><p>If <var>serialized</var>.[[Name]] is "SyntaxError", then set <var>prototype</var>
to <span>%SyntaxError.prototype%</span>.</p></li>
<li><p>Let <var>message</var> be ?
<span>StructuredDeserialize</span>(<var>serialized</var>.[[Message]], <var>targetRealm</var>,
<var>memory</var>).</p></li>

<li><p>If <var>serialized</var>.[[Name]] is "TypeError", then set <var>prototype</var> to
<span>%TypeError.prototype%</span>.</p></li>
<li><p>Perform ! <span>CreateNonEnumerableDataPropertyOrThrow</span>(<var>value</var>,
"message", <var>message</var>).</p></li>

<li><p>If <var>serialized</var>.[[Name]] is "URIError", then set <var>prototype</var> to
<span>%URIError.prototype%</span>.</p></li>
<li><p>Let <var>cause</var> be ?
<span>StructuredDeserialize</span>(<var>serialized</var>.[[Cause]], <var>targetRealm</var>,
<var>memory</var>).</p></li>

<li><p>Let <var>message</var> be <var>serialized</var>.[[Message]].</p></li>
<li><p>Perform ! <span>CreateNonEnumerableDataPropertyOrThrow</span>(<var>value</var>, "cause",
<var>cause</var>).</p></li>
Comment on lines +8774 to +8775
Copy link

Choose a reason for hiding this comment

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

Maybe it's better to install .cause only when it's present on the source? Error constructors do not install it if it's not present in options.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is an important part of the spec for error causes; a cause of undefined is quite different from an absent cause.


<li><p>Set <var>value</var> to ! <span>ObjectCreate</span>(<var>prototype</var>, «
[[ErrorData]] »).</p></li>
<li>
<p>If <var>name</var> is "AggregateError", then:</p>

<li><p>Let <var>messageDesc</var> be <span>PropertyDescriptor</span>{ [[Value]]:
<var>message</var>, [[Writable]]: true, [[Enumerable]]: false, [[Configurable]]: true
}.</p></li>
<ol>
<li><p>Let <var>errors</var> be ?
<span>StructuredDeserialize</span>(<var>serialized</var>.[[Errors]], <var>targetRealm</var>,
<var>memory</var>).</p></li>

<li><p>If <var>message</var> is not undefined, then perform !
<span>OrdinaryDefineOwnProperty</span>(<var>value</var>, "<code data-x="">message</code>",
<var>messageDesc</var>).</p></li>
<li><p>Perform ! <span>CreateNonEnumerableDataPropertyOrThrow</span>(<var>value</var>,
"errors", <var>errors</var>).</p></li>
</ol>
</li>

<li><p>Any interesting accompanying data attached to <var>serialized</var> should be
deserialized and attached to <var>value</var>.</p></li>
Expand Down Expand Up @@ -125405,6 +125422,9 @@ INSERT INTERFACES HERE
<dt id="refsJPEG">[JPEG]</dt>
<dd><cite><a href="https://www.w3.org/Graphics/JPEG/jfif3.pdf">JPEG File Interchange Format</a></cite>, E. Hamilton.</dd>

<dt id="refsJSERRORCAUSE">[JSERRORCAUSE]</dt>
<dd><cite><a href="https://tc39.es/proposal-error-cause/">Error Cause</a></cite>. Ecma International.</dd>

<dt id="refsJSERRORSTACKS">[JSERRORSTACKS]</dt>
<dd>(Non-normative) <cite><a href="https://tc39.es/proposal-error-stacks/">Error Stacks</a></cite>. Ecma International.</dd>

Expand Down Expand Up @@ -125671,7 +125691,7 @@ INSERT INTERFACES HERE
<dd><cite><a href="https://wicg.github.io/uuid/">uuid</a></cite>, B. Coe, R. Kieffer, C. Tavan. WICG.</dd>

<dt id="refsWASMJS">[WASMJS]</dt>
<dd>(Non-normative) <cite><a href="https://webassembly.github.io/spec/js-api/">WebAssembly JavaScript Interface</a></cite>, D. Ehrenberg. W3C.</dd>
<dd><cite><a href="https://webassembly.github.io/spec/js-api/">WebAssembly JavaScript Interface</a></cite>, D. Ehrenberg. W3C.</dd>

<dt id="refsWCAG">[WCAG]</dt>
<dd>(Non-normative) <cite><a href="https://w3c.github.io/wcag/guidelines/">Web Content Accessibility Guidelines (WCAG)</a></cite>, A. Kirkpatrick, J. O Connor, A. Campbell, M. Cooper. W3C.</dd>
Expand Down