tc39 / proposal-error-stacks

ECMAScript Proposal, specs, and reference implementation for Error.prototype.stack / System.getStack
https://tc39.github.io/proposal-error-stacks/
MIT License
170 stars 13 forks source link

v8 has "stack" as a magic own property #26

Open mathiasbynens opened 5 years ago

mathiasbynens commented 5 years ago

V8 now implements error.stack as as “magic” property, similar to Array.prototype.length: https://cs.chromium.org/chromium/src/v8/src/accessors.cc?rcl=472d3c8777d2ff0715cd217d6d3d4dcf27000db3&l=821-881

cc @schuay @hashseed

erights commented 5 years ago

Why not follow the proposed approach of a configurable Error.prototype.stack accessor property, as Mozilla does? As a configurable property on the prototype, it is both deletable and replaceable, enabling the censoring or virtualization of the stack trace into obtainable directly from an Error object. As a magic own property, it has none of this flexibility.

ljharb commented 5 years ago

When was that change made? It would seem unfortunate for v8 to have made a change that conflicts with this proposal, after this proposal was available.

mathiasbynens commented 5 years ago

AFAICT, the change is from July 2016 and thus predates this proposal. https://codereview.chromium.org/2142933003

ljharb commented 5 years ago

Thanks for clarifying.

Since one of the important requirements is that it be possible to make Error objects not have any stack property (ie, by deleting the accessor since it's in Annex B), and to virtualize via getStackString and friends, do you anticipate v8 being unwilling to make that change?

ljharb commented 5 years ago

This may already be described in the readme: https://github.com/tc39/proposal-error-stacks#compatibility

hashseed commented 5 years ago

V8 exposes a Error.captureStackTrace that captures a stack trace and attaches it to any object of the user's choice as .stack property. Migrating to accessors on the prototype seems infeasible for this.

@szuend

ljharb commented 5 years ago

@hashseed unless the accessor called that function lazily?

erights commented 5 years ago

Migrating to accessors on the prototype seems infeasible for this.

See the old code at https://github.com/tvcutsem/es-lab/blob/erroraccessor/src/ses/debug.js which I'd start with to shim the error stacks proposal. I just added the erroraccessor branch whose only change is the definition of the Error.prototype.stack accessor at the end of the file https://github.com/tvcutsem/es-lab/blob/erroraccessor/src/ses/debug.js#L519

You can see the shimmed stack logic in action by visiting https://rawgit.com/tvcutsem/es-lab/erroraccessor/src/ses/contract.html from various browsers, and then clicking on the plus of the [+]Error text. These stacks are being rendered from the JSON representation of the stacks, approximately according to the proposal.

erights commented 5 years ago

https://bugs.chromium.org/p/v8/issues/detail?id=9386 reports a serious bug with the current v8 implementation. Comment 3 at https://bugs.chromium.org/p/v8/issues/detail?id=9386#c3 suggests moving towards this proposal would also help in fixing this bug.

bathos commented 1 year ago

V8 stack update “report”!

V8 switched stack to own accessor properties! The get/set accessor functions are created per-realm, not per stack-decorated “target”. I’m using the word “target” because these can be arbitrary objects due to captureStackTrace (CST from here on) — they need not have [[ErrorData]] or otherwise be “errory”.

(One can demonstrate using structuredClone that the host doesn’t perceive a CST-decorated target as having sprouted an [[ErrorData]] slot, which is good — it skips past that branch.)

The new stack getter behaves as if there’s a cross-realm weak map behind the scenes associating target receivers with backing state. The stackTraceLimit/prepareStackTrace dance only happens once per target receiver per ... individual implicit or explicit captureStackTrace call, actually? Calling CST repeatedly with the same target appears to reset its backing state, so the next getter access after each CST call will be “fresh” and may invoke [UC] again even if it had previously done so for the same receiver object.

(This is a lot of text and not all of it is of equal import! The key “news” that’s directly pertinent to this thread is covered in the above three paragraphs, so you may want to stop reading here. However I figured some of my other “research findings”* related to changes / non-changes within the V8 stack trace API could be of direct interest to some of the folks here.)

Not new, but connects to a fan favorite topic: CST throws immediately if called with a Proxy exotic object. The error message produced is unique to that branch, and (due to particulars of the alternative branch’s logic), CST can be repurposed as a perfect unobservable IsProxyExoticObject predicate. I would obv ... prefer ... for that not to be the case. However I suspect this early bail is just a relic of the old “magic” version of the stack property: keep in mind the old CST was CHANGING any arbitrary ordinary target object into one that was now provably exotic (!), so one can imagine why PEOs had a special branch that was just like “oops bye gotta go!”. Perhaps this can be safely removed now — the new behavior seems like it would be totally compatible with PEOs (afaict).

Mysteriously, CST also now throws an error if target.[[IsExtensible]] returns false. This seems pretty weird to me! There are plenty of cases where the subsequent target.[[DefineOwnProperty]] that this check is “guarding” would succeed regardless, and of course plenty where it fails with extensible objects. One consequence of this is that the updated CST is idempotent (modulo resetting of the host’s private state) for extensible targets, but not for inextensible targets even though OrdinaryDOP (+ most other concrete [[DOP]] methods) are supposed to succeed when given an identical property descriptor. CST throws instead. A clue: there’s a comment in the code that says it’s bailing for frozen objects. While that also wouldn’t make sense, it seems plausible that someone mistook [[IsExtensible]] for an integrity level check? (Seems like this could be fixed, too, unless there’s some specific motivation for this behavior that’s obscure to me...)

I don’t think the Error.stackTraceLimit behavior has changed — it’s still inexplicably magical. It can be shown (but: only indirectly, not through direct observation) that V8’s %Error% behaves as if it had a unique exotic concrete [[Get]] behavior associated with this property name. (Perhaps notable: built-in function objects seem to be forbidden from having non-ordinary OIM behaviors apart from their own [[Call]]/[[Construct]], so this might be non-conformant? not certain tho.) The weird thing it does is to seemingly kick off a regular old prototype chain walk as if by OrdinaryGet, except that instead of actually delegating to its [[Prototype]]’s [[Get]] (or the next one, etc), it bails from the overall walk as soon as either (a) a matching property is found, but it’s an accessor or (b) a [[GetPrototypeOf]] call returns a PEO (again with this ... 😢). The idea seems to be to avoid any possible [UC] invocation, but it does so in a way that only seems explicable in terms of unique exoticism.

Another change I think I noticed — though I’m not 100% certain that my memory’s right about this — is that there seems to have been an adjustment to cross-realm visibility behaviors related to prepareStackTrace/CallSite in HTML Window environments in the last ~year. I’m pretty sure that CallSites from realms whose associated browsing contexts have already been destroyed were previously filtered and not passed to PST callbacks associated with other realms. Now, though, even if a nested BC is disconnected/destroyed, CallSite instances representing frames from the dying realm are included in the CallSite arrays passed to PST callbacks of other (same-origin) realms.

Bonus lil surprise thing:

console screenshot showing that if Realm B code has access to only one Realm A associated object, Realm B’s own CST is sufficient to obtain access to Realm A’s associated %GetErrorStack% function

Anyway the switch to accessors for "stack" was fantastic news IMO. They’re still own properties, but it seems like a major step towards this proposal or at least more unified behavior across agents, and it finally shut down CST’s “make any existing object exotic!” carnival booth, which was way 2spooky for me anyway.

* poking at things with sticks for hours to see if they die in interesting ways (...but taking notes!)

ljharb commented 1 year ago

That's great! So, to clarify, the accessor for stack is not on the prototype, it's an own property, and it's shared across all error instances? (can you mutate it on one error and see the mutation on another? if so, i'm confused why it's not on the prototype)

bathos commented 1 year ago

That's great! So, to clarify, the accessor for stack is not on the prototype, it's an own property, and it's shared across all error instances? (can you mutate it on one error and see the mutation on another? if so, i'm confused why it's not on the prototype)

Yep, all correct! I don’t know why it’s not on the prototype either. Just speculating, but perhaps it’s only due to sharing machinery with captureStackTrace in V8’s codebase? A lot of existing code leverages CST & and it takes (nearly) any old object and defines the stack property right on it (+ associates it with the private backing state).

ljharb commented 1 year ago

What I'd assume is that the accessor should live on the prototype, and it'd be fine if it populated an own property, and CST would just invoke the accessor.

Either way this is an improvement :-)

erights commented 1 year ago

No it is not. If it is on the prototype, it can be replaced before other code starts running. If it is an own property, since anything can provoke an error to be thrown, it cannot be censored or virtualized.

erights commented 1 year ago

FF and XS both have an accessor on the prototype. We propose here an accessor on the prototype. We make use of this on FF and XS, and we propose following their lead because it makes those easy to secure.

ljharb commented 1 year ago

It’s an improvement in that it demonstrates that it’s web compatible for it not to be a data property; i agree that v8 would still need to make changes for the reasons indicated.

bathos commented 1 year ago

No it is not. If it is on the prototype, it can be replaced before other code starts running. If it is an own property, since anything can provoke an error to be thrown, it cannot be censored or virtualized.

As far as I can tell, you’re correct that it can’t be virtualized. However it can be censored, e.g. by capturing and reimplementing %Error% to take control of the static API capabilities while retaining encapsulation or — if all one needs is to suppress stack strings altogether — with a single statement: Object.defineProperty(Error, "stackTraceLimit", { configurable: false, value: undefined, writable: false }) (note: not zero). Doing that will make the stack accessors return undefined unilaterally and will eliminate both of the associated [UC] hooks (i.e. Get(Error, "prepareStackTrace") + the subsequent invocation if found).

erights commented 1 year ago

Good. SES already replaces Error and the Error API. Will try it out and see how safe it seems.

mhofman commented 1 year ago

Here seems to be the v8 CL that introduced this new behavior: https://chromium-review.googlesource.com/c/v8/v8/+/4459251

erights commented 7 months ago

See https://github.com/endojs/endo/blob/master/packages/ses/error-codes/SES_UNEXPECTED_ERROR_OWN_STACK_ACCESSOR.md