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

Allow structured cloning of native error types #4268

Closed
domenic opened this issue Jan 2, 2019 · 19 comments · Fixed by #4665
Closed

Allow structured cloning of native error types #4268

domenic opened this issue Jan 2, 2019 · 19 comments · Fixed by #4665
Labels

Comments

@domenic
Copy link
Member

domenic commented Jan 2, 2019

Over in whatwg/streams, we're working on making streams transferable. One important missing feature right now is the ability to transfer errors across the realm boundary. For example, if you do the following:

// page.js
const ts = new TransformStream();

ts.writable.write(1);
ts.writable.write(2);
ts.writable.abort(new Error("Oh no"));

const worker = new Worker("worker.js");
worker.postMessage(ts.readable, [ts.readable]);
// worker.js

self.onmessage = async ({ data: readable }) => {
  console.log(await readable.read());
  console.log(await readable.read());
  console.log(await readable.read()); // (*)
};

The // (*) line should result in a cloned version of the Error instance being thrown.

Although we could add hacky serialization to the internals of streams for this case, we think it'd be simpler and nicer to allow cloning error instances across the platform. I expect it to be especially beneficial for library developers who want to properly forward errors from a worker to the main page, for example.


Details:

  • We can make DOMException cloneable by adding serialization/deserialization steps to Web IDL. Pretty easy.
  • JavaScript's "native error" types would be done via modifications to StructuredSerializeInternal and StructuredDeserialize, like we do for Date/RegExp/etc.
    • We'd branch on the presence of [[ErrorData]].
    • We have an issue where there is no tamper-proof way to distinguish between error types, i.e. there are no internal slots which distinguish new TypeError() from new SyntaxError(). We could:
      • Just deserialize everything into Error, copying only the message.
      • Grab the tamper-with-able .name property and use that, ensuring we fall back to "Error" if it's not on the safelist of known native error types
      • Grab the tamper-with-able .constructor property and compare that to a safelist
      • Work with the JS spec folks to store the actual error type in a tamper-proof internal slot, perhaps repurposing [[ErrorData]].
        • This is a big weird because we'd be the only consumer of this. In other words, there'd be no way to tell whether something is "really" a TypeError except by structured cloning it and seeing what comes out the other side. Right now there's no real concept of something being "really" a TypeError, and perhaps things should stay that way.

Opinions welcome on what to do with error types. I think I'd probably start with either deserializing everything to Error, or checking .name, and in parallel work with TC39 to see what they think about a stronger approach.

@domenic
Copy link
Member Author

domenic commented Apr 26, 2019

Hey folks, based on tc39/ecma262#1389 I think the best plan is as in the "Details" above, with "how to distinguish" solved by checking object.[GetPrototypeOf] vs. %TypeErrorPrototype%, %SyntaxErrorPrototype%, etc. I don't have time to drive this myself right now, but am happy to review patches toward it.

/cc @ricea @yutakahirano.

@yutakahirano
Copy link
Member

I'll try to make a PR.

@yutakahirano
Copy link
Member

Chrome is interested in this. Do any other vendors have interest?
cc: @annevk
@domenic, do you know a contact person for safari?

@domenic
Copy link
Member Author

domenic commented May 29, 2019

/cc @tschneidereit since this is streams-motivated, for Mozilla interest.

@rniwa and @cdumez are my go-to folks for Safari, although @youennf may be the right contact again since this is streams motivated.

Any interest from other browsers in allowing Errors to be structured cloned, thus allowing streams, and also user libraries, to signal errors better across worker or frame boundaries?

@tschneidereit
Copy link

CC @jorendorff, who's by now much more deeply involved in streams than I am.

That said, I think this makes a lot of sense, and at least something in this direction is probably required to make streams transferable.

@bzbarsky
Copy link
Contributor

There's an interesting problem here, which is that Error and DOMException carry state that is not specified in any specs but is widely depended on in practice. We should probably define whether serialization drops that state or preserves it, but it's a bit hard to do because as far as the specs are concerned the state does not exist.

.stack is the obvious problem here, of course. At least in the case of Firefox, it's represented internally by a fairly complex data structure that may not be trivial to serialize, especially when non-Window globals are involved.

@domenic
Copy link
Member Author

domenic commented May 30, 2019

Thanks for bringing that up, @bzbarsky. I see a few options:

  1. Specify that implementations should serialize any interesting accompanying data, calling out stack non-normatively. This would result in "best effort" implementations still being counted as compliant (e.g., non-Window globals might not work in Firefox, or Chrome's issue with user-created DOMExceptions).

  2. Specify that implementations must serialize the value returned by err.stack, as a string, and on the other side, ensure that newErr.stack returns that string. (Even here I think we wouldn't want to specify exactly how the newErr.stack property is installed, because that varies among browsers.)

  3. (Edited to add) Specify that the stack must get lost when transferring, just to keep it simple. We could probably restore it in the future as the spec infrastructure gets more solid.

Do you (or @yutakahirano) have thoughts on the better approach here?

@bzbarsky
Copy link
Contributor

I think option 1 is fine and slightly more forward-compatible to when .stack is actually standardized.

@bzbarsky
Copy link
Contributor

Oh, just saw option 3. I would probably be OK with it if we need to go there, but it seems suboptimal from a web developer point of view.

@yutakahirano
Copy link
Member

yutakahirano commented May 31, 2019

I like option 3 as a first step. Errors are not at all clonable today, and this can be a good first step. We will be able to contain .stack in the future, when the spec and/or impls get ready. .stack is surely useful information but I think the ability to clone errors has a positive value even without the information.

I cannot imagine someone depends on the assumption that stack is dropped, so choosing option 3 here will not generate a compatibility issue in the future.

cc: @nhiroki

@domenic
Copy link
Member Author

domenic commented Jun 3, 2019

Hmm. Although I can understand if Chrome does not want to also serialize stack in their first implementation, I am hesitant to have the spec prohibit Firefox from serializing stack in their first implementation, if that's their preference. Do you have objections to option 1? I suppose we could also do a variant of option 1, with "may" instead of "should".

@yutakahirano
Copy link
Member

I'm OK with option 1. As there is no way to test the implementation regarding the point, the difference will be a sentence in the spec text.

@domenic
Copy link
Member Author

domenic commented Jun 3, 2019

Well, I think we will probably add a test that .stack is the same, but in a different file since it is only a "should" and relies on shaky spec foundations.

@bzbarsky / @tschneidereit / @jorendorff, is Mozilla "interested" in this feature, i.e. thinks it should land in the spec? I can post to mozilla/standards-positions if that'd be helpful.

@jorendorff
Copy link

@domenic Please go ahead and post to mozilla/standards-positions.

Speaking for myself, I think it's a good idea, and I'm in favor of option 1 and your last comment.

@ajklein
Copy link
Contributor

ajklein commented Jun 25, 2019

Something I don't see mentioned in this issue is any history here. That is, why weren't Errors and DOMExceptions allowed to be structured cloned in the past? Curious of @domenic or @annevk could shed some light on that?

@annevk
Copy link
Member

annevk commented Jun 26, 2019

@jakearchibald shared https://www.w3.org/Bugs/Public/show_bug.cgi?id=28389 with me the other day. It seems there was some concern about accidental sharing of information, but nobody really objected. And there was still the idea that TC39 would take on this algorithm and make it more holistic. And I ended up closing it as WONTFIX since I didn't see that using the prototype could work.

@ljharb
Copy link
Contributor

ljharb commented Jun 26, 2019

stack is unspecified in the language, but will be at some point; it would be helpful to avoid adding additional constraints on doing so.

@tschneidereit
Copy link

stack is unspecified in the language, but will be at some point; it would be helpful to avoid adding additional constraints on doing so.

@ljharb can you say more about what kinds of constraints you're concerned about here?

@ljharb
Copy link
Contributor

ljharb commented Jun 26, 2019

for one, being able to construct an Error with an ErrorData internal slot that contains stack info that might have been doctored manually - ie, as opposed to being only provided by the engine.

domenic pushed a commit that referenced this issue Jul 3, 2019
Define serialization and deserialization steps for all built-in
NativeError types from the JavaScript specification. This change doesn't
cover DOMExceptions, which are covered by
whatwg/webidl#732.

Fixes #4268.

Tests:
* web-platform-tests/wpt#17095
* web-platform-tests/wpt#17159
domenic pushed a commit to whatwg/webidl that referenced this issue Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

8 participants