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

Reconsider structured cloning of errors to use .name instead of [[GetPrototypeOf]] #5140

Closed
domenic opened this issue Dec 9, 2019 · 7 comments · Fixed by #5150
Closed

Reconsider structured cloning of errors to use .name instead of [[GetPrototypeOf]] #5140

domenic opened this issue Dec 9, 2019 · 7 comments · Fixed by #5150

Comments

@domenic
Copy link
Member

domenic commented Dec 9, 2019

In #4268, largely guided by tc39/ecma262#1389, we decided to use [[GetPrototypeOf]]() (~ .__proto__) to figure out what type of error object was being cloned. In particular we compared the retrieved prototype with the current Realm's %TypeError%, %EvalError%, etc.

In https://bugs.chromium.org/p/chromium/issues/detail?id=1030086 we discovered that this is kind of fragile. In particular it leads to weird situations like:

  • If you run script in a child frame to postMessage an error to the parent frame, then it comes out with its original type intact.
  • If you run script in a parent frame to postMesssage a (synchronously-accessible) child frame's error to a different child frame, then it comes out as Error.

As such I suggest we move from using .__proto__ to using .name. Both are equally forgeable but using .name will work better in these kind of weird cross-realm cases.

We could also do something like checking .constructor.name or .__proto__.name but IMO we should minimize the number of operations we do, and those would both require two Get()s.

Thoughts? @yhirano @bzbarsky

(Note to self: if we change this then we should fix #4991 at the same time. In particular if we move to Get() then we need to use ? instead of !)

@bzbarsky
Copy link
Contributor

bzbarsky commented Dec 9, 2019

This seems fine to me, I think. @jorendorff, any opinions?

Fwiw, SpiderMonkey has a concept of "error type" that is in fact not forgeable and is attached to every Error object. But that doesn't map to any spec concepts and may not have equivalents in other implenentations, so probably isn't very usable...

@ljharb
Copy link
Contributor

ljharb commented Dec 9, 2019

There is an [[ErrorData]] internal slot on Errors, so it'd be totally reasonable in the spec for that to contain the type. Is there a reason that you can't look up the error type in an unforgeable fashion from internal slots rather than doing observable lookups?

@domenic
Copy link
Member Author

domenic commented Dec 9, 2019

We discussed that previously in tc39/ecma262#1389. In particular there is no observable way to tell the difference between a "real" TypeError and a "forged" TypeError right now, so we shouldn't introduce one via the structured clone algorithm.

@ljharb
Copy link
Contributor

ljharb commented Dec 9, 2019

Ah, right - checking the slot with a fallback to .name might be the best of both worlds, unless it'd be surprising to someone who had Object.assign(new TypeError(), { name: 'RangeError' })

@annevk
Copy link
Member

annevk commented Dec 9, 2019

Though it was discussed previously, it seems tc39/ecma262#1389 (comment) would apply to this solution.

@domenic
Copy link
Member Author

domenic commented Dec 9, 2019

Yeah, it's a tradeoff between how many edge cases we want to prohibit, versus how many we want to allow. I think the edge case of confusing cross-realm issues (which people might realistically run into) outweighs the edge case of people changing .name properties manually (which seems unrealistic outside of tests).

@yutakahirano
Copy link
Member

I'm fine with using .name.

pull bot pushed a commit to p-g-krish/v8 that referenced this issue Jan 10, 2020
Discussed at whatwg/html#5140.

Bug: chromium:1030086
Change-Id: I9decbf300cf817a5cc3396a6cb7f276c8ed8ee25
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1990917
Reviewed-by: Simon Zünd <szuend@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65676}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

5 participants