1817259 - Use ES Modules for the worker debugger

Status: NEW

(DevTools :: Debugger, task, P3)

Description

[Alexandre Poirot [:ochameau]]

For now, DevTools starts by loading a regular worker script via WorkerDebugger::Initialize
https://searchfox.org/mozilla-central/rev/b579290e6b830d1b3f0a941203b0c0e1e56c42a3/dom/workers/WorkerDebugger.cpp#359
This is done from here, when DevTools wants to starts loading code in the worker thread:
https://searchfox.org/mozilla-central/rev/b579290e6b830d1b3f0a941203b0c0e1e56c42a3/devtools/server/connectors/worker-connector.js#36

We load the following file, which isn't using any module pattern (no CommonJS, nor ESM):
https://searchfox.org/mozilla-central/source/devtools/server/startup/worker.js

WorkerDebugger::Initialize ends up delegating the work to workerInternals:: LoadAllScripts and WorkerScriptLoader:
https://searchfox.org/mozilla-central/rev/b579290e6b830d1b3f0a941203b0c0e1e56c42a3/dom/workers/ScriptLoader.cpp#207

Today:

• if I use dynamic import(), I get Dynamic module import is disabled or not supported in this context
• the initial script can't use static import: import declarations may only appear at top level of a module. It is probably not loaded as a module itself.
• Using ChromeUtills.importESModule will lead to Hit MOZ_CRASH(mozJSModuleLoader not thread-safe)

I think that the last point is the most problematic.
Regardless of how we load the very first module in the debugger thread,
we will end up sharing common modules with the rest of DevTools.
Some of these modules will use ChromeUtils.defineESModuleGetters.
It would be nice if we could keep allowing lazy loading in modules loaded for worker debugging...

In order to move forward with the overall ESMification of this particular part of DevTools,
I would need a way to load one ESM, itself being able to load more.
i.e. this is fine if WorkerDebugger::Initialize keeps loading a non ESM as soon as there is some way to load an ESM from this initial script.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1817259 - [devtools] Test loading ES Modules for Worker Debugger JS code.

Comment 2

[Alexandre Poirot [:ochameau]]

I attached a mochitest, which help reproduce devtools environment from a mochitest without any dependecy other than the WorkerDebugger API.
Please comment the various import statement to easily reproduce the exception I got.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1817259 - [devtools] Force loading the worker debugger files as ES Modules.

Comment 4

[Alexandre Poirot [:ochameau]]

This is an issue with the WIP patch. The test patch has been updated to highlight the issue.

While it works fine loading the first main debugger worker modules from a resource:// URI,
loading any dependency from a resource:// URI would fail with a NS_ERROR_DOM_SECURITY_ERR on this code.
Loading deps via chrome:// URI works.

The issue is that all DevTools modules are loaded via resource:// URIs.

Comment 5

[Alexandre Poirot [:ochameau]]

Actually, the test script was wrong with the resource:// URI used within the main script.
It looks like debugger modules can be loaded just fine from the worker thread.
The main issue then is that importScript/loadSubScript can't be used from the ES Modules in workers.
This prevent doing an incremental refactoring as the CommonJS loader depends on loadSubScript.
It would force us to at least migrate all modules used in the worker thread to ESM right away.
This isn't necessarily a big deal, it would force us to start migrating our first CommonJS modules to ESM.
But there is an issue regarding lazy loading modules. For now we are using loader.lazyRequireGetter,
this is an equivalent for defineESModulesGetters. Unfortunately, as mozJSModuleLoader is flagged as main-thread only,
we can't use it from the worker thread.
This means that we would have to drop all lazy loading in favor of immediate loading via the import syntax.
This may regress the performance in the worker thread, but also in the main thread which is using the same modules.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: list of all devtools modules loaded in the worker thread

To have a sense of how many modules (and which one would be impacted), here is the list of all CommonJS/JSM loaded in the worker thread:

Comment 7

[Tooru Fujisawa [:arai]]

(In reply to Alexandre Poirot [:ochameau] from comment #5)

But there is an issue regarding lazy loading modules. For now we are using loader.lazyRequireGetter,
this is an equivalent for defineESModulesGetters. Unfortunately, as mozJSModuleLoader is flagged as main-thread only,
we can't use it from the worker thread.
This means that we would have to drop all lazy loading in favor of immediate loading via the import syntax.
This may regress the performance in the worker thread, but also in the main thread which is using the same modules.

Bug 1803810 patches will add the support for workers as well.
With it and bug 1875638, ChromeUtils.defineESModulesGetters(..., ..., { global: "current" }) will become available also in workers.

Comment 8

[Alexandre Poirot [:ochameau]]

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

Bug 1803810 patches will add the support for workers as well.
With it and bug 1875638, ChromeUtils.defineESModulesGetters(..., ..., { global: "current" }) will become available also in workers.

Oh that would be really handy!

Note that I tried with the current patch queue, but I'm getting the following assertion:

Assertion failure: mAsyncModuleLoader, at /mnt/desktop/gecko-dev/js/xpconnect/loader/mozJSModuleLoader.cpp:2036

Related code:

NonSharedGlobalSyncModuleLoaderScope::NonSharedGlobalSyncModuleLoaderScope(
    JSContext* aCx, nsIGlobalObject* aGlobal) {
  mAsyncModuleLoader = aGlobal->GetModuleLoader(aCx);
  MOZ_ASSERT(mAsyncModuleLoader);

#8  mozilla::loader::NonSharedGlobalSyncModuleLoaderScope::NonSharedGlobalSyncModuleLoaderScope(JSContext*, nsIGlobalObject*) (this=0x7ffffb350020, aCx=<optimized out>, aGlobal=0x7fee5605f9d0)
    at /mnt/desktop/gecko-dev/js/xpconnect/loader/mozJSModuleLoader.cpp:2036
#9  0x00007fee7b2beb7a in mozilla::Maybe<mozilla::loader::NonSharedGlobalSyncModuleLoaderScope>::emplace<JSContext*&, nsCOMPtr<nsIGlobalObject>&>(JSContext*&, nsCOMPtr<nsIGlobalObject>&)
    (this=0x7fee89b2fa60 <_IO_stdfile_2_lock>, this@entry=0x7ffffb350020, aArgs=@0x7ffffb34ff98: 0x7fee7202e100, aArgs=...) at /mnt/desktop/gecko-dev/obj-firefox/dist/include/mozilla/Maybe.h:845
#10 0x00007fee7b28ad23 in mozilla::dom::GetModuleLoaderForOptions(JSContext*, mozilla::dom::GlobalObject const&, mozilla::dom::ImportESModuleOptionsDictionary const&, mozilla::Maybe<mozilla::loader::NonSharedGlobalSyncModuleLoaderScope>&) (aCx=0x7fee7202e100, aGlobal=..., aOptions=..., aMaybeSyncLoaderScope=...) at /mnt/desktop/gecko-dev/dom/base/ChromeUtils.cpp:655
#11 0x00007fee7b28a5b4 in mozilla::dom::ChromeUtils::ImportESModule(mozilla::dom::GlobalObject const&, nsTSubstring<char16_t> const&, mozilla::dom::ImportESModuleOptionsDictionary const&, JS::MutableHandle<JSObject*>, mozilla::ErrorResult&) (aGlobal=..., aResourceURI=..., aOptions=..., aRetval=..., aRv=...) at /mnt/desktop/gecko-dev/dom/base/ChromeUtils.cpp:707
#12 0x00007fee7c34e2fd in mozilla::dom::ChromeUtils_Binding::importESModule(JSContext*, unsigned int, JS::Value*) (cx_=cx_@entry=0x7fee7202e100, argc=<optimized out>, vp=<optimized out>)
    at ./ChromeUtilsBinding.cpp:4933

Feel free to ignore for now, I'll provide a test once this large patch queue get landed.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1817259 - Tentative patch converting a worker JSM to ESM

Comment 10

[Tooru Fujisawa [:arai]]

Thank you for pointing that out.

Apparently the { global: "current" } option doesn't work in the sandbox which doesn't have corresponding window, simply because such sandbox doesn't have its own module loader.

https://searchfox.org/mozilla-central/rev/1e726a0e49225dc174ab55d1d0b21e86208d7251/js/xpconnect/src/Sandbox.cpp#2215,2220-2223

ModuleLoaderBase* SandboxPrivate::GetModuleLoader(JSContext* aCx) {
...
  JSObject* object = GetGlobalJSObject();
  nsGlobalWindowInner* sandboxWindow = xpc::SandboxWindowOrNull(object, aCx);
  if (!sandboxWindow) {
    return nullptr;

If there's actual need to load into such sandbox, outside of testcase, we should support regular modules there (e.g. dynamic import) first.

Comment 11

[Tooru Fujisawa [:arai]]

The sandbox seem to be created in the following:

https://searchfox.org/mozilla-central/rev/1e726a0e49225dc174ab55d1d0b21e86208d7251/devtools/shared/loader/Loader.sys.mjs#45,48-49,75-78,80,105

export function DevToolsLoader({
...
  useDevToolsLoaderGlobal = false,
} = {}) {
...
  const sharedGlobal = useDevToolsLoaderGlobal
    ? Cu.getGlobalForObject({})
    : undefined;
  this.loader = new Loader({
...
    sharedGlobal,
...
  });

https://searchfox.org/mozilla-central/rev/1e726a0e49225dc174ab55d1d0b21e86208d7251/devtools/shared/loader/base-loader.sys.mjs#493,533-535,540-545

export function Loader(options) {
...
  if (options.sharedGlobal) {
    sharedGlobal = options.sharedGlobal;
  } else {
...
    sharedGlobal = Sandbox({
      name: options.sandboxName || "DevTools",
      invisibleToDebugger: options.invisibleToDebugger || false,
      prototype: options.sandboxPrototype || globals,
      freshCompartment: options.freshCompartment,
    });

https://searchfox.org/mozilla-central/rev/1e726a0e49225dc174ab55d1d0b21e86208d7251/devtools/shared/loader/base-loader.sys.mjs#99,101,137,141,143

function Sandbox(options) {
...
  const sandboxOptions = {
...
    sandboxPrototype: "prototype" in options ? options.prototype : {},
...
  };
...
  return Cu.Sandbox(systemPrincipal, sandboxOptions);

So, if DevToolsLoader is called without useDevToolsLoaderGlobal: true, or if the current global doesn't have associated window,
the sandbox won't have associated window, and that results in not having ScriptLoader, and module loader doesn't work there.

Is there any reason that it doesn't have associated window?
Is there any chance that all sandboxes there get associated window object?
It could possibly be a window object in windowless browser, just like the testcase in https://phabricator.services.mozilla.com/D199458 uses (see createContentWindow or createChromeWindow)

Then, the other loader created by BrowserLoaderBuilder has associated window, so modules should work there.

https://searchfox.org/mozilla-central/rev/1e726a0e49225dc174ab55d1d0b21e86208d7251/devtools/shared/loader/browser-loader.js#108,112-113,121-122,188

function BrowserLoaderBuilder({
...
  window,
}) {
...
  const opts = {
    sandboxPrototype: window,
...
  this.loader = BaseLoader.Loader(opts);

Comment 12

[Alexandre Poirot [:ochameau]]

So I'm using global: "current" for the sake of making the import to work from the worker thread, per comment 7.
But these imports should rather be using global: "contextual" when they are loaded from the main thread (and going throught the DevToolsLoader codepath).

Would "contextual" also work from the worker thread?

Otherwise regarding you suggestion of making DevToolsLoader have a windowless browser's window... It sounds like. They are meant to load similarly to JSM/ESM and the "shared global". It sounds weird to make them be loaded in a window-like global.
Having said that, DevToolsLoader/Loader/BrowserLoader should all go away and be replaced by regular ESM, so if we ultimately have to workaround, it sounds ok to have such temporary hack.

Comment 13

[Tooru Fujisawa [:arai]]

As discussed on matrix, we'll support "contextual" also in worker thread, as an alias for "current", assuming the current global is where all special modules are loaded into, in the same way as devtools global.

Then, the Assertion failure: mAsyncModuleLoader case can be made runtime error, and we won't support loading modules into sandbox without associated window,
given the sandbox is only for commonJS modules, and all ESMified modules are supposed to be loaded into shared or devtools globals.

Comment 14

[Tooru Fujisawa [:arai]]

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

As discussed on matrix, we'll support "contextual" also in worker thread, as an alias for "current", assuming the current global is where all special modules are loaded into, in the same way as devtools global.

Then, the Assertion failure: mAsyncModuleLoader case can be made runtime error, and we won't support loading modules into sandbox without associated window,
given the sandbox is only for commonJS modules, and all ESMified modules are supposed to be loaded into shared or devtools globals.

For the record, the assertion about mAsyncModuleLoader should now be caught by the following line with runtime error:
https://searchfox.org/mozilla-central/rev/f63ca2952da98e0817bdae0ddf1314281a497106/dom/base/ChromeUtils.cpp#634-640

RefPtr targetModuleLoader = global->GetModuleLoader(aCx);
if (!targetModuleLoader) {
  // Sandbox without associated window returns nullptr for GetModuleLoader.
  JS_ReportErrorASCII(aCx, "No ModuleLoader found for the current context");
  return nullptr;
}

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Yesterday we discussed adding worker support to BiDi and based on Julian's feedback we need that bug fixed for it. He will most likely work on it.