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

Expand JS error serialization #5749

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

Conversation

domenic
Copy link
Member

@domenic domenic commented Jul 21, 2020

  • Generalize the framework to work on any NativeError types introduced by the JS spec, instead of listing them explicitly in a way that could require future updates.
  • Also include WebAssembly Error classes.
  • Switch semantics for serializing/deserializing "message". Previously, we would check for the presence of the property, and if it was a data property, get its value, and then ToString() it. Now, we just Get() it and then structured-serialize the result. We also unconditionally install it on the other side, regardless of whether it was present on the original. This new property-cloning procedure is more general.
  • Use these new more general property cloning procedure on "cause" (for all errors) and "errors" (for objects that present as "AggregateError" objects).

Closes #5716.

(See WHATWG Working Mode: Changes for more details.)

Suggested tests for the semantics as specced here:

  • Expand the tests to cover WebAssembly.CompileError, WebAssembly.LinkError, and WebAssembly.RuntimeError.
  • Test the following for both message and cause (updating message tests as appropriate since we're changing the semantics):
    • Deleting the property from one side results in an own property with value undefined on the destination
    • Setting the property to something uncloneable results in a DataCloneError
    • Overwriting the property with a getter results in that getter being called and its return value being cloned
    • Overwriting the property's enumerable/configurable/writable-ness does not have any impact; the other side always gets nonenumerable/configurable/writable.
    • Setting the property to something complex-but-serializable (e.g. a Map) results in the value bieng cloned
  • Test all of the above for errors too, but only on AggregateError
  • errors specific tests:
    • Any errors property on a non-AggregateError is ignored, except:
    • Mutating a non-AggregateError to have .name === "AggregateError" makes errors get cloned (and the other side getting an AggregateError)

/infrastructure.html ( diff )
/references.html ( diff )
/structured-data.html ( diff )

@annevk
Copy link
Member

annevk commented Jul 22, 2020

Did you consider using deep? Would that also cover cases such as stack automatically?

@domenic
Copy link
Member Author

domenic commented Jul 22, 2020

Since these properties are non-enumerable, that would not work.

@annevk
Copy link
Member

annevk commented Jul 22, 2020

I guess we could add a branch to that algorithm though and keep a list of desired properties around in a different way.

@domenic
Copy link
Member Author

domenic commented Jul 22, 2020

That seems pretty indirect, compared to just serializing AggregateError-specific data in the AggregateError branch.

@annevk
Copy link
Member

annevk commented Jul 22, 2020

It is a bit, but if we want message, errors, and at some point stack it does kind of lean in that direction to me. And we probably want the same kind of value retrieval and copying semantics for these fields as we do for deep?

@domenic
Copy link
Member Author

domenic commented Jul 22, 2020

I don't think so. In particular the main characteristic of deep is that it iterates over an unbounded set of enumerable properties. If you don't reuse that part, then you're basically just "reusing" property gets, which is a lot of indirection to just get a one-liner. (Literally the only commonality is step 26.4.1.1; note that 26.4.1's predicate is only necessary because of the unbounded enumeration.)

@domenic
Copy link
Member Author

domenic commented Jul 22, 2020

If we did want consistency with deep though, then it would argue for my

If we were to change .message behavior, here are some potential semantics we could consider that would keep them in sync:

from the OP.

@domenic
Copy link
Member Author

domenic commented Aug 5, 2020

Ping @yutakahirano @evilpie for thoughts. Summary of the open question: The PR as-drafted contains reasonable semantics, but we could make things more reasonable if we also revisited how message is serialized. Is that churn to implementations + tests worth it, or should we just go ahead with the slight inconsistency?

source Outdated
<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 not one of "AggregateError", "Error", "EvalError", "RangeError",
"ReferenceError", "SyntaxError", "TypeError", or "URIError", then set <var>name</var> to
Copy link
Member

Choose a reason for hiding this comment

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

This sentence seems incomplete.

@yutakahirano
Copy link
Member

Ping @yutakahirano @evilpie for thoughts. Summary of the open question: The PR as-drafted contains reasonable semantics, but we could make things more reasonable if we also revisited how message is serialized. Is that churn to implementations + tests worth it, or should we just go ahead with the slight inconsistency?

The last discussion was: #4665 (comment) #4665 (comment). I think the argument is still valid.

I can implement this in either way, I believe.

@evilpie
Copy link

evilpie commented Aug 6, 2020

am I correct to assume that you filing the issue means you'd be interested in implementing in Firefox?)

I am not currently looking into implementing structured cloning for native errors. However if we are going to implement it, it would be better to have AggregateError squared away so that the on-disk format doesn't have to be changed again.

Base automatically changed from master to main Jan 15, 2021
@evilpie
Copy link

evilpie commented Jul 16, 2021

@domenic Is this still on your radar? We probably want to support the cause property now as well.

@domenic
Copy link
Member Author

domenic commented Jul 16, 2021

I got a bit discouraged by the lack of clear answer, and also the fact that the tests for error cloning are annoying to update.

I am willing to try again on the spec front if others do the tests and we can ensure implementation interest. It sounds like there's at least support from yourself and @yutakahirano for doing something here in the spec, even if the implementation priority is not very high.

As for what the spec should say: at this point I am convinced it should be more generic so that it can adapt to future such error extensions from TC39. I am not sure exactly how, but probably some recursive cloning of object properties (preserving their enumerable/writable/configurable attributes??) would be best. I'll try to write something up and see how I like it... stay tuned.

* Generalize the framework to work on any NativeError types introduced by the JS spec, instead of listing them explicitly in a way that could require future updates.
* Also include WebAssembly Error classes.
* Switch semantics for serializing/deserializing "message". Previously, we would check for the presence of the property, and if it was a data property, get its value, and then ToString() it. Now, we just Get() it and then structured-serialize the result. We also unconditionally install it on the other side, regardless of whether it was present on the original. This new property-cloning procedure is more general.
* Use these new more general property cloning procedure on "cause" (for all errors) and "errors" (for objects that present as "AggregateError" objects).

Closes #5716.
@domenic domenic force-pushed the serialize-aggregateerror branch from de9bc6e to 5da5838 Compare Jul 16, 2021
@domenic
Copy link
Member Author

domenic commented Jul 16, 2021

OK, I rewrote it to be reasonably general, although new properties will still need to get added one by one. I'll copy and paste the new commit description here and also into the OP:

  • Generalize the framework to work on any NativeError types introduced by the JS spec, instead of listing them explicitly in a way that could require future updates.
  • Also include WebAssembly Error classes.
  • Switch semantics for serializing/deserializing "message". Previously, we would check for the presence of the property, and if it was a data property, get its value, and then ToString() it. Now, we just Get() it and then structured-serialize the result. We also unconditionally install it on the other side, regardless of whether it was present on the original. This new property-cloning procedure is more general.
  • Use these new more general property cloning procedure on "cause" (for all errors) and "errors" (for objects that present as "AggregateError" objects).

Somewhat related: tc39/proposal-error-cause#35

This will require a lot of test updates, which I'm still not signing up for. I kind of wish the TC39 process did not get to go to stage 3 until we had this host integration figured out, or stage 4 until we had tests written; I'm talking with @syg about whether moving some of this responsibility into the ES spec might help with that.

@domenic domenic changed the title Support structured serialization of AggregateErrors Expand JS error serialization Jul 16, 2021
@evilpie
Copy link

evilpie commented Jul 16, 2021

At least for SpiderMonkey it would be nicer if message was either a String or undefined. We can of course just overwrite the message property to an arbitrary value, but the internal Error object design is meant for (nullable) strings.

johanholmerin added a commit to johanholmerin/structured-clone that referenced this pull request Jul 28, 2021
? <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

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>Perform ! <span>CreateNonEnumerableDataPropertyOrThrow</span>(<var>value</var>, "cause",
<var>cause</var>).</p></li>
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.

@zloirock zloirock mentioned this pull request Nov 4, 2021
27 tasks
@zloirock
Copy link

Hey, maybe makes sense to finish it before exposing structuredClone in all stable modern browsers?

@domenic
Copy link
Member Author

domenic commented Jan 13, 2022

We'd welcome your help in working on a spec that can get implementer interest, and/or doing such implementation work!

@zloirock
Copy link

@domenic this PR is already not so bad excepting some mentioned above minor moments.

@evilpie
Copy link

evilpie commented Jun 21, 2022

Fwiw. We landed serialization support for cause and errors (on AggregateError) in Firefox. https://bugzilla.mozilla.org/show_bug.cgi?id=1556604

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Moving the issue forward requires someone to write tests topic: serialize and transfer
Development

Successfully merging this pull request may close these issues.

Handle AggregateError in structured data cloning
6 participants