Consider not exposing ErrorEvent/PromiseRejectionEvent in ShadowRealms

[smaug----]

Exposing PromiseRejectionEvent in Shadow Realms doesn't make much sense, since the global isn't an EventTarget.
https://html.spec.whatwg.org/#unhandled-promise-rejections just doesn't work with that kind of globals.
And https://html.spec.whatwg.org/#report-the-exception doesn't really work either, again because the global isn't an EventTarget.

[annevk]

@domenic @Ms2ger @leobalter do you agree with @smaug---- about #7244?

[domenic]

I'm not sure.

I think it would generally be useful to expose ErrorEvent, so that people could fire ErrorEvents at their own custom EventTarget instances, in case application-specific errors occur. That seems nicer for developers than forcing them to use CustomEvent or Event-with-expandos for such cases.

Directing promise rejections to custom EventTarget instances seems less likely to be useful, but not inconceivable. And I kind of feel like we should treat sync errors and promise rejections the same.

So I lean toward including it, even though the browser will never fire it.

But another consistent perspective would be to keep the number of Event subclasses exposed minimal, e.g. only Event and CustomEvent.

[leobalter]

I was able to review this internally with my team and we all agreed @domenic's feedback feels pretty solid. Please let me know if there is anything else I can provide from my end.

[smaug----]

I could vaguely see someone using ErrorEvent for something, but PromiseRejectionEvent rather unlikely.
In general the logic seems to be that one might use something for something, so expose. And in that case exposing for example MouseEvent sounds reasonable. One could use MouseEvent to emulate some UI actions.
But events aren't exposed that way in workers either.

[caridy]

We discussed this topic today during the SES meeting, and we are in agreement with @domenic's position as well.

@smaug---- with respect to PromiseRejectionEvent, there are other things that the host can do in the future to provide similar mechanism to what you get via window today, it might be placed into another value, not the global object. @mhofman suggested that maybe something like globalThis.errors, which could be an event target just for that purpose. Exposing the PromiseRejectionEvent will be a step closer to eventually provide such mechanism.

[mhofman]

I also want to re-iterate that no matter what, the incubator realm (or whatever the name is for the realm which holds the window as global), is not allowed to expose the error or promise objects from shadow realms, as that would violate the callable boundary.

[smaug----]

@caridy, I can totally understand exposing PromiseRejectionEvent once there is some API using it. But before that? We don't normally expose interfaces beforehand just in case some API later might use them.

[caitp]

Has there been agreement about continuing to expose PromiseRejectionEvent and ErrorEvent?

[caridy]

Ok, I dropped the ball on this one. Let me clarify my position. But first, keep in mind that now we have the semantics of error copying across realms well defined here: https://github.com/tc39/proposal-shadowrealm/blob/main/errors.md, this is important because it allows to think about errors as objects that can be copied from one realm to another at will. With that context, here is what I think:

• ErrorEvent: I agreed with @domenic, it should be there, it doesn't give you any extra power and works nicely with EventTargets, that's useful for programs running inside a shadow realm.
• reportError: the program there might choose to report an error, and such error might be observable via window.onerror on the root realm.
  * PromiseRejectionEvent: this, IMO, is similar to reportError. I think it should be there even if we don't have any API at the ShadowRealm level to deal with it. The reason for that is that nothing in the spec prevents us from surfacing a copy of that error via window.onunhandledrejection on the root realm, the semantics of that are not different from a try/catch IMO. This is going to be important for developers to not only log (or debug) unhandled rejections from the main program, but also for programs running on a child shadowRoot. Eventually, if we surface a mechanism to observe that from within the ShadowRealm itself, we can talk about the new semantics of the propagation if needed. I think I got confused between realms... the fact that a promise is not handled correctly inside a child shadowRealm can still be surfaced in the root realm as a promise rejection with a copy of the error message, and such, but the event itself does not need to be exposed into a shadowRealm, as @smaug---- pointed out.

Note: when an error is copied from a shadowRealm to the root realm, the type changes to TypeError, and the message is stitched together by the Host, but the stack reflects the whole stack, so logging and instrumentation of the errors should remain useful for developers out there.

[caridy]

To be more specific:

const s = new ShadowRealm()
const letItThrow = await s.importValue('./x.js', 'letItThrow');
try {
   letItThrow();
} catch (e) {
   // this is useful
}

That should be equivalent to:

const s = new ShadowRealm()
const letItReport = await s.importValue('./x.js', 'letItReport');
window.onerror = (message, source, lineno, colno, error) => {
  // this is useful
  console.error(`message: ${error.message}, lineno: ${lineno}` );
  return true;
};
letItReport();

That does not violate any of the semantics described in the ShadowRealm proposal.

[mhofman]

The problem of promise rejections and the unhandledrejection event is that it currently provides a direct reference to the rejecting promise, which of course in the root realm cannot be the original promise from the shadow realm, or that would break the callable boundary invariants.

Unless you replaced the promise event property with a representative of the original promise, you wouldn't be able to implement this functionality at all. That representative could be a new Promise of the root realm rejected with the copied error. Its identity would have to be stable with a later rejectionhandled event, since a common approach is to hold these in a WeakMap. This representative may need to be a then-able instead of a platform promise so that "handling" it would also handle the original unhandled rejected promise from the ShadowRealm. Or at least specify a way to filter a separate handling of the original promise in the shadow realm and of its representative in the root realm to not trigger the rejectionhandled event twice.

To avoid this complexity, I would recommend that both uncaught errors and unhandled rejections have a mechanism to be handled directly inside the ShadowRealm. That's why I suggested above a new globalThis.errors which would be an event target for these purposes.

[andreubotella]

To avoid this complexity, I would recommend that both uncaught errors and unhandled rejections have a mechanism to be handled directly inside the ShadowRealm. That's why I suggested above a new globalThis.errors which would be an event target for these purposes.

HTML has a mechanism where if an error is uncaught inside a dedicated worker, it will be reported as an error in the worker's "parent", possibly propagating up the chain until you reach a non-dedicated worker agent (which usually is the window's main agent). The thrown exception is obviously not included in such error events (the error attribute of ErrorEvent is set to null), but if something similar is adopted for ShadowRealms, it could include a copied error with the semantics in https://github.com/tc39/proposal-shadowrealm/blob/main/errors.md.

There doesn't seem to be anything equivalent for unhandled promise rejections, but I wonder if it could be added as well.

[mhofman]

The main problem with unhandled rejections is the fact that the promise instance is part of the event, and that it's not a simple fire once event, but is potentially a pair of unhandled/handled events which need to be consistent.

Another argument in favor of exposing the unhandled rejection events directly in the ShadowRealm is the likely impossibility for these events to be recreated inside the shadow realm by the code running in the root realm. Even if the program could identify which event is tied to a particular ShadowRealm, it is impossible for the program to derive the identity of the original promise inside the shadowrealm if the event is only triggered in the root realm.

The only thing I can imagine is a unique symbol included on the event in addition to (or maybe in place of) the promise property, which could be used inside the ShadowRealm through a dedicated API to retrieve the original promise from that Realm. (I really wish we had standardized Box / ObjectPlaceholder as this was exactly the kind of pattern where this would be useful).

Of course as @andreubotella suggest, triggering the event in the shadow realm, and propagating to the root realm if unhandled may help. However the question arises then to what should happen inside the shadow realm to the original promise if the root realm adds a reaction to the representative promise. (aka would the rejection be become handled in the shadow realm, or not?)

[caridy]

Few notes from today's discussion at SES:

Errors Observed from within a ShadowRealm:

1. It seems important for code running inside the ShadowRealm to be able observe uncaught errors and unhandled rejections. Meaning ErrorEvent and PromiseRejectionEvent should be exposed inside a ShadowRealm.

2. 262 spec for ShadowRealm does not preclude the ShadowRealm's Global Object to be an EventTarget, the only thing that we prescribe there is to be forgeable (which is NOT the case of WindowProxy, and globalThis in Workers). You might have two options here:
   a) make the default ordinary global object a qualifying EventTarget and make its __proto__ to be EventTarget.prototype.
   b) add a new global interface (e.g.: globalThis.errors) that can be an EventTarget.

Errors observed from the incubator realm or root realm:

3. It seems important for code running on the root realm to also observe uncaught errors from a ShadowRealm. This is analogous to what happens with workers.

4. At the moment, we don't see a coherent story to observe unhandled rejections from a ShadowRealm in the root realm or incubator realm. Similar to what happens with workers, in part because of the callable boundary invariants.

5. We don't have an opinion on whether or not the uncaught errors should be propagated through incubator realms, or just directly to the root realm.

[caridy]

@smaug---- @annevk what do you guys think about the notes from last month? #7591 (comment)

I'm leaning toward 2.a, I will like to know if that's possible with an ordinary global object.

[domenic]

I like option 2.a personally. I don't like option 2.b at all, because it causes weird bifurcations in the platform.

It does indeed open up a can of worms, regarding ordinary vs. not. We were already reaching a tricky state with the desire to add certain global methods and properties (e.g. structuredClone()). Let me explain.

Simple version, which was our plan until now IIUC:

shadow realm global object:
  own properties: self, structuredClone, etc.
↓
Object.prototype

Inherit directly from EventTarget:

actual global object:
  own properties: self, structuredClone, etc.
  internal slots: from EventTarget
↓
EventTarget.prototype:
  own properties: "addEventListener", "constructor", "dispatchEvent", "removeEventListener", @@toStringTag
↓
Object.prototype

Web platform usual pattern:

actual global object:
  own properties: self, structuredClone, etc.
  internal slots: from EventTarget, maybe from ShadowRealmGlobalScope if we define any
↓
ShadowRealmGlobalScope.prototype:
  own properties: "constructor", @@toStringTag
↓
EventTarget.prototype:
  own properties: "addEventListener", "constructor", "dispatchEvent", "removeEventListener", @@toStringTag
↓
Object.prototype

I worry that "Inherit directly from EventTarget" is a bad intermediate state. For example, you'd end up with globalThis.constructor === EventTarget, and globalThis.toString() === "[object EventTarget]".

So if we go down the 2.a route, I think "web platform usual pattern" is probably the way to go. /cc @yuki3 @syg as I know they were recently looking at some related parts of Blink/V8.

[smaug----]

yeah, I think it should be "web platform usual pattern".
In general the global inheriting from EventTarget makes sense.
That starts to feel like moving the definition of the global to HTML/DOM land from pure Ecma.
I assume we'd have webidl for ShadowRealmGlobalScope and defines methods and attributes there - that would at least make the implementation less unusual.
I'm sure some people outside from the browser implementations would object that though.

(And yes, 2b looks awkward.)

[mhofman]

I believe the only requirement 262 has on the ShadowRealm global is that it behaves like an ordinary unsealed object, aka that all properties be configurable, that the prototype can be set, and that changing any of those succeeds like for ordinary objects (including explicitly freezing the global object).

[caridy]

I'm fine with "web platform usual pattern", and I think the rest of the champions will also agree with that approach. Again, the main goal was to get off the business of WindowProxy and unforgeable properties, and that proposal from @domenic does that.

[annevk]

Only quibble I have is that both "realm" and "global" in the name seems odd, but overall it having a more solid Web IDL footing seems good.