tc39 / proposal-shadowrealm

ECMAScript Proposal, specs, and reference implementation for Realms
https://tc39.es/proposal-shadowrealm/
1.44k stars 67 forks source link

Exposed=* vs Exposed=Shadow #398

Closed annevk closed 2 months ago

annevk commented 9 months ago

https://github.com/whatwg/dom/pull/1253 suggests that in order to be a global you now need to support an event loop. When we started out with this that's not what ShadowRealms would imply. And in fact it would be the APIs we could imagine each global to support.

I wonder if this means we should simply give them their own Exposed value and add that in various places.

Correct me if I'm wrong, but I don't think all worklets have an event loop for instance.

cc @domenic @littledan @Ms2ger @ptomato

domenic commented 9 months ago

Right, Exposed=* doesn't work for anything event loop related, because of worklets. I wonder what happens when you construct a ShadowRealm inside such a worklet? I think that just argues against exposing such things to ShadowRealms.

Ms2ger commented 9 months ago

The main reason for Exposed=* is that it would be surprising to expose things in a shadow realm that are not exposed in its principal realm. I suppose the alternative is that exposure inside a shadow realm is conditional on them being exposed in the principal realm as well.

annevk commented 9 months ago

Or we don't expose shadow realms in certain globals. There's a lot of options, but you can't impose event loops on all (potential) globals.

caridy commented 9 months ago

Oh, I thought we already made that decision a while ago when we decided to make the global object of a shadow realm an event target.

domenic commented 9 months ago

? EventTarget is unrelated to the event loop.

caridy commented 9 months ago

Ok, I think I understand it now. It seems that we have multiple options:

A. Exposed=Shadow regardless of what the principal realm exposes. (I don't think this is really an option). B. Exposed=Shadow conditional on them being exposed in the principal realm. (Would this open the door for ShadowRealms inside worklets if use-cases surface in the future?) C. Exposed=* expose shadow realms in certain globals, which means it must be excluded from worklets.

Is there any other option? It sounds to me (with my very limited knowledge) that option B is superior, but I have no idea how that could be spec'd.

domenic commented 9 months ago

Another option is to not expose anything in shadow realms which is not exposable to worklets, i.e. not expose anything which has an event loop dependency. I believe that was the original consensus.

annevk commented 9 months ago

Yeah, the original idea was that shadow realms would only have features we could imagine being in all possible (including future) global environments. That's why * felt justified. If you stray from that you will need to exclude either shadow realms from certain globals or exclude features from shadow realms when used in certain globals.

What syntax to use if you decide to stray from the original idea would need some consideration.

caridy commented 9 months ago

@annevk @domenic here is where we need your guidance and knowledge. The only recent change was about timers, specifically setTimeout & co. Since we are not in the business of protecting against side-channel attacks (spectre/meltdown), we have no reasons to prevent or sensor timers, for those reasons, we thought it was OK to include setTimeout & co in the initial list. It seems that this has bigger implications (which we didn't know). In other words, we have no opinions on whether or not this can be/should be included or not.

Additionally, I will like to know what other APIs depend on the event loop? we can double check our use-cases against those!

littledan commented 9 months ago

While timers would be convenient, they aren't essential for any ShadowRealm use case I've heard of. I like following the philosophy of exposing to ShadowRealms the intersection of what will be available in any conceivable environment. This sounds like a convenient rule of thumb when deciding what to put in ShadowRealms, avoiding the need for web spec developers to think through "confidentiality" and "integrity" with respect to a certain programming model that they might not be familiar with.

ptomato commented 9 months ago

Specifically, not exposing any APIs that depend on an event loop seems like a good criterion, and the rationale outweighs the requests in #393 for the convenience provided by setTimeout.

Concretely, this would mean reverting the recent changes to whatwg/html#9893 re. {set,clear}{Timeout,Interval}, and closing whatwg/dom#1253. Agreed?

Additionally, I will like to know what other APIs depend on the event loop

@Ms2ger is double-checking that none of the other, non-timer, APIs that we proposed to have in ShadowRealm have this dependency. I'm not sure about queueMicrotask; at first glance it seems that it only depends on having a current execution stack, not an event loop, but we'd need to verify that.

ljharb commented 9 months ago

Given that Atomics.wait exists, isn't there already a timer in the language, that would definitely exist in ShadowRealms? Assuming that's accurate, what would be the harm of including other ones like setTimeout?

ptomato commented 9 months ago

The point raised by Anne and Domenic in this thread isn't objecting to timing facilities being accessible inside ShadowRealm. There is Atomics.wait as you point out, and you can even build a shim using Date.now() as Rick Waldron pointed out to me: https://github.com/web-platform-tests/wpt/blob/1ae054a00ce5f883d790569972ed417a80291d32/resources/testharness.js#L1204-L1219

The problem with setTimeout isn't that it's a timer, it's that it does its job in terms of the event loop. Atomics.wait does not (it says to suspend the surrounding agent.) ECMA-262 has no notion of an event loop, so it's something added by the host.

Anne and Domenic point out that worklets don't have an event loop, so if you create a ShadowRealm inside a worklet, it cannot have setTimeout. I saw 4 options mentioned earlier in the thread for how to deal with this, they are:

  1. Expose setTimeout in ShadowRealm anyway and specify it to use some other mechanism than the event loop
  2. Expose setTimeout in ShadowRealm only if the ShadowRealm was created from a realm that has it
  3. Don't have ShadowRealm in worklets
  4. Don't have setTimeout in ShadowRealm

(1) seems not feasible at this time, like it would require making the dependency on the event loop optional in setTimeout and friends. (3) seems undesirable, since ShadowRealm is intended to be part of the language, not a host-provided facility. Of (2) and (4), (2) seems like the option that is more likely to have hidden caveats.

ljharb commented 9 months ago

While that's true and I agree with your analysis of 1 and 3, I think that 2 will be far more ergonomic and intuitive to developers than 4, since in 4 they'll have to pass it in manually in most cases anyways.

mhofman commented 9 months ago

There is Atomics.wait as you point out

The one I'm curious about in worklets is Atomics.waitAsync. Both it and FinalizationRegistry use a host queue. Are they available in worklets?

ptomato commented 9 months ago

While that's true and I agree with your analysis of 1 and 3, I think that 2 will be far more ergonomic and intuitive to developers than 4, since in 4 they'll have to pass it in manually in most cases anyways.

We don't have to agree on this, but I have different priorities there — I think doing 1 some time in the future is the only really ergonomic solution.

None of the others are really my first choice, to be honest. I find 2 more of a short-term win, but has the disadvantage that when you import some code that uses ShadowRealm and setTimeout, it might not work depending on which realm you import it into. So library authors will have to code defensively and not use setTimeout anyway, or ask consumers to pass it in like in 4.

I agree with you that 4 is probably going to go against developers' intuition in the short-term, but at least if you have to pass in setTimeout yourself, you'll easily identify the problem if the principal realm you're working in has no setTimeout. So I think it has better debuggability, which I tend to value highly.

caridy commented 8 months ago

Update: igalia has reverted the changes for timers (https://github.com/whatwg/html/pull/9893/commits/1edd30b1055db71a03e2305eeb4b8959c9704ef2) and closed the PR for AbortSignal.timeout (https://github.com/whatwg/dom/pull/1253). That put us back to square one with respect to this issue.

It looks like we are in agreement to continue pursuing Exposed=*, which means there will be no setTimeout & co in SR for now. Once https://github.com/whatwg/html/issues/10136 is resolved, we can reassess this in the future to try to have a version of the option 1 described above to make it more ergonomic for developers.

@annevk @domenic how do you feel about this outcome? can we close this and move on?

annevk commented 8 months ago

Yeah, if no other APIs were added with similar dependencies we should be okay.

ptomato commented 2 months ago

Trying to solve #401, I've made a proposal for this in https://github.com/w3ctag/design-principles/issues/509. It explicitly says that APIs depending on an event loop should not be annotated with [Exposed=*]. (That doesn't preclude dropping that particular sentence if sometime in the future it's decided that all realms need to have an event loop.) If folks are OK with that, I think we can close this issue.

ptomato commented 2 months ago

Given the positive comments on w3ctag/design-principles#509, I think the event loop question is clear. We'll keep [Exposed=*] and apply it following the design principle described there.