Update ESLint for changes to JSM globals

RESOLVED FIXED in Firefox 65

Status

P1
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: Nika, Assigned: standard8)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla65
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 months ago
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.
(Reporter)

Comment 1

5 months ago
ni? as an answer to your question :standard8 https://phabricator.services.mozilla.com/D9736#inline-41588
Flags: needinfo?(standard8)
(Reporter)

Comment 2

5 months ago
(Marking this as blocking bug 1501127, as removing those calls without these changes will probably cause ESLint failures)
Blocks: 1501127
(Assignee)

Comment 3

5 months ago
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.
Flags: needinfo?(standard8) → needinfo?(nika)
(Reporter)

Comment 4

5 months ago
(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 :-)
Flags: needinfo?(nika)
(Assignee)

Comment 5

4 months ago
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.
(Assignee)

Comment 6

4 months ago
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.
Flags: needinfo?(nika)
(Reporter)

Comment 7

4 months ago
(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.
Flags: needinfo?(nika)
Attachment #9026105 - Attachment description: Bug 1502048 - Update the list of ESLint globals for recent JSM scope changes. → Bug 1502048 - Update the list of ESLint globals for recent JSM scope changes. r?nika,r?Mossop
(Assignee)

Updated

4 months ago
Assignee: nobody → standard8
Status: NEW → ASSIGNED
Priority: -- → P1

Comment 8

4 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a48334dcd15
Update the list of ESLint globals for recent JSM scope changes. r=nika,mossop

Comment 9

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5a48334dcd15
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
(Assignee)

Updated

4 months ago
Blocks: 1509490
You need to log in before you can comment on or make changes to this bug.