whatwg / html

HTML Standard
https://html.spec.whatwg.org/multipage/
Other
8.19k stars 2.72k forks source link

Should we delete or otherwise neuter globalThis in worklets? #6059

Open domenic opened 4 years ago

domenic commented 4 years ago

The worklets spec currently claims

The following techniques are used in order to encourage authors to write code in an idempotent way:

  • No reference to the global object, e.g. self on a DedicatedWorkerGlobalScope.

In the process of moving this to HTML in #6056, I noticed that this is no longer a true statement. Since the Worklets spec was written, the JavaScript specification has introduced the globalThis global, which points to the global this value/global object.

I see several options here:

  1. Do nothing, and let globalThis continue to live and point to the WorkletGlobalScope object. The main justification for this is that the purpose of globalThis was to provide cross-environment uniformity, and we shouldn't counteract that. This would involve rephrasing that section of the spec to admit that you can store global state if you want to; the browser won't stop you.

  2. Delete the globalThis property on WorkletGlobalScope creation, like we currently do for SharedArrayBuffer in some cases. This gets us back to the status quo when worklets were originally specced, at the cost of going against the spirit of the JS specification. (But not the letter; hosts are allowed to change global properties, just like JS code can.)

  3. Say that the "global this value" in worklets is undefined, even though the global object is a WorkletGlobalScope. This would have the following more-subtle effects:

    • eval("this"), which evals this in sloppy mode global scope, would return undefined. I.e., this would plug the current way in which code can escape the strict/module-level global scope worklets are usually confined to. That means the global is really super-censored, even more than it ever has been.

    • hasOwnProperty('globalThis') would still return true (unlike with option (2)).

    • This would probably require ES spec updates, as currently the ES spec requires the value to be an Object. At a quick skim, however, it seems like nothing in the ES spec would break if we relaxed this constraint.

    • Probably this would be harder to implement than (2).

/cc @syg for ES side considerations; @nhiroki for Chrome worklet stuff; @bfgeek as worklets editor.

Yay295 commented 4 years ago

Would it be possible to freeze WorkletGlobalScope so that it could be accessible but not modifiable? This would still require a change to the worklets spec, but I think the original intention would remain.

syg commented 4 years ago

I'm not very familiar with Worklets. Do different runs of a worklet get a different WorkletGlobalScope or the same one?

domenic commented 4 years ago

Would it be possible to freeze WorkletGlobalScope so that it could be accessible but not modifiable? This would still require a change to the worklets spec, but I think the original intention would remain.

That's also a very interesting option. I wonder if it's been considered in the past? @bfgeek?

I'm not very familiar with Worklets. Do different runs of a worklet get a different WorkletGlobalScope or the same one?

They generally share the same one. The issue is that the runs will be done in an implementation-defined way. E.g., one browser might spin up 2 WorkletGlobalScopes and run all CSS custom paint code in those two in an alternating, deterministic fashion; another might spin up 5 and choose at random; another, very inefficient, browser might destroy and recreate WorkletGlobalScopes every time it needs to do a custom paint operation.

Because of this by-design interop issue, the spec is trying to nudge web developers to write their code to be more idempotent and less stateful, to avoid in-practice interop issues. See https://drafts.css-houdini.org/worklets/#code-idempotency and https://drafts.css-houdini.org/worklets/#speculative-evaluation for a bit more background on that.

So concretely, it's bad if authors write worklet code that stores state on the global object, because it might stick around in one browser between calls into the worklet code, whereas in another browser it might not stick around.

syg commented 4 years ago

Thanks for the very helpful background @domenic!

For the intention of discouraging easy-to-reach-for language features that'll make code accidentally non-idempotent, I like @Yay295's suggestion to shallow freeze the global object the most.

A note that one may be tempted to take this approach further and shallow freeze built-in prototypes as well, to close off that avenue of non-idempotence. But this is not possible due to the "override mistake", whereby a frozen property on the prototype prevents assignment of a same-named property on instances, freezing built-in prototypes is highly likely not web compatible.

Also I'd probably rule out option 3 in https://github.com/whatwg/html/issues/6059#issue-720528492, because they're too involved for the surface they're trying to plug. The general problem of "make global state impossible to store" is very hard in general and to make future proof, and the extra spec and implementation contortions there seem not worth the effort.

annevk commented 4 years ago

cc @padenot

padenot commented 4 years ago

No, this is required for AudioWorklet, where sharing state on a global is a feature. At the very least, there is a necessity to have a mechanism to have the global accessible (opt-in or opt-out).

domenic commented 4 years ago

@padenot I understand that state is required for audio worklet, but is globalThis in particular required? From what I can see looking at examples, state is stored elsewhere, not on the global object.

padenot commented 4 years ago

It's useful if one wants to share data between different instances of AudioWorkletProcessor. A simple example would be a sample bank, that is a few hundreds of megabytes up to multiple gigabytes, that has to be resident in memory, and obviously only present once.

domenic commented 4 years ago

Alright. I find it kind of strange that AudioWorkletGlobalScope doesn't define self then, if it's explicitly desirable to expose the global, and instead relies on this accident of history that TC39 happened to standardize globalThis some years after AudioWorkletGlobalScope was specced. What were people doing for the intervening years? But it sounds like we're not going to change anything, so I'll close this.

annevk commented 4 years ago

Well, I think it's still worth considering for non-audio worklets. And I think you make a good point that audio worklets should probably expose self for consistency with other globals.

padenot commented 4 years ago

cc @hoch

This sounds like something we want to do yes.

hoch commented 4 years ago

+1 on exposing self on AudioWorkletGlobalScope.

annevk commented 2 years ago

I wonder if @tabatkins has opinions here or knows someone who can give input from the perspective of the CSS-related worklets.

I'm also not sure%0A%3C%2Fscript%3E) how I can get this to not return undefined:

<script type=module>
console.log(eval("this")); // undefined
</script>
bathos commented 2 years ago

@annevk needs some indirection%0A%3C%2Fscript%3E), eval lives in a weird limbo between regular function and syntax (step 6 at that link).

annevk commented 2 years ago

Okay, so I think this primarily hinges on what we want for CSS-related worklets and to what extent they're still malleable due to lack of multiple implementations. If they are malleable, freezing seems interesting given the discussion above.

What's left at that point is whether self is exposed in all worklets or just audio worklets. I'd expose it in all worklets given that the global already leaks in multiple ways and hiding it comprehensively isn't feasible (again, per discussion above). Might as well make all the web platform globals look somewhat similar.