2019262 - Copy error entries to the sync module loader in ModuleLoaderBase::CopyModulesTo

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task)

Description

[Tooru Fujisawa [:arai]]

bug 2018794 is caused by a single module being imported both by normal module loader and also the sync module loader,
where the normal module loader somehow fails to import it, but the sync module loader succeeds, for some reason,
and the sync module loader tries to move the module to the normal module loader, and it fails because there's an error entry.

https://searchfox.org/firefox-main/rev/1f43fe5ffadde0b6898daf607cabb3335dd75d6f/js/loader/ModuleLoaderBase.cpp#1685,1690-1695,1699,1703-1716

void ModuleLoaderBase::CopyModulesTo(ModuleLoaderBase* aDest) {
...
  for (const auto& entry : mFetchedModules) {
    RefPtr<ModuleScript> moduleScript = entry.GetData();
    if (!moduleScript) {
      continue;
    }
    aDest->mFetchedModules.InsertOrUpdate(entry, moduleScript);
...
void ModuleLoaderBase::MoveModulesTo(ModuleLoaderBase* aDest) {
...
  for (const auto& entry : mFetchedModules) {
    RefPtr<ModuleScript> moduleScript = entry.GetData();
    if (!moduleScript) {
      continue;
    }

#ifdef DEBUG
    if (auto existingEntry = aDest->mFetchedModules.Lookup(entry)) {
      MOZ_ASSERT(moduleScript == existingEntry.Data());
    }
#endif

    aDest->mFetchedModules.InsertOrUpdate(entry, moduleScript);
  }

While it's not really expected that single module is imported with both loaders, it looks like the situation is happening really widely.
In the dependency graph for moz-label.mjs, there are several modules that's imported both from <script> and ChromeUtils.import,
and fixing all of them doesn't look doable in the short term.

A short-term workaround is to copy the error entry also to the sync module loader, and avoid importing it but throw an error.
That way the assertion failure doesn't happen, but still it will cause an error.
If the normal module loader fails simply because the global is dying, the consumer of the ChromeUtils.import is also going away, and throwing an extra error might not be a too much trouble, but not sure...

Comment 1

[Tooru Fujisawa [:arai]]

Unfortunately this doesn't really solve the bug 2018794's testcase.
If we copy the error entry to the sync module loader, it means the ChromeUtils.importESModule fails during the custom element creation.

https://searchfox.org/firefox-main/rev/1f43fe5ffadde0b6898daf607cabb3335dd75d6f/toolkit/content/customElements.js#899-904

customElements.setElementCreationCallback(
  tag,
  function customElementCreationCallback() {
    ChromeUtils.importESModule(script, { global: "current" });
  }
);

and it hits the following assertion failure:
https://searchfox.org/firefox-main/rev/1f43fe5ffadde0b6898daf607cabb3335dd75d6f/dom/base/CustomElementRegistry.cpp#526-527,531-532

NS_IMETHODIMP
CustomElementRegistry::RunCustomElementCreationCallback::Run() {
...
  MOZ_ASSERT(NS_SUCCEEDED(er.StealNSResult()),
             "chrome JavaScript error in the callback.");

So, the code expects the import never fail.

Remaining option is the following:

• (a) Really rewrite the all consumers of ChromeUtils.importESModule(..., {global: "current"}) not to import modules that are imported also from <script> or dynamic import
• (b) Modify CustomElementRegistry to allow errors
• (c) Overwrite the error entry with the successful entry
• (d) Do not overwrite the error entry with the successful entry, but still use the successful entry for the ongoing import

as mentioned above, (a) is really difficult and huge task.
(b) maybe simpler, and if we expect such error (e.g. when the window is quickly closed), CustomElementRegistry::RunCustomElementCreationCallback::Run should really expect error case and somehow propagate the error to the consumer.
(c) may work for some case, but it's risky, as it makes the race more complex.
(d) may also work, but it means the modules that failed on the normal module loader can be evaluated multiple times, as it's not registered to the global's normal module loader

I'll look into (b).

Comment 2

[Tooru Fujisawa [:arai]]

the custom element part is handled by bug 2020008.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Bug 2019262 - Propagate the error status of the modules from/to SyncModuleLoader, to avoid importing single module twice even for error case. r?jonco!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/bf1873b92859
https://hg.mozilla.org/integration/autoland/rev/8024c7d8af52
Propagate the error status of the modules from/to SyncModuleLoader, to avoid importing single module twice even for error case. r=jonco

Comment 5

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/8024c7d8af52