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

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

Open
mathiasbynens opened this issue Mar 22, 2019 · 20 comments
Open

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

mathiasbynens opened this issue Mar 22, 2019 · 20 comments

Comments

@mathiasbynens
Copy link
Member

mathiasbynens commented Mar 22, 2019

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
Copy link
Collaborator

erights commented Mar 22, 2019

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
Copy link
Member

ljharb commented Mar 22, 2019

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
Copy link
Member Author

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

@ljharb
Copy link
Member

ljharb commented Mar 22, 2019

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 ljharb changed the title V8 implementation differs from what’s described v8 has "stack" as a magic own property Mar 28, 2019
@ljharb
Copy link
Member

ljharb commented Mar 28, 2019

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

@hashseed
Copy link

hashseed commented Jun 11, 2019

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
Copy link
Member

ljharb commented Jun 11, 2019

@hashseed unless the accessor called that function lazily?

@erights
Copy link
Collaborator

erights commented Jun 11, 2019

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
Copy link
Collaborator

erights commented Jun 25, 2019

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
Copy link

bathos commented Aug 11, 2023

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
Copy link
Member

ljharb commented Aug 11, 2023

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
Copy link

bathos commented Aug 11, 2023

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
Copy link
Member

ljharb commented Aug 11, 2023

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
Copy link
Collaborator

erights commented Aug 12, 2023

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
Copy link
Collaborator

erights commented Aug 12, 2023

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
Copy link
Member

ljharb commented Aug 12, 2023

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
Copy link

bathos commented Aug 12, 2023

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
Copy link
Collaborator

erights commented Aug 12, 2023 via email

@mhofman
Copy link
Member

mhofman commented Aug 14, 2023

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

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