1803810 - Support synchronously loading ESMs into given global, in the same way as Services.scriptloader.loadSubScript

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[Tooru Fujisawa [:arai]]

Services.scriptloader.loadSubScript has been used for loading regular script (non ES module) into given global.
We'll need the equivalent for ES modules, for example in custom elements, to support ES modules in more cases.

https://searchfox.org/mozilla-central/rev/ce78234f5e653a5d3916813ff990f053510227bc/toolkit/content/customElements.js#846

Services.scriptloader.loadSubScript(script, window);

ChromeUtils.importESModule supports synchronous loading, but it uses per-process shared global, and it doesn't accept random global as a target.

Comment 1

[Tooru Fujisawa [:arai]]

What the expectation for the loaded module would be?

case A:

• module M1 is loaded with the new sync module loader API into global G1
• module M2 is loaded with dynamic import() inside global G1
• both module M1 and module M2 imports module M3

should module M3 be a singleton in G1 and shared between them?
or should module M3 be separate instances for sync API and others?

case B:

• module M4 is loaded into the shared system global with ChromeUtils.importESModule
• module M4 loads module M5 with the new sync module loader API into the shared system global
• both module M4 and module M5 imports module M6

should module M6 be shared between them?

Comment 2

[Tooru Fujisawa [:arai]]

:hjones, what's the requirement in the custom elements case?
I think the case A above can be related.
does the custom element's sub script require their own scope for sharing/reusing modules and they need to be hidden from outside?

Comment 3

[Jon Coppeard (:jonco)]

I think 'shared' in both cases is the only answer that makes sense for the module system. Since loadSubScript is not yet used for modules, I'm hoping that is an acceptable requirement.

Comment 4

[Hanna Jones [:hjones]]

(In reply to Tooru Fujisawa [:arai] from comment #2)

:hjones, what's the requirement in the custom elements case?
I think the case A above can be related.
does the custom element's sub script require their own scope for sharing/reusing modules and they need to be hidden from outside?

Hey :arai I don't know that I have a deep enough understanding of how the current subscript loader works to be able to answer this definitively. What I can say is that for reusable components purposes we would want whatever allows us to replicate this behavior with our module based custom elements: https://searchfox.org/mozilla-central/source/toolkit/content/customElements.js#811-813

Basically we're just looking for a way to defer loading the module until the element is getting created.

Possibly also relevant for getting a sense of our needs - we've implemented this window.ensureCustomElements function and the related importCustomElementFromESModule function as temporary workaround. It's less than ideal as we end up needing to call window.ensureCustomElements(<element-name>) in lots of arbitrary JavaScript files.

Comment 5

[Tooru Fujisawa [:arai]]

thanks!
okay, so it's already using dynamic import(), and those modules import another modules.

https://searchfox.org/mozilla-central/rev/7907fd85f6fd1cee3acaddbaf27a76b23d6d9993/toolkit/content/customElements.js#821-823

return import(
  "chrome://global/content/elements/moz-button-group.mjs"
);

https://searchfox.org/mozilla-central/rev/7907fd85f6fd1cee3acaddbaf27a76b23d6d9993/toolkit/content/widgets/moz-button-group/moz-button-group.mjs#5-6

import { html } from "chrome://global/content/vendor/lit.all.mjs";
import { MozLitElement } from "chrome://global/content/lit-utils.mjs";

in that case, it sounds like there's no need to have separate scope, and the 'shared' option is possible.

Then, if sync and async module loading should have the same scope, the next question is what should happen if single module is imported with both ways.

A minimal testcase would be something like this:

// dynamic import
import("a.mjs");
// sync import.  (pseudo API)
Services.scriptloader.loadSubScript("a.mjs", this, { module: true });

in regular import (dynamic import) case, a module request can have Fetching state (or possibly Compiling or LoadingImports state) while JS is executing.

https://searchfox.org/mozilla-central/rev/7907fd85f6fd1cee3acaddbaf27a76b23d6d9993/js/loader/ScriptLoadRequest.h#204-207

enum class State : uint8_t {
  Fetching,
  Compiling,
  LoadingImports,

When the sync import starts, the target module or dependencies can be in above states,
but the sync module loader cannot asynchronously wait for existing module requests to finish.

So, we would need to:

• make it possible to discard the existing async request and start a sync request, and swap the existing references
• make it possible to convert the existing async request into a sync request
• disallow importing the same module with multiple ways

Also, the same limitation from the system ESM applies to the sync module loader, where the top-level await is not allowed,
and it may require re-compiling a module with different CompileOptions even if an async request is already parsed.

Comment 6

[Tooru Fujisawa [:arai]]

There's possible consumer of loadSubScript with ESM (reusable components) in newtab, which has been using loadSubScript in order to utilize ScriptPreloader cache (context in bug 1416066 comment #22) for content scripts.

The HTML file is activity-stream-noscripts.html, and AboutNewTabChild.sys.mjs loads scripts into the global, instead of using <script> elements.

In this case, there's already a restriction that the page shouldn't use <script> elements, and applying the "disallow importing the same module with multiple ways" restriction will work, except for dynamic import which is used by customElements.js above.
So, if we implement sync loading semantics for dynamic import, this will work.

Comment 7

[Tooru Fujisawa [:arai]]

Possible design:

• add SyncModuleLoader class
• SyncModuleLoader is a singleton, and enabled only while importing single module graph synchronously
• if SyncModuleLoader is enabled, ModuleLoaderBase::GetCurrentModuleLoader returns the SyncModuleLoader
• add a ImportedSynchronously flag to each module, and it's set when loaded by SyncModuleLoader
• SyncModuleLoader is enabled with a reference to regular module loader
• SyncModuleLoader has its own module cache, but all cached modules are moved to the regular module loader's cache when finished importing the module graph
• SyncModuleLoader asserts that the requested module is either:
  • it doesn't exist in regular module loader, or
  • the module has ImportedSynchronously flag
• if there's any event that needs the regular module loader while SyncModuleLoader is enabled, SyncModuleLoader forwards the operation to the regular module loader (e.g. if asynchronously requested module file arrives)
• import with SyncModuleLoader is not re-entrant (except for import declaration), for simplicity

Comment 8

[Tooru Fujisawa [:arai]]

Now looking into customElements.js consumer, which imports moz-button-group.mjs, which imports lit.all.mjs.

https://searchfox.org/mozilla-central/rev/5d6e8d45a5cf4bdfa02b6187bde53f2d42f40be2/toolkit/content/customElements.js#821-824

case "moz-button-group":
  return import(
    "chrome://global/content/elements/moz-button-group.mjs"
  );

https://searchfox.org/mozilla-central/rev/5d6e8d45a5cf4bdfa02b6187bde53f2d42f40be2/toolkit/content/widgets/moz-button-group/moz-button-group.mjs#5

import { html } from "chrome://global/content/vendor/lit.all.mjs";

apparently lit.all.mjs is imported by many modules: https://searchfox.org/mozilla-central/search?q=lit.all.mjs&path=

I'm not sure if the "disallow importing the same module with multiple ways" rule works.

Comment 9

[Tooru Fujisawa [:arai]]

Testing a patch with the above option, except for forwarding part, which seems to be unnecessary.
https://treeherder.mozilla.org/jobs?repo=try&revision=f9c1674ec3e6ee88ab86142c2e77cd826793ddb2

Comment 10

[Tooru Fujisawa [:arai]]

With the "disallow importing the same module with multiple ways" fully implemented, and customElements rewritten with the sync module loader,
lit.all.mjs hits the issue:
https://treeherder.mozilla.org/jobs?repo=try&revision=15f38888d6a1fb801335e689590f9539e63be4e6&group_state=expanded&selectedTaskRun=FyaQgVYeT06RrSgdB-d-lg.0

So, there's surely a situation that single module is loaded with both loaders.

Possible options are:

• allow loading single modules with multiple ways, except for the case sync import happens while async import is ongoing (patch stack). this doesn't hit issue so far, but this introduces timing-dependent behavior:
  • sync import can sometimes fail
  • top-leval-await and dynamic import works only sometimes (only when async import happens first)
• allow all situations. when async import is ongoing, sync import rewrites the async import's request. I'm not sure if this really works. and this also introduces the timing-dependent behavior:
  • top-leval-await and dynamic import works only sometimes (only when async import completes first)
• keep disallowing. rewrite the consumers to use separate module graph for sync and async. this would work, but this will increase the memory consumption by duplicating the module graph

Comment 11

[Tooru Fujisawa [:arai]]

if all modules loaded into privileged globals are local file, we can use SyncModuleLoader in all privileged global by default,
This can avoid the issue with async vs sync mismatch, given all modules are sync-loaded.

The remaining issues with that way are top-level-await and dynamic import.

For dynamic import, if we can assume the all modules are local file, it can be implemented by existing sync import behavior and wrap the result with Promise.resolve.

For top-level-await, the proposal sounds like the deferred evaluation isn't supported for top-level-await.
This means even if we eventually switched to the standardized one, top-level-await is still problematic.
In web content, top-level-await disables the optimization (deferred evaluation), so we can have stricter rule for Firefox-internal code that top-level-await cannot appear in the module graph if people wants to lazify.

Then, using SyncModuleLoader allows another solution for the "load into current global", that is a combination with dynamic import + "unwrap promise" function.
Given the dynamic import will be implemented by sync import + Promise.resolve, it means the Promise is already resolved and we can get the result synchronously.

const { func } = UnwrapPromise(import("resource://.../Foo.mjs"));

Comment 12

[Tooru Fujisawa [:arai]]

bug 1875836 will add support for dynamic import in sync module loader.

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Bug 1803810 - Part 1: Rename ComponentModuleLoader to SyncModuleLoader. r?jonco!

The behavior is longer tightly associated with "component".

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Bug 1803810 - Part 2: Make module loader overridable. r?jonco!

In order to perform sync load in arbitrary global, add
ModuleLoaderBase::mOverriddenBy field, and let
ModuleLoaderBase::GetCurrentModuleLoader return the overridden loader,
so that the module loading can be overridden by SyncModuleLoader.

AutoOverrideModuleLoader manages the lifetime of the override.

Depends on D199453

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Bug 1803810 - Part 3: Add methods to copy and move imported modules. r?jonco!

In non-shared global, the sync load is performed by separate SyncModuleLoader,
with the following algorithm:

1. copy all loaded modules from async module loader, so that they can be
   used in the module graph in SyncModuleLoader
2. import a module graph
3. move all modules to async module loader, replacing fetching modules in
   async module loader with fetched modules if any

ModuleLoaderBase::CopyModulesTo is for step 1, and
ModuleLoaderBase::MoveModulesTo is for step 3.

Depends on D199454

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Bug 1803810 - Part 4: Add mozJSModuleLoader for non-shared global. r?jonco!

On the main thread, single mozJSModuleLoader instance is shared across all
loads in non-shared global, with resetting the internal state after importing
a module graph.

NonSharedGlobalSyncModuleLoaderScope manages the lifetime of each usage.
Import into the same non-shared global can be nested, but import into the
different non-shared gloobal is not allowed while other import for non-shared
global is ongoing.

Depends on D199455

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Bug 1803810 - Part 5: Add global option to ChromeUtils.importESModule. r?jonco!,ochameau!

Add { global: "current" } option to import the module into the current global,
either shared or non-shared.

Also add the following:

• { global: "shared" }: the existing default behavior, except for the implicit contextual behavior
• { global: "devtools" }: equivalent of loadInDevToolsLoader: true
• { global: "contextual" }: the explicit version of the contextual behavior

The later patch is going to drop the loadInDevToolsLoader option and also the
implicit contextual behavior, and require global option for modules in the
DevTools global.

Depends on D199456

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: Bug 1803810 - Part 6: Add tests for the main thread case. r?jonco!

Depends on D199457

Comment 19

[Tooru Fujisawa [:arai]]

Attachment: Bug 1803810 - Part 7: Add NonSharedGlobalSyncModuleLoaderScope::sTlsActiveLoader to hold per-thread loader. r?jonco!

In order to sync-import modules in workers, the non-shared loader needs to be
available also in worker.
The worker doesn't share the single sync loader, but use temporary loader for
each time, to simplify the lifetime management.

The reference to the thread-local sync loader is stored into
mozJSModuleLoader::sTlsNonSharedGlobalLoader while it's active, and the lifetime
is managed by NonSharedGlobalSyncModuleLoaderScope.

Depends on D199458

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Bug 1803810 - Part 8: Make mozJSModuleLoader::ImportESModule compatible with workers. r?jonco!

On worker, the module file is loaded on the main thread and passed to worker as
a string.

Depends on D199459

Comment 21

[Tooru Fujisawa [:arai]]

Attachment: Bug 1803810 - Part 9: Make ChromeUtils.importESModule compatible with workers. r?jonco!

Expose importESModule to worker as well, while only supporting
{ global: "current" } option.

Depends on D199460

Comment 22

[Tooru Fujisawa [:arai]]

Attachment: Bug 1803810 - Part 10: Add tests for worker case. r?jonco!

Depends on D199461

Comment 23

[Tooru Fujisawa [:arai]]

Attachment: Bug 1803810 - Part 8.5: Allow spinning event loop when waiting for ongoing fetching modules in worker. r?jonco!

Comment 24

[Tooru Fujisawa [:arai]]

Attachment: Bug 1803810 - Part 0: Synchronously fetch the entire module graph for dynamic import in worker, if the module is loaded from trusted scheme with system principal. r?smaug!,jonco!

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/656b61cbd15c
Part 1: Rename ComponentModuleLoader to SyncModuleLoader. r=jonco
https://hg.mozilla.org/integration/autoland/rev/4aa6e036fe7a
Part 2: Make module loader overridable. r=jonco
https://hg.mozilla.org/integration/autoland/rev/58160b9119ef
Part 3: Add methods to copy and move imported modules. r=jonco
https://hg.mozilla.org/integration/autoland/rev/84cdfd738db6
Part 4: Add mozJSModuleLoader for non-shared global. r=jonco
https://hg.mozilla.org/integration/autoland/rev/9699c18e5bf7
Part 5: Add global option to ChromeUtils.importESModule. r=jonco,ochameau
https://hg.mozilla.org/integration/autoland/rev/2e2f01296233
Part 6: Add tests for the main thread case. r=jonco
https://hg.mozilla.org/integration/autoland/rev/9593275c0195
Part 7: Add NonSharedGlobalSyncModuleLoaderScope::sTlsActiveLoader to hold per-thread loader. r=jonco
https://hg.mozilla.org/integration/autoland/rev/c02d879622bd
Part 8: Make mozJSModuleLoader::ImportESModule compatible with workers. r=jonco,dom-worker-reviewers,smaug
https://hg.mozilla.org/integration/autoland/rev/b8f45cbb5596
Part 9: Make ChromeUtils.importESModule compatible with workers. r=jonco
https://hg.mozilla.org/integration/autoland/rev/d5df64b38425
Part 10: Add tests for worker case. r=jonco

Comment 26

[Cosmin Sabou [:CosminS]]

Backed out for xpcshell failures on test_import_global.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=d5df64b38425761410eeadad051e241f0f83113d&selectedTaskRun=eiik8uHxSeaHPPd_NoEXSA.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=447109023&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/7682952aed897b4caf7ff9a744fdd5bc0fdddf43

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3d41616259c
Part 1: Rename ComponentModuleLoader to SyncModuleLoader. r=jonco
https://hg.mozilla.org/integration/autoland/rev/5937cea3bd81
Part 2: Make module loader overridable. r=jonco
https://hg.mozilla.org/integration/autoland/rev/747201bf7fd5
Part 3: Add methods to copy and move imported modules. r=jonco
https://hg.mozilla.org/integration/autoland/rev/3407176fa503
Part 4: Add mozJSModuleLoader for non-shared global. r=jonco
https://hg.mozilla.org/integration/autoland/rev/5cb4e71113c5
Part 5: Add global option to ChromeUtils.importESModule. r=jonco,ochameau
https://hg.mozilla.org/integration/autoland/rev/661f5fdef063
Part 6: Add tests for the main thread case. r=jonco
https://hg.mozilla.org/integration/autoland/rev/cbba5d464f03
Part 7: Add NonSharedGlobalSyncModuleLoaderScope::sTlsActiveLoader to hold per-thread loader. r=jonco
https://hg.mozilla.org/integration/autoland/rev/f0ae08143827
Part 8: Make mozJSModuleLoader::ImportESModule compatible with workers. r=jonco,dom-worker-reviewers,smaug
https://hg.mozilla.org/integration/autoland/rev/3289c7394894
Part 9: Make ChromeUtils.importESModule compatible with workers. r=jonco
https://hg.mozilla.org/integration/autoland/rev/8afc625dd4fd
Part 10: Add tests for worker case. r=jonco

Comment 28

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/d3d41616259c
https://hg.mozilla.org/mozilla-central/rev/5937cea3bd81
https://hg.mozilla.org/mozilla-central/rev/747201bf7fd5
https://hg.mozilla.org/mozilla-central/rev/3407176fa503
https://hg.mozilla.org/mozilla-central/rev/5cb4e71113c5
https://hg.mozilla.org/mozilla-central/rev/661f5fdef063
https://hg.mozilla.org/mozilla-central/rev/cbba5d464f03
https://hg.mozilla.org/mozilla-central/rev/f0ae08143827
https://hg.mozilla.org/mozilla-central/rev/3289c7394894
https://hg.mozilla.org/mozilla-central/rev/8afc625dd4fd