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

Should we make native error types distinguishable? #1389

Closed
domenic opened this issue Jan 2, 2019 · 18 comments
Closed

Should we make native error types distinguishable? #1389

domenic opened this issue Jan 2, 2019 · 18 comments

Comments

@domenic
Copy link
Member

domenic commented Jan 2, 2019

For background, see whatwg/html#4268. On the web, we'd like to make cloning of errors between realms possible (via structured clone). There's a slight wrinkle though, which is that unlike other cloneable types which have an individual brand, all NativeError types share the [[ErrorData]] brand. I'm opening this issue to get a TC39 discussion started on what to do.

To start, note that if you do

const map = new Map([[1, 2]]);
map.__proto__ = null;
worker.postMessage(map);

the structured serialization algorithm looks at map, finds its [[MapData]] brand, and deserializes inside the worker using the worker's Map.prototype. The question is, what should happen if you do

const t1 = new TypeError();
t1.__proto__ = null;
worker.postMessage(t1);

or if you do

const t2 = new TypeError();
t2.__proto__ = SyntaxError.prototype;
worker.postMessage(t2);

?

I see two paths:

  1. Decide that error types being non-distinguishable is a feature. It's intended behavior that t2 is indistinguishable from new SyntaxError(). Native errors have no private state; everything interesting about them is derived from their public properties.

    • HTML implementation option 1a: deserialize everything that had [[ErrorData]] in the original realm to use Error.prototype in the worker realm. (I.e., clones are always Error, never anything more specific.)
    • HTML implementation option 1b: if the input has [[ErrorData]], get its .name property, and if that is one of the predefined native error types, use the corresponding prototype in the worker realm. Otherwise fall back to Error.prototype.
  2. Decide that it should be possible to distinguish error types.

    • In this world, both t1 and t2 would deserialize using the worker realm's TypeError.prototype.
    • Indeed, in this world, structured cloning would be the only way to discover an error's "true" type.
    • We may want to consider a follow-up non-layering change to expose the "true" type to JS consumers, although I don't have any actual use cases, just a bit of discomfort about giving magic only to other spec hooks.

Thoughts welcome!

@ljharb
Copy link
Member

ljharb commented Jan 2, 2019

Error (and the error types) are the only builtins that are not distinguishable via a brand check. It would be appropriate, imo, to directly add some way to cross-realm distinguish NativeErrors from SyntaxErrors, say - at which point structured cloning could use it.

One simple change might be, make Symbol.toStringTag be a brand-checking getter on each error type (that still returns the same string) - I'm sure that's just one of many alternatives, though.

@bakkot
Copy link
Contributor

bakkot commented Jan 2, 2019

For 1b, why would you use name rather than the current prototype? Offhand that seems to make the most sense to me, but I am not particularly familiar with the conventions of structured clones.

(I.e.: if the input has [[ErrorData]], get its prototype, and if that is one of prototypes of the predefined native error types, use the corresponding prototype in the worker realm. Otherwise fall back to Error.prototype.)

@ljharb
Copy link
Member

ljharb commented Jan 2, 2019

(It's also worth noting that the stacks proposal will add private state to native errors, and a user mechanism of exposing/brand-checking it - however, the stacks proposal does not currently provide any mechanism of distinguishing between error types)

@domenic
Copy link
Member Author

domenic commented Jan 2, 2019

@bakkot fair point; I don't really know what principle would help choose between .__proto__, .name, and .constructor for picking a type.

@domenic
Copy link
Member Author

domenic commented Jan 2, 2019

@ljharb I don't understand what this means:

add some way to cross-realm distinguish NativeErrors from SyntaxErrors, say - at which point structured cloning could use it.

since SyntaxErrors are NativeErrors.

In particular the main question of this thread isn't really about brand checking for errors in general; that's already in the spec via [[ErrorData]], and may eventually be expanded (although I'm not holding my breath) if the stacks proposal ever advances. The question is about whether "being a TypeError" vs. "being a SyntaxError" vs. "being an EvalError" is just a consequence of public properties of error instances, or is a more intrinsic part of their identity.

@ljharb
Copy link
Member

ljharb commented Jan 2, 2019

oh sorry, that's a mistype - RangeErrors from SyntaxErrors, then :-)

The question is about whether "being a TypeError" vs. "being a SyntaxError" vs. "being an EvalError" is just a consequence of public properties of error instances, or is a more intrinsic part of their identity.

Indeed - my personal feeling is that since they're separate constructors, they have thus intrinsically different identities, and it's useful to be able to distinguish them - both in the same realm, and across them via serialization.

@domenic
Copy link
Member Author

domenic commented Jan 2, 2019

RangeErrors are also NativeErrors...

@ljharb
Copy link
Member

ljharb commented Jan 2, 2019

To avoid confusion I'll restate my entire sentence, with a correction:

It would be appropriate, imo, to directly add some way to cross-realm distinguish RangeErrors from SyntaxErrors, say - at which point structured cloning could use it.

@bakkot
Copy link
Contributor

bakkot commented Jan 2, 2019

My preference is to avoid adding brands unnecessarily, and I don't see a good reason for user code to need an unforgeable way to distinguish between different types of errors. So my preference would be for options 1a or 1b (I think 1b or a variant would be more useful and less surprising, but that's a bit outside the scope of this issue).

@domenic, suppose that WHATWG did go with 1b and then later TC39 added some way to allow JS code to distinguish between error types (though like I say I'd prefer that we not do that). Do you think it would be feasible to change the structured clone algorithm to match? Do you think it would be a problem if it didn't match?

@domenic
Copy link
Member Author

domenic commented Jan 2, 2019

Do you think it would be feasible to change the structured clone algorithm to match? Do you think it would be a problem if it didn't match?

Respectively: yes, and no problem. In more detail:

In general we've found on the web very little code cares about error typing, i.e. we are often able to change error types without compat risk. I'd be especially surprised if anyone managed to write code that changes the "type" of an error at runtime by mutating __proto__/constructor/name, and then cloning it, and then depended on that change persisting into the clone. So, I think we could follow any such JS change pretty safely.

And, even if for some bizarre reason it turns out to be web-incompatible to update and match, I'd see that as only a minor edge-case wart, not a big deal.

@devsnek
Copy link
Member

devsnek commented Jan 3, 2019

how about the constructors set [[ErrorData]] to the current constructor? RangeErrors would have [[ErrorData]] === %RangeError% and etc

@ljharb
Copy link
Member

ljharb commented Jan 3, 2019

@devsnek currently nothing is set in that slot; however, the stacks proposal needs a slot to store the stack data. It's fine if [[ErrorData]] is repurposed, but that means the stacks proposal would need to add a second slot (which also might be fine; just pointing it out)

@littledan
Copy link
Member

It feels broken to me to copy the message without the error type, or let the error type be strangely spoofed; I'm not sure what benefit that would have to users. I like option 2.

Note that the WebAssembly JS API also includes NativeErrors; these should probably be treated similarly.

@bakkot
Copy link
Contributor

bakkot commented Jan 5, 2019

@littledan:

or let the error type be strangely spoofed

The way I see it is that, if the spec does not distinguish between new RangeError and Object.setPrototypeOf(new SyntaxError, RangeError.prototype), as I think it does not, then the latter thing has had its type changed, not spoofed.

@bathos-wistia
Copy link

bathos-wistia commented Jan 6, 2019

@bakkot that reasoning seems technically sound for the example given, but I think littledan might be pointing at a different kind of spooky “spoofing”:

err = new Error;
err.name = 'TypeError';
// comes out on the other side as inheriting from TypeError?

@bakkot
Copy link
Contributor

bakkot commented Jan 6, 2019

Sure. That's a good argument for switching on __proto__ rather than .name.

@domenic
Copy link
Member Author

domenic commented Apr 26, 2019

Thanks all for the help on this. Over in whatwg/html#4268 I've recommended that we switch on __proto__. I'll close this out now!

@domenic
Copy link
Member Author

domenic commented Dec 9, 2019

In whatwg/html#5140 I am suggesting we change from .__proto__ to .name because of cross-realm issues. If any folks have thoughts please do chime in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants