1502048 - Update ESLint for changes to JSM globals

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement, P1)

Description

[Nika Layzell [:nika] (ni? for response)]

In bug 1489301 the way that global interfaces are exposed on system globals was changed. 

It used to be that on the main thread we would have 2 major types of globals: Window, and BackstagePass. BackstagePass is the global used by JSMs. Until recently, each JSM module would get its own BackstagePass global, but that was a memory usage and performance problem. JSMs have since been changed to share a global, but some other mitigations we held in place were changed.

Specifically, it used to be that WebIDL interfaces would have to explicitly expose themselves on `System` to be exposed to BackstagePass globals. This led to the need for workarounds such as https://searchfox.org/mozilla-central/rev/72b1e834f384a2ffec6eb4ce405fbd4b5e881109/js/xpconnect/idl/xpccomponents.idl#270-279 to import these constructors into System globals.

After bug 1489301, the `System` global designation is now gone, and the same set of interfaces are exposedon System and Window globals. However, the JSM global ESLint files have not been updated to include the full set of window-exposed interfaces. It is likely that the Window and JSM ESLint specifications should be merged as well.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

ni? as an answer to your question :standard8 https://phabricator.services.mozilla.com/D9736#inline-41588

Comment 2

[Nika Layzell [:nika] (ni? for response)]

(Marking this as blocking bug 1501127, as removing those calls without these changes will probably cause ESLint failures)

Comment 3

[Mark Banner (:standard8)]

Thank you Nika.

I'm aware that our Window globals are likely to be quite out of date.

Do you know if using https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/ResolveSystemBinding.cpp be a good way of working out the currently exposed Window globals? (this was suggested in bug 1415483 comment 1).

Alternately, is there a different way we could work this out either programmatically or manually?


From a lint perspective, we probably want to change our current lib/environments/jsm.js to be a window.js, move some of the global definitions from recommended.js into it, supplement with an updated WebIDL list, and then likely enable it everywhere. We probably also want to search for duplicate definitions in case people have added `global` statements in the code.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

(In reply to Mark Banner (:standard8) from comment #3)
> Thank you Nika.
> 
> I'm aware that our Window globals are likely to be quite out of date.
> 
> Do you know if using
> https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/
> ResolveSystemBinding.cpp be a good way of working out the currently exposed
> Window globals? (this was suggested in bug 1415483 comment 1).
> 
> Alternately, is there a different way we could work this out either
> programmatically or manually?

It wouldn't be too hard to generate this file automatically from webidl's Codegen.py, but I don't know if WebIDL is run during an artifact build, or if this file would be required in artifact builds.

> From a lint perspective, we probably want to change our current
> lib/environments/jsm.js to be a window.js, move some of the global
> definitions from recommended.js into it, supplement with an updated WebIDL
> list, and then likely enable it everywhere. We probably also want to search
> for duplicate definitions in case people have added `global` statements in
> the code.

Yup, that sounds like a good plan. It'd be good to also get rid of the calls to importGlobalProperties :-)

Comment 5

[Mark Banner (:standard8)]

Attachment: Bug 1502048 - Update the list of ESLint globals for recent JSM scope changes. r?nika,r?Mossop

This patch creates a new list of privileged globals, based on the webidl bindings. The list of jsm globals is reduced to only the ones specially extended to that scope.

The privileged globals are enabled everywhere for the time being - although we likely could limit scopes, doing so is difficult with our current mozilla-central layout and configuration settings.

Comment 6

[Mark Banner (:standard8)]

Nika, please could I get your feedback on how this looks?

The basic idea is that I'm using the list of mozilla::dom::constructor::id::ID from dist/include/mozilla/dom/PrototypeList.h to generate the "privileged" set and apply that everywhere.

I suspect we should really have what's available in content for the unprivileged scope, and then our privileged scopes separate. However at the moment I'm not sure where to get the unprivileged set, and we'd likely have difficulty applying it correctly (due to the layout of m-c/current config limitations of eslint). So I think this is a longer term follow-up.

I'd also like to find a way to either generate the list dynamically or update it automatically (e.g. on building with a new binding), I suspect that's a harder task and so will separate that out to a separate bug - and just gain the updated globals list here.

Regarding importGlobalProperties removal, I'm not sure we'll be able to do that automatically, we might still need to do it manually, but we should have at least the correct environment information to know if we've removed too much.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

(In reply to Mark Banner (:standard8) from comment #6)
> Nika, please could I get your feedback on how this looks?
> 
> The basic idea is that I'm using the list of
> mozilla::dom::constructor::id::ID from
> dist/include/mozilla/dom/PrototypeList.h to generate the "privileged" set
> and apply that everywhere.

That list unfortunately includes global values which aren't exposed on main-thread globals, such as worker or worklet-only constructors. If you want to pull the names from somewhere, the best place is probably `RegisterBindings.cpp` (https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/RegisterBindings.cpp#626), as that's the actual string table which is used to generate the list of bindings on Window.

There are also dynamic checks for some globals which determine if they should be visible, but that may not be worth checking with eslint right now :-).

> Regarding importGlobalProperties removal, I'm not sure we'll be able to do
> that automatically, we might still need to do it manually, but we should
> have at least the correct environment information to know if we've removed
> too much.

I don't think that there is any 'importGlobalProperties' use which should still be needed after these changes, so I'm inclined to think that knowing we've removed "too much" is unnecessary.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a48334dcd15
Update the list of ESLint globals for recent JSM scope changes. r=nika,mossop

Comment 9

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/5a48334dcd15