Normative: fix #353 - propagate error details across realms when possible

[caridy]

This PR introduces 2 normative changes (needs consensus):

[ ] CreateTypeErrorCopy abstract operation must not cause any ECMAScript code execution when creating a new message and stack.
[ ] censor the error's stack if the host is providing such information inside ShadowRealms.

Refactor PR to make it simpler to recognize when an error must be copied over the callable boundary into the caller realm, whether that's a result of evaluation, importValue or just calling a wrapped function.

Additionally, this PR defines when to censor the error's stack if the host is providing such information.

[caridy]

/cc @littledan

[ljharb]

what about an AggregateError's errors?

[littledan]

Looks great. Just a few changes I'd make:

• Remove the cause handling, since that tends to be an object anyway. And don't add AggregateError for the same reason. More advanced handling can be done by a membrane.
• Optional, feel free to decide against it: Check that it's a string, rather than doing ToString with its error recovery, for simplicity.
• Somehow note what implementations are supposed to do with their implementation-specific stack introspection APIs.

[caridy]

@littledan @ljharb I have made the suggested changes.

[caridy]

@littledan what do you think about this note?

[ljharb]

This note is unclear as to whether implementations can, or should, or must not, allow stack information from inside the ShadowRealm to be copied across.

[littledan]

Agreed, this doesn't answer the question at all. Implementers have asked us for guidance. What is our concrete recommendation to them?

[ljharb]

This note is unclear as to whether implementations can, or should, or must not, allow stack information from inside the ShadowRealm to be copied across.

[ljharb]

this should use ToString - message is almost never not a string, and when it is, users will expect that coercion since everything in the language does it.

[littledan]

I agree with you that message is almost never not a string, so we're talking about a low-priority edge case in terms of the developer impact. In general, when crossing a ShadowRealm boundary, we don't do coercions, we just throw when things don't work out. A coercion creates extra complexity, e.g., in error recovery. This is why I proposed the semantics that Caridy used, in a comment above. I don't feel extremely strongly about this, though; I'd be OK if this switched back to the previously proposed semantics.

[caridy]

another option here is to implement the semantics of the callable boundary, installing a wrapped accessor that eventually calls Get on the original object. In which case, if the message is not primitive, it will throw. The two benefits there are: 1) not need to touch the error obj, 2) it not only allows strings, but any other primitive, as data or accessor on the descriptor. On the other hand, it has 1 drawback: the new error to have one accessor, which is not common, and makes the error rather predictable as an error from boundary. Please, advice on what route to take.

[ljharb]

tbh that seems weirder than calling Get and just allowing any value (thus objects would throw) as a data property

[littledan]

We need to close on both this question Jordan asks, as well as the suggestion from @mhofman that we avoid a Get and use a GetOwnProperty instead. I'm happy with this current spec text though :)

[caridy]

I'm fine with this... although I noticed that firefox is doing something a little bit more elegant, they make the error message a composed one, e.g.:

// original message:
TypeError: null has no properties

// new message after crossing a boundary: 
Uncaught TypeError: wrapped function threw, error was TypeError: null has no properties

Which is a lot more descriptive. The stack is still needed, but the error is very good, at least during my tests, it was great that they have that level of detail. Now, I'm not sure how to spec that part... because of the nature of the message, which needs to be localized as well. I suspect we can just add a note allowing them to augment the message value. Or just remove the whole copy of the message spec, and just add a note for that allowing them to do this while keeping the spec simple. What do you think?

[caridy]

As for calling Get, this is the conclusion that I have arrived to:

Accessing the name and message of the original object is observable, but considering that the error is crossing (been copied), meaning there is at least another point in the stack frame were some other code is interacting with the wrapped function, and that code can have a try/catch, and access the name and message, there is no way to make a clear distinction if these two properties were accessed because of the callable boundary, or was just the code on the caller controlling the error propagation.

For those reasons, I think the Get is appropriate.

[littledan]

If we allow an augmented message, I guess we'd do that by not specifying the message at all, but rather explaining this possibility in the separate markdown file. I have no strong opinion whether we do this or the Get here, as long as we make a decision and document it.

[caridy]

@littledan @ljharb what about now with the note?

[ljharb]

Suggested change

	For implementations that provide implementation-specific stack for introspection APIs, the TypeError object can be augmented without releaving any information about the stack from within the ShadowRealm, and without providing direct access to any object reference from withtin the ShadowRealm.
	For implementations that provide implementation-specific stack for introspection APIs, the TypeError object can be augmented without revealing any information about the stack from within the ShadowRealm, and without providing direct access to any object reference from withtin the ShadowRealm.

without revealing any information about the stack from within the ShadowRealm

i think this will basically shred debuggability. I understand why it's desirable for membranes, but I think that this information should explicitly be encouraged to be revealed.

[mhofman]

I think there are multiple separate concerns mixed here:

• Is the implementation allowed to expose stack information to the program while respecting ShadowRealm object graph isolation (e.g. through strings or object copies), or should part of the stack information be censored.
• Should the implementation include uncensored information in its debug tooling even when it's not available to the program.
• What is the impact of implementation specific error-related features on object graph isolation.

The latter should IMO simply be a note referring to a normative section on invariants / guarantees that under no circumstances the host can make it possible for the program to break the callable boundary, as discussed in #324.

Regarding debug tooling, I'd encourage hosts to include as much information as possible for the sake of the developer, the same way async stack traces are usually available in debuggers but not in .stack properties.

As for stack information available to the program, I am somewhat worried about revealing information such as filenames and even function name from a different realm.

I would like to point out that these issues are not specific to errors crossing the callable boundary, but apply to any Error created when multiple realms are on the stack. As such I'm not sure such notes belong in this section,

[mhofman]

I gave this more thoughts and posted a longer explanation in the original issue of what in our opinion the approach should be.

[caridy]

cc @mhofman please review.

[mhofman]

I'm still not sure that simply copying the message into a new TypeError is the right approach. We lose the name of the original error, without which the message may make less sense.

I also would find it weird that such an error created at the boundary could ever show stack information from the other side of the boundary.

In #353 I had suggested a new ShadowRealmError which would copy both the message and the name (probably as a new property, like originalName) of the original error, and use that as the cause of a plain TypeError. That ShadowRealmError could have an appropriate toString() to make it obvious it's a copy of an error which had a given name and message.

With that approach there is no question about what stack details are included on the TypeError, and implementations can internally link the ShadowRealmError instance with the original error to display better stack info.

Finally I still find it weird that throwing a primitive (or even a callable) across the boundary does not pass through the same way as return values do, but instead ends up as a TypeError, with no access to the original value.

[mhofman]

I think there are multiple separate concerns mixed here:

• Is the implementation allowed to expose stack information to the program while respecting ShadowRealm object graph isolation (e.g. through strings or object copies), or should part of the stack information be censored.
• Should the implementation include uncensored information in its debug tooling even when it's not available to the program.
• What is the impact of implementation specific error-related features on object graph isolation.

The latter should IMO simply be a note referring to a normative section on invariants / guarantees that under no circumstances the host can make it possible for the program to break the callable boundary, as discussed in #324.

Regarding debug tooling, I'd encourage hosts to include as much information as possible for the sake of the developer, the same way async stack traces are usually available in debuggers but not in .stack properties.

As for stack information available to the program, I am somewhat worried about revealing information such as filenames and even function name from a different realm.

I would like to point out that these issues are not specific to errors crossing the callable boundary, but apply to any Error created when multiple realms are on the stack. As such I'm not sure such notes belong in this section,

[jdalton]

😌

[leobalter]

duplicate of #372 (comment) but yes, not a "relief".

[littledan]

This patch makes a requirement on implementations, that if they expose a stack, it has to be a certain way. That's a normative requirement, so it shouldn't go in a note, and it should have consensus in plenary (since we're already at Stage 3). I take it we're concluding that we do hope the stacks will be visible though introspection APIs across the ShadowRealm boundary, just subject to this censorship?

[littledan]

We need to close on both this question Jordan asks, as well as the suggestion from @mhofman that we avoid a Get and use a GetOwnProperty instead. I'm happy with this current spec text though :)

[caridy]

With the latest changes, this PR now covers to things:

1. makes it simpler to recognize when an error must be copied over the callable boundary into the caller realm, whether that's a result of evaluation, importValue or just calling a wrapped function.

2. defines when to censor the error's stack if the host is providing such information.

The explainer (#381) does describe how to censor, and how to produce a message value for copied errors, to complement this PR.

[mhofman]

Besides a sentence I find a little confusing, LGMT.

[caridy]

Closing in favor of #382