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

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

Open
domenic opened this issue Oct 13, 2020 · 15 comments
Open

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

domenic opened this issue Oct 13, 2020 · 15 comments

Comments

@domenic
Copy link
Member

domenic commented Oct 13, 2020

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

Yay295 commented Oct 13, 2020

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

syg commented Oct 13, 2020

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

@domenic
Copy link
Member Author

domenic commented Oct 13, 2020

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

syg commented Oct 13, 2020

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 #6059 (comment), 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
Copy link
Member

annevk commented Oct 14, 2020

cc @padenot

@padenot
Copy link

padenot commented Oct 14, 2020

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

domenic commented Oct 14, 2020

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

padenot commented Oct 15, 2020

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

domenic commented Oct 15, 2020

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.

@domenic domenic closed this as completed Oct 15, 2020
@annevk
Copy link
Member

annevk commented Oct 15, 2020

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.

@annevk annevk reopened this Oct 15, 2020
@padenot
Copy link

padenot commented Oct 15, 2020

cc @hoch

This sounds like something we want to do yes.

@hoch
Copy link
Contributor

hoch commented Oct 15, 2020

+1 on exposing self on AudioWorkletGlobalScope.

@annevk
Copy link
Member

annevk commented Mar 11, 2022

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 how I can get this to not return undefined:

<script type=module>
console.log(eval("this")); // undefined
</script>

@bathos
Copy link

bathos commented Mar 11, 2022

@annevk needs some indirection, eval lives in a weird limbo between regular function and syntax (step 6 at that link).

@annevk
Copy link
Member

annevk commented Mar 11, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

7 participants