1875639 - Replace loadInDevToolsLoader option with more explicit options to make it less error-prone

Status: RESOLVED FIXED

(Core :: XPConnect, task)

Description

[Tooru Fujisawa [:arai]]

loadInDevToolsLoader option for ChromeUtils.importESModule is optional, and it has implicit "contextual loader" behavior when the option is omitted (load into DevTools global if the current global is DevTools global, otherwise load into shared system global).
This is error-prone that the people can overlook the implicit behavior, and also it's difficult to see which call is actually covered by the behavior.

I'd like to propose an replacement for the option and behavior with the following:

• add global option (will be added by bug 1803810) with the following variants:
  • "shared": load into the shared system global
  • "devtools": load into DevTools' distinct system global
  • "contextual": load into DevTools global if the current global is DevTools global, otherwise load into shared system global
  • "current": load into the current global
• require global option for ChromeUtils.importESModule call inside DevTools global
• drop loadInDevToolsLoader and the implicit "contextual loader" behavior

This enforces checking each ChromeUtils.importESModule call in modules loaded into DevTools, and people should decide where the module should be loaded into, for each case:

• new file is added and loaded into DevTools global
• new code is added to existing module which has been loaded into DevTools global
• existing file is newly added to the set of modules loaded into DevTools global

especially the last case can easily be overlooked unless the option is required.

Also the option can be used as a signal that the file is loaded into DevTools global, when reading the code.

Comment 1

[Tooru Fujisawa [:arai]]

prototype patches in https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=011519c64df32e6218798c67e3a3e02a620182e1
https://hg.mozilla.org/try/rev/937315ad92a755c32d1b1e27bed89cc61ae09a9c and https://hg.mozilla.org/try/rev/d081ac864eb97dbc4b170790248514a33baa0697 are necessary changes for DevTools code.

Comment 2

[Alexandre Poirot [:ochameau]]

It looks great to me:

• we are aligning the devtools-only loadInDevToolsLoader flag into something more generic and used for other usecase
• we will be able to use defineESModuleGetters. (loadInDevToolsLoader was only implemented for importESModule) I see a couple of places which could be simpliefied thanks to this (where we use loader.lazyGetter("foo", ()=> importESModule()))!

Otherwise I imagine that once we start moving from CommonJS module to ESM, we will replace all the importESModule(..., {global: "contextual"}) with import syntax import * from ...;. This will make the code more readable, but this also highlights that once all mozilla-central is using ESM, including devtools, we will still have to be careful to use importESModule, with the global argument if we have to load in the shared global.
In that sense, I think the API change will not necessary drastically reduce the errors related to the devtools loader.
We know some modules ought to be loaded in the shared global. It may be relevant to have a special assertion at the toplevel of these modules. Or have a list of URIs somewhere in the loader which ought to always be loaded in the shared global. I think this is the type of tweak which will finally address the issues around devtools loader.

Having said that, this is a great proposal and this should help moving DevTools to ESM.

Comment 3

[Tooru Fujisawa [:arai]]

Thank you for your feedback!

Good point that this doesn't cover the import declaration.
For standard syntax, we could utilize import attributes proposal's syntax, extending the attribute for this purpose, for example the following:

import * from "..." with { global: "contextual" };

The attribute will work only as an assertion, given import declaration itself doesn't cover a behavior that imports modules into different global.
So, { global: "shared" } inside DevTools global should just throw, and that line needs to be rewritten with ChromeUtils.importESModule(..., { global: "shared" });. Requiring the attributes in modules loaded into DevTools global will keep the less-error-prone behavior even after those modules are migrated to ESM.

But yeah, if there's a known set of modules that needs to be singleton in shared global, having an assertion for the file sounds safer option.
For example, having the following code at the top of the module:

ChromeUtils.assertLoadedIntoSharedGlobal();

Anyway, if the global option looks good, I'll go with it, with adding the option also to defineESModuleGetters.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Bug 1875639 - Part 1: Use global option instead of loadInDevToolsLoader option. r?ochameau!

global option supercedes loadInDevToolsLoader, with omitting the
implicit contextual loader behavior.
In order to make the API less error-prone, use global option, with requireing
global option in DevTools global, to choose from shared global or explicit
contextual loader.

Depends on D199465

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Bug 1875639 - Part 2: Explicitly use contextual loader in files loaded into DevTools global. r?ochameau!

Depends on D199466

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Bug 1875639 - Part 3: Remove loadInDevToolsLoader option and contextual loader behavior. r?ochameau!,jonco!

ChromeUtils.importESModule and ChromeUtils.defineESModuleGetters calls in
DevTools distinct global require the global option.

Depends on D199467

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50520b4ebc8a
Part 1: Use global option instead of loadInDevToolsLoader option. r=ochameau,devtools-reviewers
https://hg.mozilla.org/integration/autoland/rev/628f0c5e7ca6
Part 2: Explicitly use contextual loader in files loaded into DevTools global. r=ochameau,devtools-reviewers
https://hg.mozilla.org/integration/autoland/rev/3df435480a86
Part 3: Remove loadInDevToolsLoader option and contextual loader behavior. r=ochameau,jonco

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/50520b4ebc8a
https://hg.mozilla.org/mozilla-central/rev/628f0c5e7ca6
https://hg.mozilla.org/mozilla-central/rev/3df435480a86