Closed Bug 1803984 Opened 1 year ago Closed 10 months ago

importmap fails to map imports for external scripts when the scripts are served locally.

Categories

(Core :: JavaScript Engine, defect, P3)

Product:
Core
Component:
JavaScript Engine
Version:
Firefox 107
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
116 Branch
Tracking Flags:
Tracking Status
firefox116 --- fixed

People

(Reporter: 1679278, Assigned: allstars.chh)

References

Details

Crash Data

Attachments

(5 files)

Bug 1803984 - Part 1: Don't use the preloaded module request if there's an import map.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1803984 - Part 3: Add tests for the interaction between speculative preloading and module import maps
48 bytes, text/x-phabricator-request
Details | Review
Bug 1803984 - Part 2: Add assertion that we only remove unlinked modules from the map
48 bytes, text/x-phabricator-request
Details | Review
Bug 1803984 - Part 4: Move test to mochitest-plain test.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1803984 - Part 5: Check IsModuleFetched() before removing it.
48 bytes, text/x-phabricator-request
Details | Review

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:107.0) Gecko/20100101 Firefox/107.0

Steps to reproduce:

dom.importMaps.enabled = True

thisworks.html:

<!DOCTYPE html>
<html>
	<head>
		<meta charset="utf-8">
		<title>This will work</title>
		<style>
			body { margin: 0; }
		</style>
		<script type="importmap">
		{
			"imports": {
			"three": "https://cdn.skypack.dev/three"
			}
		}
		</script>
	</head>
	<body>
		<script type="module">
			import * as THREE from 'three';
			console.log("THREE has been imported:", typeof THREE);
		</script>
	</body>
</html>

bug.js:

import * as THREE from 'three';
console.log("This doesn't work:", typeof THREE);

thisdoesnotwork.html:

<!DOCTYPE html>
<html>
	<head>
		<meta charset="utf-8">
		<title>This will NOT work</title>
		<style>
			body { margin: 0; }
		</style>
		<script type="importmap">
		{
			"imports": {
			"three": "https://cdn.skypack.dev/three"
			}
		}
		</script>
	</head>
	<body>
		<script type="module" src="bug.js"></script>
	</body>
</html>

Actual results:

The inline script works fine, but the sourced script does not recognise the mapped imports.

Expected results:

Both cases should have worked.

The specific error message in the console is:

Uncaught TypeError: The specifier “three” was a bare specifier, but was not remapped to anything. Relative module specifiers must start with “./”, “../” or “/”. bug.js:1:23

The Bugbug bot thinks this bug should belong to the 'Core::DOM: Core & HTML' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
Component: DOM: Core & HTML → JavaScript Engine
Flags: needinfo?(allstars.chh)

Hi,
I have met this before, if the external module script is from a local HTTP server, because Gecko will do speculative parsing, so the module script will be preloaded before the import map is loaded, therefore the specifier couldn't be resolved.
If the HTML is hosted on a remote server (github in my example) then the problem went away.

Do your test pages (HTML and bug.js) come from a local HTTP server?

Thanks

Flags: needinfo?(allstars.chh) → needinfo?(1679278)

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #3)

Do your test pages (HTML and bug.js) come from a local HTTP server?

Yes.

As I understand, this is a race condition then?

Flags: needinfo?(1679278)

(In reply to A from comment #4)

As I understand, this is a race condition then?

Yes, because the module script will be preloaded by speculative parser, but the import map script tag won't.
The preloading of import-map will be done in Bug 1762595, but it needs to be implemented together with external import-maps. (Bug 1765745).

Please try to host your scripts/HTMLs in a remote web server, if the problem still exists, please provide the URL so I can test it as well.

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #5)

Please try to host your scripts/HTMLs in a remote web server, if the problem still exists, please provide the URL so I can test it as well.

Very nice catch!

  • I have tested on a remote web server and the problem goes away. 👍
  • If I test locally via python -m SimpleHTTPServer or similar the problem manifests itself, as reported. 👎
  • If I test locally via `ssh -L8001:localhost:443 remoteserver.example.com (and accept the certificate warning) the problem still goes away (because of latency?). 👍
  • All of the above is still true if I change the import map as follows (so that it imports from the same server): ✔
		<script type="importmap">
		{
			"imports": {
			"three": "/three.module.js"
			}
		}
		</script>

Thanks for the clarification.
I'll duplicate this bug with Bug 1762595, as this problem should go away when we have speculative parsing of import-maps.

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #5)

Yes, because the module script will be preloaded by speculative parser, but the import map script tag won't.

This seems bad for developers as it's easy to trigger this problem and not obvious what's going on.

Is there some way we can fix this? The spec says to disallow futher import maps when a module script is fetched. Can we delay this while we're preloading?

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

This seems bad for developers as it's easy to trigger this problem and not obvious what's going on.

Is there some way we can fix this?

See Bug 1762595 for details, we could preload import-maps, but that will also bring other problems, like https://bugzilla.mozilla.org/show_bug.cgi?id=1762595#c7
and https://github.com/WICG/import-maps/issues/281

Because speculative parsing is implementation-dependant and external import-maps isn't supported now so we don't preload import-maps yet (As there is other corner-case like <modulepreload>)

The spec says to disallow futher import maps when a module script is fetched. Can we delay this while we're preloading?

In our implementation, if the module load is of type preload, we won't disallow import-maps, otherwise import-maps won't work because we will preload modules.
See https://searchfox.org/mozilla-central/rev/a44deb21744de6269329b05461c55488a147f95e/dom/script/ScriptLoader.cpp#989
The disallow is called when we try to re-use the preloaded module load.

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #9)

In our implementation, if the module load is of type preload, we won't disallow import-maps, otherwise import-maps won't work because we will preload modules.

OK I'm confused then. I thought that was the problem here (and this is the fix I was going to suggest).

Going back to the description of the problem:

so the module script will be preloaded before the import map is loaded

If we don't disallow import maps until the preload is used, why is there a problem? The module script preload should only get used after we have parsed the import map as far as I understand it.

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

OK I'm confused then. I thought that was the problem here (and this is the fix I was going to suggest).

Let me explain in more detail.

I guess you think the problem is:

  1. We preload the module
  2. We disallow the import-map
  3. import map is not loaded because it is disallowed.
  4. module script fails to resolve the specifier 'THREE'.

But it is not.
Because we actually don't do step 2 (disallow import-map when doing preload).
So the import map tag here is actually registered.

Is this what you think?

Going back to the description of the problem:

so the module script will be preloaded before the import map is loaded

If we don't disallow import maps until the preload is used, why is there a problem? The module script preload should only get used after we have parsed the import map as far as I understand it.

As mentioned above, we don't disallow import-maps in this case, as the (normal) module load happens after the import map is loaded.

What happened here are:

  1. Preload the module script (bug.js in this case)
  2. Parse the import map
  3. Load the external module script, bug.js. ScriptLoader found this is a preloaded request, so it reuses the preload request.
  4. Module script is fetched, then the module script is instantiated, but import-map is still being parsed, so it fails to resolve the specifier.

So the problem is the preloaded module script is used before the import-map is parsed.
It happens because the fetching happens before the import-map is parsed, and also the script is from a local server and its size is small,
so the fetching finishes very quickly.

Perhaps we could delay the preloaded module script or discard it and start a new load
But we don't know if the module script will actually use the specifier from the import-map
And it might hurt the performance for pages using import-maps.

Or we could preload the import-map as well (bug 1762595), but that brings some other problem as mentioned in comment 9.

We could also consider together with <modulepreload>, so let webpage itself decide whether to preload the module script or not.

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #11)

Is this what you think?

That is what I thought originally.

What happened here are:

  1. Preload the module script (bug.js in this case)
  2. Parse the import map
  3. Load the external module script, bug.js. ScriptLoader found this is a preloaded request, so it reuses the preload request.
  4. Module script is fetched, then the module script is instantiated, but import-map is still being parsed, so it fails to resolve the specifier.

What I don't understand is that if we parse the import map in step 2, how is it still being parsed in step 4? Does this not all happen synchronously on the main thread?

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

What I don't understand is that if we parse the import map in step 2, how is it still being parsed in step 4? Does this not all happen synchronously on the main thread?

Sorry, I miss something, let me correct my comment 11.

  1. Preload the module script.
  2. the module is fetched, but fails to resolve the specifier so now it has parse error.
  3. Parse the import map.
  4. Load the external module script, and reuse the preloaded request.
  5. The preloaded request has parse error (from step 2), SM throws the error.

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #13)
Aha, that explains it. Thanks.

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #7)

Thanks for the clarification.
I'll duplicate this bug with Bug 1762595, as this problem should go away when we have speculative parsing of import-maps.

Many thanks for your help and the clear analysis.

Based on comment 9, I'm marking this as depending on bug 1762595 and setting priority and severity the same as that bug.

Severity: -- → S4
Status: UNCONFIRMED → NEW
Depends on: 1762595
Ever confirmed: true
Priority: -- → P3
Blocks: importmaps
No longer blocks: js-lang
No longer depends on: 1762595
Summary: importmap fails to map imports for sourced scripts → importmap fails to map imports for external scripts when the scripts are served locally.

Is there a workaround for this? Can speculative parsing be disabled? It makes development very annoying; I have to reload on average 3 times before it's working.

I also don't understand S4 severity. This should affect everyone who is using import maps with a local development server.

I could check if this can be fixed without preloading the import-maps (bug 1762595), but this release will be freeze soon, so probably in the next release.

Assignee: nobody → allstars.chh

Hello, I reported bug 1833371, which may contain useful information.

I can confirm that this bug can occur on non-local web servers as I have observed it on a github page, here is the github issue that was open with us:
https://github.com/odoo/owl/issues/1436
and the state of the code at the time:
https://github.com/odoo/owl/blob/aa441274c7c588aeed504e1ee1ef19c44d411d20/docs/index.html

As noted in 1833371 the presence of other non-module scripts in the page before the import map seems to make the bug more likely. I don't know much about firefox internals but if I had to hazard a guess, it seems likely that it's because the non-module script is delaying the parsing of the import map.

I've created a fork of the above repo at that point in git history and enabled the github page, which showcases this issue happening on a web server: https://degueldre.dev/firefox-import-map-bug-demo/

https://bugdash.moz.tools/ is another site that exhibits this behaviour, although normally during a session restore; see issue 15 for some more details. I didn't hit this issue at all during development, locally or when fully hosted on Heroku.

I also don't understand S4 severity

I agree, S4 is defined as minor significance, cosmetic issues, low or no impact to users.

This should be at least S3. S3 requires a work-around (Blocks non-critical functionality and a work around exists). Technically rewriting your application to not use importmaps is a work-around, but not one that is always viable.

Will - would it be possible for you to consider retriaging this issue?

Flags: needinfo?(wmedina)

(In reply to :glob ✱ from comment #21)

https://bugdash.moz.tools/ is another site that exhibits this behaviour, although normally during a session restore; see issue 15 for some more details. I didn't hit this issue at all during development, locally or when fully hosted on Heroku.

I also don't understand S4 severity

I agree, S4 is defined as minor significance, cosmetic issues, low or no impact to users.

This should be at least S3. S3 requires a work-around (Blocks non-critical functionality and a work around exists). Technically rewriting your application to not use importmaps is a work-around, but not one that is always viable.

Will - would it be possible for you to consider retriaging this issue?

Severity: S4 → S3
Flags: needinfo?(wmedina)
Attachment #9334547 - Attachment description: Bug 1803984 - Don't use the preloaded request if it has an error and there's an import map. → Bug 1803984 - Don't use the preloaded module request if there's an import map.

It would not be correct to remove modules that were already linked or evaluated.

Attachment #9335422 - Attachment description: Bug 1803984 - Add assertion that we only remove unlinked modules from the map r?allstarschh → Bug 1803984 - Add assertion that we only remove unlinked modules from the map
Attachment #9335409 - Attachment description: Bug 1803984 - Add tests for the interaction between speculative preloading and module import maps r?allstarschh → Bug 1803984 - Add tests for the interaction between speculative preloading and module import maps
Pushed by allstars.chh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3bc8c30ecc25
Don't use the preloaded module request if there's an import map. r=jonco
https://hg.mozilla.org/integration/autoland/rev/bcbcbdf4f449
Add assertion that we only remove unlinked modules from the map r=allstarschh
https://hg.mozilla.org/integration/autoland/rev/d6316cfb118d
Add tests for the interaction between speculative preloading and module import maps r=allstarschh

Backed out for causing top crash https://bugzilla.mozilla.org/show_bug.cgi?id=1835468 on nightly
Backout: https://hg.mozilla.org/mozilla-central/rev/c10ba3b6a8ecfaa3bebc0a33d8740e61594766c2

Status: RESOLVED → REOPENED
status-firefox115: fixed → ---
Resolution: FIXED → ---
Target Milestone: 115 Branch → ---
Duplicate of this bug: 1835468

Copying crash signatures from duplicate bugs.

Crash Signature: [@ JS::loader::ModuleLoaderBase::RemoveFetchedModule]

The bug is linked to a topcrash signature, which matches the following criteria:

  • Top 10 desktop browser crashes on nightly
  • Top 10 AArch64 and ARM crashes on nightly

:allstars.chh, could you consider increasing the severity of this top-crash bug?

For more information, please visit BugBot documentation.

Flags: needinfo?(allstars.chh)
Keywords: topcrash
Regressions: 1835337

it has been backout already.

Flags: needinfo?(allstars.chh)
Duplicate of this bug: 1837044

Based on the topcrash criteria, the crash signature linked to this bug is not a topcrash signature anymore.

For more information, please visit BugBot documentation.

Keywords: topcrash
Attachment #9334547 - Attachment description: Bug 1803984 - Don't use the preloaded module request if there's an import map. → Bug 1803984 - Part 1: Don't use the preloaded module request if there's an import map.
Attachment #9335422 - Attachment description: Bug 1803984 - Add assertion that we only remove unlinked modules from the map → Bug 1803984 - Part 2: Add assertion that we only remove unlinked modules from the map
Attachment #9335409 - Attachment description: Bug 1803984 - Add tests for the interaction between speculative preloading and module import maps → Bug 1803984 - Part 3: Add tests for the interaction between speculative preloading and module import maps

Move the test to mochitest-plain, as I'll use HTTP redirect to reproduce
the crash in later patch.

The crash of https://bugzilla.mozilla.org/show_bug.cgi?id=1835468 can be
reproduced by HTTP redirect in mochitest-plain.

When the script is being processed, sometimes the preloaded request is
still being loaded (for example, the script is being redirected to
another place). So we use the IsModulleFetched() check before calling
RemoveFetchedModule.

Pushed by allstars.chh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/24f62f68f45a
Part 1: Don't use the preloaded module request if there's an import map. r=jonco
https://hg.mozilla.org/integration/autoland/rev/87fcd0af9ab0
Part 2: Add assertion that we only remove unlinked modules from the map r=allstarschh
https://hg.mozilla.org/integration/autoland/rev/bf84446fdef4
Part 3: Add tests for the interaction between speculative preloading and module import maps r=allstarschh
https://hg.mozilla.org/integration/autoland/rev/e0a7ff16a8ed
Part 4: Move test to mochitest-plain test. r=jonco
https://hg.mozilla.org/integration/autoland/rev/a1c94c7f0839
Part 5: Check IsModuleFetched() before removing it. r=jonco
QA Whiteboard: [qa-116b-p2]
Regressions: 1865410
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: