2020605 - Prohibit importing single module both with the regular module loader and the sync module loader

Status: ASSIGNED

(Core :: XPConnect, task)

Description

[Tooru Fujisawa [:arai]]

bug 2018794 is caused by importing single module both with the global's regular module loader (<script> or dynamic import()) and sync module loader ChromeUtils.importESModule(..., {global:"current"}).

The sync module loader has some different characteristics and also some limitation, and importing single module with both ways can easily result in inconsistent behavior depending on which one happens first.
And also ChromeUtils.importESModule isn't supposed to fail, but the regular module loader can fail, because the latter happens asynchronously and the global can get discarded in between them (which is why the crash happens in the bug).

We should prohibit importing single module with both ways.
Bug 2018794 comment #4 has an incomplete list of such modules, and apparently this issue happens across the tree, in multiple components.

Comment 1

[Tooru Fujisawa [:arai]]

Here's try run that asserts that modules aren't imported with two different loaders.
https://treeherder.mozilla.org/jobs?repo=try&revision=d3dfd0f8e04798e6e53690429a9700b439c8b86d
The module URI is printed before the assertion failure in the log.

Comment 2

[Tooru Fujisawa [:arai]]

The problem around the reusable components is the following:

• Reusable components are used both in privileged global and content global
  • this means, any code inside reusable components and their dependencies cannot use ChromeUtils
  • the major dependencies are lit.all.mjs and lit-utils.mjs
  • those modules should use the normal import declarations, and let the consumer decide which loader to use
• customElements.js is not the sole consumer of moz-* modules
  • other non-shared (?) custom elements based on them also need to import moz-* modules (e.g. page-assist.mjs)
  • If those other custom elements are also imported both in privileged global and content global, they cannot also use ChromeUtils

So, possible solution would be that, figure out the boundary between the following two types of the modules, and enforce the appropriate module loader when crossing the boundary:

• modules that can be imported both to the privileged global and the content global
• modules that are always imported into either of the global

Comment 3

[Tooru Fujisawa [:arai]]

Possible options for the runtime verification:

• Verify that modules imported by sync module loader doesn't have dependency already imported by regular module loader (the patch above)
• For modules that are known to be imported with two ways, assert the following:
  • If the global has ChromeUtils, ensure that it's imported with ChromeUtils.importESModule (set some flag during the sync module loader import handling, and add an API to verify the state)

Comment 4

[Tooru Fujisawa [:arai]]

The other solution for this problem is to always use the sync module loader in the privileged global, also for <script> and dynamic import().
This can achieve the goal that single module is imported with single module loader,
but this means you cannot use top-level-await.
If there's any consumer of top-level-await in the modules loaded into the privileged global, this isn't applicable.

Comment 5

[Tooru Fujisawa [:arai]]

The following comment looks concerning.
My understanding was that you don't have to import moz-* modules in privileged globals because that's managed by customElements.js,
but the following comment says that's too late.

https://searchfox.org/firefox-main/rev/22d04b52b0eb8d9fa11bf8ede5ccc0243a07c5ba/toolkit/components/satchel/megalist/content/MegalistAlpha.mjs#15-17

// Directly import moz-button here, otherwise, moz-button will be loaded and upgraded on DOMContentLoaded, after MegalistAlpha is first updated.
// eslint-disable-next-line import/no-unassigned-import
import "chrome://global/content/elements/moz-button.mjs";

and indeed customElements.js performs the import after DOMContentLoaded.

https://searchfox.org/firefox-main/rev/22d04b52b0eb8d9fa11bf8ede5ccc0243a07c5ba/toolkit/content/customElements.js#891-893,903

document.addEventListener(
  "DOMContentLoaded",
  () => {
...
              ChromeUtils.importESModule(script, { global: "current" });

Comment 6

[Tooru Fujisawa [:arai]]

:hjones, can I have your input about the comment #5 above?

If the auto-import performed in DOMContentLoaded is not sufficient at least in some case, I wonder if we should instead require
all consumer to manually import reusable component modules with regular import declaration.
In that case, there won't be any ChromeUtils.importESModule consumer for those modules,
and the problem goes away at least for those modules.

Comment 7

[Hanna Jones [:hjones]]

Hey - sorry for the slow response, I was trying to dig into this a bit to see if there were other similar examples where we're importing a reusable component directly out of some concern around timing, but I couldn't find anything else. I also did some manual testing and removing the import statement doesn't seem to have any noticeable effect (the moz-buttons in the UI function as expected, and there's no FOUC or other visual issues that I could see associated with the upgrade happening after DOMContentLoaded).

I'll tentatively say that I think it's ok to disregard this one example and move forward with only allowing auto imports, but I'll take this back to the rest of my team to see if anyone has concerns about that.