Closed Bug 1803810 Opened 2 years ago Closed 4 months ago

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

Categories

(Core :: XPConnect, enhancement)

enhancement

Tracking

()

RESOLVED FIXED
124 Branch
Tracking Status
firefox124 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(10 files, 2 obsolete files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

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.

Blocks: 1803678

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?

: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?

Flags: needinfo?(hjones)

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.

(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.

Flags: needinfo?(hjones)

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.

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.

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
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED

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.

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

Blocks: 1875638
Blocks: 1875639

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

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

Depends on: 1875836

The behavior is longer tightly associated with "component".

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

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

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

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

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

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

Depends on D199459

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

Depends on D199460

Depends on D199461

Attachment #9376173 - Attachment description: Bug 1803810 - Part 10: Add tests for workser case. r?jonco! → Bug 1803810 - Part 10: Add tests for worker case. r?jonco!
Depends on: 1877596
Attachment #9376170 - Attachment description: Bug 1803810 - Part 7: Add mozJSModuleLoader::sTlsNonSharedGlobalLoader to hold per-thread loader. r?jonco! → Bug 1803810 - Part 7: Add NonSharedGlobalSyncModuleLoaderScope::sTlsActiveLoader to hold per-thread loader. r?jonco!
Attachment #9377436 - Attachment is obsolete: true
Attachment #9378222 - Attachment description: WIP: Bug 1803810 - Part 0: WIP: Synchronously fetch the entire module graph for dynamic import in worker, if the module is loaded from trusted schem with system principal. → 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!
Attachment #9378222 - Attachment is obsolete: true
Pushed by arai_a@mac.com:
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
Pushed by arai_a@mac.com:
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
Flags: needinfo?(arai.unmht)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: