1540913 - Implement Dynamic import for workers

Status: RESOLVED FIXED

(Core :: DOM: Workers, enhancement, P3)

Description

[Tim]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:67.0) Gecko/20100101 Firefox/67.0

Steps to reproduce:

Attempting to import an ES6 module inside a worker fails with "SyntaxError: dynamic module import is not implemented".
javascript.options.dynamicImport is set to true, and import works outside of a worker.

The code looks like this:
import("path_to_file_on_same_server").then(function() {
//do stuff
});

The worker is created like:
var worker=new Worker("filename_of_worker.js");

This works on Chrome (only tested 75).

An I doing something wrong? Is this not implemented/supported?

Actual results:

Threw an error: "SyntaxError: dynamic module import is not implemented"

Expected results:

No error, the module is loaded.

Comment 1

[Tim]

Maybe related to Bug 1536094?

Comment 2

[Rares Doghi, Desktop QA]

This is a bit to advanced for me to reproduce but I think the component for it is Core DOM: Core & HTML, If not please change the component to a more suitable one.

Comment 3

[Perry Jiang [:perry] [no longer employee, use ni?]]

I don't think this is implemented for Workers (so unfortunately MDN may be a little inaccurate). The dynamic import tests for Workers are are expected failures, too (e.g. https://searchfox.org/mozilla-central/source/testing/web-platform/meta/workers/modules/dedicated-worker-import.any.js.ini).

Comment 4

[Tim]

(In reply to Perry Jiang [:perry] from comment #3)

I don't think this is implemented for Workers (so unfortunately MDN may be a little inaccurate). The dynamic import tests for Workers are are expected failures, too (e.g. https://searchfox.org/mozilla-central/source/testing/web-platform/meta/workers/modules/dedicated-worker-import.any.js.ini).

Yeah I figured that was the case, but I didn't find a bug to implement this, so I opened this one.

Comment 5

[Tim]

So is there any movement/progress/interest in this?
Emscripten relies on dynamic module import in a worker for it's ES6 Module + pthreads + WASM implementation, so it would be nice if this wasn't Chrome exclusive.

Comment 6

[Daniel Huigens [:twiss]]

In the meantime, would it be possible to make this a runtime error rather than a syntax error, so that it's possible to if/else or try/catch around it? Currently, any worker script with import() in it fails to load completely, so using it in a backwards-compatible way (or even detecting support) would require eval, which is not ideal.

Comment 7

[Andrew Overholt [:overholt]]

Jon, do you know what's involved in making import() work in web workers?

Comment 8

[Andrea Marchesini [:baku]]

(In reply to Andrew Overholt [:overholt] from comment #7)

Jon, do you know what's involved in making import() work in web workers?

Currently, the module loading is supported only on the main thread. When loading a module, it can require the "sync" loading of submodules. This involves network operations and script parsing. So far, this is supported only on the main thread because it's strongly connected with the <script> element. If we want to support modules in workers, we need to take out the networking part and make it able to run on workers.

Alternatively, we can have an intermediate step, in which we support modules, but we don't allow "import". We do something similar in worklets.

Comment 9

[Jon Coppeard (:jonco)]

Indeed. There's a separate script loader for workers and as baku says networking is also different on workers (I don't know the details). So this requires factoring the current implementation out of the main thread script loader and integrating it into the worker script loader. It's quite a lot of work.

Comment 10

[Andrew Overholt [:overholt]]

Thank you for clarifying, baku and jonco. In the meantime, can we do as comment 6 suggests?

Comment 11

[Jon Coppeard (:jonco)]

(In reply to Andrew Overholt [:overholt] from comment #10)
Sure. I've filed bug 1580536.

Comment 12

[Tom Tung [:tt, :ttung]]

(In reply to Jon Coppeard (:jonco) from comment #11)

(In reply to Andrew Overholt [:overholt] from comment #10)
Sure. I've filed bug 1580536.

Let's take this as an enhancement and put into our backlog

Comment 13

[Katelyn Gadd (:kael)]

I'm running into this when integrating emscripten pthreads with our wasm stack - should I expect Firefox will not ever support this, or not support it anytime soon? If so I'll try and figure out a workaround, but if it's in the works that would be good to know.

Comment 14

[Katelyn Gadd (:kael)]

Sorry for comment spam: It's actually not clear to me how I could work around this, since it seems to be a syntax error - hiding the dynamic import in an if statement doesn't seem to work, and neither does a try/catch. I've seen people suggest using eval or new Function, but those aren't compatible with CSP. Is there a non-syntax-errory way to probe for support? Do I need to useragent probe (and will that work if FF freezes UAs in the future)?

Comment 15

[Yulia Startsev [:yulia]]

The work is currently in progress, and should land in the next iterations. The underlying issue is that we don't support modules at all for workers, and that work needs to be finished before this can be done. See bug #1247687 for the progress on that.

Comment 16

[tobiasuhlig78]

Hi guys,

I just played around with FF nightly v111. Now that the Support for JS modules inside the worker scope is done:
https://bugzilla.mozilla.org/show_bug.cgi?id=1247687

this ticket might be the last showstopper to get the neo.mjs dev mode running inside Firefox.

Chromium & Webkit support this for dedicated, shared & service workers for a long time already, so I would love to see Firefox catching up. Otherwise devs using neo have(!) to develop their apps in other browsers and are only able to test them inside the dist/dev or dist/prod environments for FF.

In case you need a rather complex app for testing if your feature works, feel free to use this one:
https://neomjs.github.io/pages/node_modules/neo.mjs/apps/covid/index.html

In case you need more infos about the neo.mjs project:
https://github.com/neomjs/neo

It would be highly appreciated if you can make this ticket a priority.

Best regards,
Tobi

Comment 17

[Yulia Startsev [:yulia]]

Attachment: Bug 1540913 - Part 6: Implement Dynamic import for workers; r=jonco

Comment 18

[Yulia Startsev [:yulia]]

Attachment: Bug 1540913 - Part 1: allow ModuleLoaders to return BaseURI without ScriptLoader;r=jonco

This is a required change for dynamic import on workers, as the ScriptLoader is not guaranteed to
live long enough. Once the initial script loading is finished, and the script has executed, the
scriptloader is cleared and the strong reference to the workerRef is cleared so that shutdown is
possible. The workerRef is required in order to access the worker private safely. In order to
address this, we move the GetBaseURI method to the module loader itself. In the future, we should
remove the script loader interface all together.

Comment 19

[Yulia Startsev [:yulia]]

Attachment: Bug 1540913 - Part 2: Clear and shutdown ModuleLoader at worker termination; r=asuth,jonco

When running an infinitely-looping dynamic import, it is possible for the module loader to still be
holding on to that module at shutdown. Shutdown on the worker means that we will no longer run the
event loop, which will execute the dynamic import promises necessary to clear the global. The result
is that the global is kept alive. By calling Shutdown() we prevent this partially. We additionally
need to address the failure in the module code (next patch).

Depends on D171682

Comment 20

[Yulia Startsev [:yulia]]

Attachment: Bug 1540913 - Part 3: Exit early if worker module evaluation is aborted; r=jonco

This change addresses the second issue around worker shutdown with infinitely running dynamic
imports: As the event loop is prevented from running when the worker is dying, we do not want to
delegate to the event loop in this case. This case always has a failing promise. We can instead
directly call the code that would have been called by the promise within FinishDynamicImport, rather
than waiting for a tick (that never comes) from the event loop.

Depends on D171683

Comment 21

[Yulia Startsev [:yulia]]

Attachment: Bug 1540913 - Part 4: Introduce WorkerScriptLoader configuration for dynamic import; r=jonco

Earlier, we introduced GetBaseURI to the module loader. This allows us to get the BaseURI for a
dynamic import even after the importing script/module's ScriptLoader has been cleaned up. However,
we now need to be able to handle the case where we need to run the dynamic import and load it. In
order to do this, we need to create a scriptloader configured for dynamic import. The most important
difference between this scriptloader and the one that is normally used for script loading in workers
is that we do not have a syncLoopTarget to which we return. There are a couple of reasons for
this:

• Dynamic import (and modules in general) relies on the event loop to resolve. If we create a
  syncLoop here, we will end up pausing execution, and this breaks the StartModuleLoad algorithm. We
  will never complete and the result will be that we are in the wrong state when we return here.
• We do not have perfect knowledge of the future, so we cannot keep the existing script loader alive
  in the case that it might be used for loading in the future.
• We cannot migrate the ModuleLoader to not use sync loading without significantly changing other
  aspects of how loading scripts on workers works. This becomes particularily evident with error
  handling
  (https://searchfox.org/mozilla-central/rev/00ea1649b59d5f427979e2d6ba42be96f62d6e82/dom/workers/WorkerPrivate.cpp#383-444),
  and in addition, for reasons I wasn't able to discern, using the CurrentEventTarget results in hard
  to identify errors. When there is time to investigate this fully, the ModuleLoader may move away
  from using a syncLoop itself.

For now, all main-script loads (whether they are modules or classic scripts) will use the sync loop,
and all dynamic imports will create a new script loader for their needs that is not using the sync
loop. The book keeping for this is in the next patch.

Depends on D171684

Comment 22

[Yulia Startsev [:yulia]]

Attachment: Bug 1540913 - Part 5: Track ScriptLoaders on WorkerLoadContext; r=jonco

As mentioned in the previous patch, this patch introduces tracking and shutdown of scriptloaders in
the case that we have multiple scriptloaders involved in module loading.

Depends on D171685

Comment 23

[Yulia Startsev [:yulia]]

Attachment: Bug 1540913 - Part 6: Do not shutdown WorkerScriptLoader for dynamic imports too early; r=jonco

Introduce an ergonomic api for checking if the current script loader is blocked on dynamic import or
not. If the scriptloader is a dynamic import, but is not the most recent one, it will be shut down
when the script completes or errors even if there is another dynamic import still in flight. This is
fine, as we will always have one living scriptLoader as long as there is one dynamic import, which will be shutdown when all dynamic imports are finished. Regular modules shut themselves down with no check for dynamic import.

Depends on D171686

Comment 24

[Yulia Startsev [:yulia]]

Attachment: Bug 1540913 - Part 8: Update wpt-tests; r=jonco

Depends on D169629

Comment 25

[Yulia Startsev [:yulia]]

Attachment: Bug 1540913 - Part 7: Add test to verify correct behavior of script loader shutdown; r=jonco

Depends on D169629

Comment 26

[Yulia Startsev [:yulia]]

Attachment: Bug 1540913 - Part 6-a: Fix BaseURL for importScripts; r=jonco

Depends on D169629

Comment 27

[Hamish Willee]

What's the best wpt test to run in order to show that import() works in workers? The context is this browser compatibility update: https://github.com/mdn/browser-compat-data/pull/19054.

I want to show the versions in which it is supported in workers, because that differs from the release version.

Comment 28

[Yulia Startsev [:yulia]]

I've answered the question in the github issue.

Comment 29

[Yulia Startsev [:yulia]]

Attachment: Bug 1540913 - Part 9: update fs tests to use dynamic import; r=janv

Depends on D171701

Comment 30

[Yulia Startsev [:yulia]]

Attachment: Bug 1540913 - Part 10: Update shadowRealms test to not fail when dynamic import is enabled; r=smaug

Depends on D172276

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed077e5a0595
Part 1: allow ModuleLoaders to return BaseURI without ScriptLoader;r=jonco
https://hg.mozilla.org/integration/autoland/rev/25d9fb6ea968
Part 2: Clear and shutdown ModuleLoader at worker termination; r=asuth,jonco
https://hg.mozilla.org/integration/autoland/rev/f577a4bae45c
Part 3: Exit early if worker module evaluation is aborted; r=jonco
https://hg.mozilla.org/integration/autoland/rev/bb0e194cca2c
Part 4: Introduce WorkerScriptLoader configuration for dynamic import; r=jonco
https://hg.mozilla.org/integration/autoland/rev/ab3b49bbd644
Part 5: Track ScriptLoaders on WorkerLoadContext; r=jonco
https://hg.mozilla.org/integration/autoland/rev/41438744db26
Part 6: Implement Dynamic import for workers; r=jonco
https://hg.mozilla.org/integration/autoland/rev/e9bc05ca2062
Part 6-a: Fix BaseURL for importScripts; r=jonco
https://hg.mozilla.org/integration/autoland/rev/aa0d0697ccd0
Part 7: Add test to verify correct behavior of script loader shutdown; r=jonco
https://hg.mozilla.org/integration/autoland/rev/433bd19b879e
Part 8: Update wpt-tests; r=jonco
https://hg.mozilla.org/integration/autoland/rev/90e3a819bce6
Part 9: update fs tests to use dynamic import; r=janv
https://hg.mozilla.org/integration/autoland/rev/341a8995a7fc
Part 10: Update shadowRealms test to not fail when dynamic import is enabled; r=smaug

Comment 32

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/ed077e5a0595
https://hg.mozilla.org/mozilla-central/rev/25d9fb6ea968
https://hg.mozilla.org/mozilla-central/rev/f577a4bae45c
https://hg.mozilla.org/mozilla-central/rev/bb0e194cca2c
https://hg.mozilla.org/mozilla-central/rev/ab3b49bbd644
https://hg.mozilla.org/mozilla-central/rev/41438744db26
https://hg.mozilla.org/mozilla-central/rev/e9bc05ca2062
https://hg.mozilla.org/mozilla-central/rev/aa0d0697ccd0
https://hg.mozilla.org/mozilla-central/rev/433bd19b879e
https://hg.mozilla.org/mozilla-central/rev/90e3a819bce6
https://hg.mozilla.org/mozilla-central/rev/341a8995a7fc

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Is this something we should call out in the Fx113 relnotes?

Comment 34

[Yulia Startsev [:yulia]]

It is still pref'd off while we do fuzzing. Maybe we can do a call out with the shipping bug? https://bugzilla.mozilla.org/show_bug.cgi?id=1812591

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Sounds great, thanks!

Comment 36

[Hamish Willee]

Is this behind the same pref as static import in workers? dom.workers.modules.enabled

Comment 37

[Yulia Startsev [:yulia]]

Yes, it is behind dom.workers.modules.enabled.