Closed Bug 1540913 Opened 5 years ago Closed 1 year ago

Implement Dynamic import for workers

Categories

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

67 Branch
enhancement

Tracking

()

RESOLVED FIXED
113 Branch
Tracking Status
firefox113 --- fixed

People

(Reporter: tim57282, Assigned: yulia)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(11 files, 1 obsolete file)

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
48 bytes, text/x-phabricator-request
Details | Review

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.

Maybe related to Bug 1536094?

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.

Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
Component: DOM: Core & HTML → DOM: Workers

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

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

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.

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.

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

Component: DOM: Workers → JavaScript Engine
Depends on: 1342012
Flags: needinfo?(jcoppeard)
Priority: P3 → --

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

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.

Flags: needinfo?(jcoppeard)

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

Component: JavaScript Engine → DOM: Workers

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

(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

Type: defect → enhancement
Priority: -- → P3
Assignee: nobody → ystartsev
Depends on: 1247687, 1311726

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.

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

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.

Severity: normal → S3
Blocks: interop-2023
Blocks: interop-2023-modules
No longer blocks: interop-2023
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Summary: Dynamic module import doesn't work in a worker → Implement Dynamic import for workers
Keywords: dev-doc-needed

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.

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

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

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

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

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

Attachment #9317331 - Attachment description: WIP: Bug 1540913 - Implement Dynamic import for workers → Bug 1540913 - Part 7: Implement Dynamic import for workers; r=jonco

Depends on D169629

Attachment #9321305 - Attachment description: Bug 1540913 - Part 3: Adjust FinishDynamicImport to eagerly return if the promise has already been rejected; r=jonco → Bug 1540913 - Part 3: Exit early if worker module evaluation is aborted; r=jonco
Attachment #9317331 - Attachment description: Bug 1540913 - Part 7: Implement Dynamic import for workers; r=jonco → Bug 1540913 - Part 6: Implement Dynamic import for workers; r=jonco
Attachment #9321308 - Attachment is obsolete: true

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.

Flags: needinfo?(ystartsev)

I've answered the question in the github issue.

Flags: needinfo?(ystartsev)
Pushed by ystartsev@mozilla.com:
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
Regressions: 1822452
Regressions: 1822541

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

Flags: needinfo?(ystartsev)

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

Flags: needinfo?(ystartsev)

Sounds great, thanks!

Blocks: 1822699
Regressions: 1822937
Regressions: 1822839

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

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

Regressions: 1828130
No longer regressions: 1828992
You need to log in before you can comment on or make changes to this bug.