1886820 - Consider adding support for Error.captureStackTrace

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Thomas Wisniewski [:twisniewski]]

WebKit quietly decided to implement this pretty recently [1], meaning Firefox is the only browser without support for this API.

We just saw our first webcompat issues [2] because of this.

[1] https://github.com/WebKit/WebKit/commit/997e074bb35ed07b69c9b821141c91dd548e0d02
[2] https://github.com/webcompat/web-bugs/issues/134899

Comment 1

[Jan de Mooij [:jandem]]

Worth noting that the WebKit motivation was mostly performance:

We mostly want it because web tooling benchamrk uses it in the hot path of the chai-wtb test, where having native Error.captureStackTrace is a 5% progression over the polyfill.

According to that commit message, they diverge from V8 by making this a data property instead of an accessor. Bug 1740472 has some background on this. I wish they had followed V8 here instead of adding another potential web compat problem.

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Jan de Mooij [:jandem] from comment #1)

Worth noting that the WebKit motivation was mostly performance:

We mostly want it because web tooling benchamrk uses it in the hot path of the chai-wtb test, where having native Error.captureStackTrace is a 5% progression over the polyfill.

According to that commit message, they diverge from V8 by making this a data property instead of an accessor. Bug 1740472 has some background on this. I wish they had followed V8 here instead of adding another potential web compat problem.

Also worth noting that there's no evidence this is a real-world performance improvement - the JetStream test in question is a test runner typically used in CI / server side environments so we should expect no performance improvement for real world users or content. It would have been much simpler to update the benchmark to drop the usage of the API, or the polyfill, than to ship an unspecified API to the web to improve the subtest score.

Comment 3

[Steven DeTar [:sdetar]]

We discussed this and we're planning to implement this to fix the web compat issues. We'll likely follow V8 and make .stack an accessor property, similar to our .stack property on Error.prototype. We also want to bring this up with TC39 to add this to the spec (maybe as part of Annex B).

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1886820 - Split Stack Limits out of Stack.h r?jandem

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1886820 - Add experimental support for Error.captureStackTrace r?jandem

Comment 6

[Matthew Gaudet (he/him) [:mgaudet]]

So the draft implementation here is... draft.

Missing:

1. Test cases: Particularly edge case test cases (what if the caller filter is a proxy to a callable? What if the callable is exotic?)
2. Early spec text
3. A decision about how to handle the stack property. One option is to install a getter/setter pair that are very similar to Error.prototype.stack, and then store in the object in a private slot the saved frames -- this would avoid the work to build a stack string until it's inspected, and align us with v8
4. Preference control
5. Making a proposal at TC39 to avoid shipping something which has no standards proposal. But also, should we ship this without standards for webcompat?

Comment 7

[Mathieu Hofman]

Could you clarify the intended change in the case where an error instance is used as the object to capture the stack trace onto? Would a new property be added instead of leaving the existing prototype stack accessor? Would this affect error instances when captureStackTrace is not explicitly called?

Adding an accessor own property on all error instances would be a problem. It enables undeniable access to this mutable state on error objects even if those objects are frozen. It would be unacceptable to some TC39 delegates preventing standardization.

We reported these concerns when v8 introduced this behavior and have documented them.

v8 is actually open to using an accessor on the prototype like SpiderMonkey, they just haven't prioritized its implementation.

Given that JavaScriptCore is only installing a data property and to my knowledge no code in the wild relies on there being an own property on error instances, please do not follow the v8 approach.

To be clear, we have fewer concerns with adding an accessor own property to non error objects or through an explicit call to captureStackTrace as that API is deniable. We would still prefer a different shape for that API but understand the web compatibility concerns. The problem is if all errors automatically gain an own accessor property when they come into existence like they currently do in v8.

Comment 8

[Matthew Gaudet (he/him) [:mgaudet]]

So the existing patches use the same semantics as JSC: [[Define]] a new property "stack" on the target object with the value being the stringified stack

One thing I'd consider is to not add the string value, but an accessor; the raw internal stack would then be stored on the object as a private field, and only stringified on access.

I don't really prefer this approach -- fiddly, runs into the issues of what to do with proxies.

The invariant I would maintain if I pursued this however would be that you can't pull that accessor off an access the stack arbitrarily -- instead, the accessor would throw unless the stack had been prepared by Error.captureStackTrace; which might address some of your concern. I'm not sure how V8 is installing this property onto all instances when one gets it... that certainly seems weird.

I did make a typo in my previous comment; when I said "this would avoid the work to build a stack string until it's inspected, and align us with JSC" that should have read V8, as JSC does -not- use the getter approach.

Comment 9

[Brian Grinstead [:bgrins]]

5. Making a proposal at TC39 to avoid shipping something which has no standards proposal. But also, should we ship this without standards for webcompat?

Others here can chime in on timing in terms of the compat priority, but I'd prefer we at least raise the proposal to the group before we ship unless it's especially urgent. How much work would it be / how long would it take to do that? Hopefully others who have shipped this without a standard will be motivated to engage and move it forward.

Comment 10

[Matthew Gaudet (he/him) [:mgaudet]]

I do know that because we're seeing compat issues we'd like to move this; how long to wait I'm less sure about. This should come up a little bit this week at committee.

I'll definitely have a draft proposal before shipping; given that this week is TC39 I have sort of missed the boat for this meeting so earliest next presentation opportunity would be early next year.

Comment 11

[Brian Grinstead [:bgrins]]

A draft proposal & committee discussion seems fine if we're otherwise ready to get it landed to fix compat issues. Am I right that this is basically specifying and implementing V8's behavior, or are there areas we'll diverge?

Comment 12

[Mathieu Hofman]

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #8)

The invariant I would maintain if I pursued this however would be that you can't pull that accessor off an access the stack arbitrarily -- instead, the accessor would throw unless the stack had been prepared by Error.captureStackTrace; which might address some of your concern. I'm not sure how V8 is installing this property onto all instances when one gets it... that certainly seems weird.

I believe that v8 is using the same mechanism for error instances as the one for Error.captureStackTrace.

const e1 = {}; Error.captureStackTrace(e1);
const e2 = (()=>{ try { null.error } catch(e) { return e; } })();
console.log(Object.getOwnPropertyDescriptor(e1, 'stack').get
  === Object.getOwnPropertyDescriptor(e2, 'stack').get); // true

Which means the own accessor is not just installed by an API but also by construction of errors which can be triggered by syntax. Furthermore that accessor also has a setter which modifies the internal slot instead of defining a stack data property on the target.

I think in principle I'm ok for an explicit call to Error.captureStackTrace() to install an accessor property if the target object lacks an [[ErrorData]] slot, and it may be acceptable for that accessor the be the same as the one as Error.prototype.stack (with some fuzziness if that prototype property has been redefined). We also need to consider what installing a private field on the target means: I don't think it should make it appear like a real error object (such that Error.isError would return true), but that doesn't seem to be something v8 is doing today.

Comment 13

[Matthew Gaudet (he/him) [:mgaudet]]

(In reply to Brian Grinstead [:bgrins] from comment #11)

A draft proposal & committee discussion seems fine if we're otherwise ready to get it landed to fix compat issues. Am I right that this is basically specifying and implementing V8's behavior, or are there areas we'll diverge?

Honestly my current inclination is to spec what JSC did which is simpler (c.f. all the discussion around accessor properties here). If that's sufficient and compatible, we're pretty much good to go.

Comment 14

[Matthew Gaudet (he/him) [:mgaudet]]

(In reply to Mathieu Hofman from comment #12)

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #8)

The invariant I would maintain if I pursued this however would be that you can't pull that accessor off an access the stack arbitrarily -- instead, the accessor would throw unless the stack had been prepared by Error.captureStackTrace; which might address some of your concern. I'm not sure how V8 is installing this property onto all instances when one gets it... that certainly seems weird.

I believe that v8 is using the same mechanism for error instances as the one for Error.captureStackTrace.

const e1 = {}; Error.captureStackTrace(e1);
const e2 = (()=>{ try { null.error } catch(e) { return e; } })();
console.log(Object.getOwnPropertyDescriptor(e1, 'stack').get
  === Object.getOwnPropertyDescriptor(e2, 'stack').get); // true

Which means the own accessor is not just installed by an API but also by construction of errors which can be triggered by syntax. Furthermore that accessor also has a setter which modifies the internal slot instead of defining a stack data property on the target.

I think in principle I'm ok for an explicit call to Error.captureStackTrace() to install an accessor property if the target object lacks an [[ErrorData]] slot, and it may be acceptable for that accessor the be the same as the one as Error.prototype.stack (with some fuzziness if that prototype property has been redefined). We also need to consider what installing a private field on the target means: I don't think it should make it appear like a real error object (such that Error.isError would return true), but that doesn't seem to be something v8 is doing today.

Ok, I definitely need to start the proposal repo so we can have this conversation there not here :) I'll start that now.

Briefly however: Were I to do an accessor, no I wouldn't make it so isError returns true. Likely I would use a different accessor than the Error.prototype one too.

Comment 15

[Matthew Gaudet (he/him) [:mgaudet]]

Let's redirect design discussion to https://github.com/mgaudet/proposal-error-capturestacktrace and the issues there

I'll bring this to committee next meeting. Ideally I'd hand this off to another champion, but I can do the initial presentation.

Comment 16

[Steven DeTar [:sdetar]]

Matt, since you are working on this bug and it is priority WebCompat issue I assigned it to you.

Comment 17

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1886820 - Add required header to EncoderConfig.h r?chunmin

It appears that historically Variant has leaked into this file via JS public headers
which was an accident, but breaks if I try to refactor said public headers :)

Comment 18

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1886820 - Forward declare IPC::ParamTraits explicitly r?bryce

Comment 19

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1886820 - Re-indent parse_with_source YAML r?jandem

Comment 20

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1886820 - Add a pref to control Error.captureStackTrace r?jandem

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

The following patch is waiting for review from an inactive reviewer:

ID	Title	Author	Reviewer Status
D232544	Bug 1886820 - Forward declare IPC::ParamTraits explicitly r?bryce	mgaudet	bryce: Inactive

:mgaudet, could you please find another reviewer?

For more information, please visit BugBot documentation.

Comment 22

[Jan de Mooij [:jandem]]

(Actually maybe we should wait until this rides the trains.)

Comment 23

[Sebastian Zartner [:sebo]]

I believe dev-doc-needed is fine, because the prototype should at least be added to https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Experimental_features.

Once there is a bug to let it ride the trains, dev-doc-needed should be added there as well to add the related MDN page and the compat data and remove it again from the experimental features page.

Sebastian

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f31983de17e3
Add required header to EncoderConfig.h r=chunmin
https://hg.mozilla.org/integration/autoland/rev/5ae624507f7f
Forward declare IPC::ParamTraits explicitly r=alwu
https://hg.mozilla.org/integration/autoland/rev/be7acf149be3
Split Stack Limits out of Stack.h r=jandem
https://hg.mozilla.org/integration/autoland/rev/a50998b9515d
Re-indent parse_with_source YAML r=jandem
https://hg.mozilla.org/integration/autoland/rev/32719a5fd93d
Add a pref to control Error.captureStackTrace r=jandem
https://hg.mozilla.org/integration/autoland/rev/c27c123c7051
Add experimental support for Error.captureStackTrace r=jandem

Comment 25

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/f31983de17e3
https://hg.mozilla.org/mozilla-central/rev/5ae624507f7f
https://hg.mozilla.org/mozilla-central/rev/be7acf149be3
https://hg.mozilla.org/mozilla-central/rev/a50998b9515d
https://hg.mozilla.org/mozilla-central/rev/32719a5fd93d
https://hg.mozilla.org/mozilla-central/rev/c27c123c7051

Comment 26

[Hamish Willee]

FF136 MDN docs for this can be tracked in https://github.com/mdn/content/issues/37930. This is basically done, though there will likely be some minor updates if the TC39 proposal is accepted.