1842792 - Shipping <link rel="modulepreload"> causes breakage in some(?) ImportMap usage

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Dennis Schubert [:denschub]]

We've been made aware of this bug report in the Rails project about a regression in Firefox 115 that's affecting some Rails applications using importmaps. mozregression reliably points to bug 1425310.

There is a testcase in the form of a Rails application available here. If you have Ruby 3.2 on your system, you can start this with a

bin/bundle install --full-index
bin/rake db:prepare
bin/rake assets:precompile
bin/rails server

and unfortunately, I had no luck trying to build a somewhat more isolated testcase for it. Even downloading all the JS files and running them on a local static file server didn't reproduce, so this might be something else (maybe the preload header or something).

Either way, the relevant exception here is

Uncaught TypeError: The specifier “@hotwired/turbo-rails” was a bare specifier, but was not remapped to anything. Relative module specifiers must start with “./”, “../” or “/”.

from inside a script file (application-....js) that is pretty much just

import "@hotwired/turbo-rails"
import "controllers"

The HTML in question is

<script>
  console.log("Hello from non-imported script");
</script>
<script type="importmap" data-turbo-track="reload">
  {
    "imports": {
      "application": "/assets/application-d6fc2efb17d38ec5d072b865968a4b48cef4bc0fd7c4df74ba09d39ecfbc6e38.js",
      "@hotwired/turbo-rails": "/assets/turbo.min-f309baafa3ae5ad6ccee3e7362118b87678d792db8e8ab466c4fa284dd3a4700.js",
      "@hotwired/stimulus": "/assets/stimulus.min-d03cf1dff41d6c5698ec2c5d6a501615a7a33754dbeef8d1edd31c928d17c652.js",
      "@hotwired/stimulus-loading": "/assets/stimulus-loading-1fc59770fb1654500044afd3f5f6d7d00800e5be36746d55b94a2963a7a228aa.js",
      "controllers/application": "/assets/controllers/application-368d98631bccbf2349e0d4f8269afb3fe9625118341966de054759d96ea86c7e.js",
      "controllers/hello_controller": "/assets/controllers/hello_controller-549135e8e7c683a538c3d6d517339ba470fcfb79d62f738a0a089ba41851a554.js",
      "controllers": "/assets/controllers/index-2db729dddcc5b979110e98de4b6720f83f91a123172e87281d5a58410fc43806.js"
    }
  }
</script>
<link rel="modulepreload" href="/assets/application-d6fc2efb17d38ec5d072b865968a4b48cef4bc0fd7c4df74ba09d39ecfbc6e38.js" />
<link rel="modulepreload" href="/assets/turbo.min-f309baafa3ae5ad6ccee3e7362118b87678d792db8e8ab466c4fa284dd3a4700.js" />
<link rel="modulepreload" href="/assets/stimulus.min-d03cf1dff41d6c5698ec2c5d6a501615a7a33754dbeef8d1edd31c928d17c652.js" />
<link rel="modulepreload" href="/assets/stimulus-loading-1fc59770fb1654500044afd3f5f6d7d00800e5be36746d55b94a2963a7a228aa.js" />
<script src="/assets/es-module-shims.min-4ca9b3dd5e434131e3bb4b0c1d7dff3bfd4035672a5086deec6f73979a49be73.js" async="async" data-turbo-track="reload"></script>
<script type="module">
  import "application";
</script>

So this breakage kind of makes no sense, as application is imported after the importmap is defined, and since it can look up application, it should also be able to look up @hotwired/turbo-rails. So a completely unqualified hunch that I have is that somehow the module file gets interpreted before the importmap is evaluated, possibly due to the modulepreload links.

Comment 1

[Dennis Schubert [:denschub]]

zqianem, please have a look.

Comment 2

[Dennis Schubert [:denschub]]

[Tracking Requested - why for this release]:

I don't really know how to prioritize this. The frontend pipeline in use in the testcase is a very standard "new Ruby on Rails" setup, so it's probably in use in a lot of sites and applications. I don't know exactly under which conditions it breaks, but if it breaks, it's very reproducible for me - so this has the chance of doing a lot of damage.

Having the fact that 115 is an ESR in mind, and me currently not being aware of any breakage that was caused by us not supporting <link rel="modulepreload"> as there were shims available, this might be too risky to ship in an ESR unless we have a good understanding of what's happening here.

Comment 3

[Dennis Schubert [:denschub]]

I should have added this to the initial comment, but forgot:

Apparently, this breaks only with the first non-module <script> tag in place. If you remove that, it works.

Comment 4

[Dennis Schubert [:denschub]]

jftr, I deployed a Rails demo application to private infrastructure and provided :zqianem and :smaug with access to it. As soon as a public and static testcase will become available, it'll be attached to this bug.

Comment 5

[zqianem]

Investigating this now.

Just a clarification to comment 2 — the shims for <link rel="modulepreload"> did not work for recent versions of Firefox and are also explicitly turned off; but it's true that not supporting modulepreload would not cause any breakages, just slower loading on some sites.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

This looks like bug 1803984, which is landed in Firefox 116.
In short, the external script (who is doing the import “@hotwired/turbo-rails”) is preloaded (by the HTML parser), and import-map script tag is inline so it isn't preloaded. So when the preloaded script was fetched and evaluated it didn't have the information of the import-map hence caused the resolving failure.

Comment 7

[Dennis Schubert [:denschub]]

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

This looks like bug 1803984, which is landed in Firefox 116.

No, this bug reproduces in the current Nightly.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The bug is marked as tracked for firefox115 (release), tracked for firefox116 (beta) and tracked for firefox117 (nightly). However, the bug still isn't assigned.

:hsinyi, could you please find an assignee for this tracked bug? Given that it is a regression and we know the cause, we could also simply backout the regressor. If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 9

[mamhoff]

Hi! I found the bug reproduces on current Nightly (117), but not on Beta (116), interestingly. I hope this helps.

Comment 10

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Hi Will, delegating the NI from comment 8 to you, as your team has been much more involved in the planning and implementation for Importmap or modulepreload than my team, and seemed to at a better position to chime in for https://bugzilla.mozilla.org/show_bug.cgi?id=1842792#c2 Thank you.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Speculative load of modulepreload seems to cause this. I think we should just remove that code for now and then figure out how/if enable it again.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1842792, disable speculative load of modulepreload if parser has seen importmap, r=zqianem,jonco,allstarschh,peterv

This is backing out parts of bug 1425310

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

I believe this is basically the same bug Yoshi mentioned in comment 6. This just applies to modulepreload and that one was about speculative loads of <script>s.

Comment 14

[zqianem]

I ran a debugger against Nightly, and found the following:

• The inline import map <script type="importmap" data-turbo-track="reload"> is always processed first before the link modulepreloads, as expected
• Both the inline import map and link modulepreloads call ModuleLoaderBase::DisallowImportMaps(); I believe this is true for Firefox 114 as well
• After the calls above, the shim <script src="/assets/es-module-shims.min-4ca9b3dd5e434131e3bb4b0c1d7dff3bfd4035672a5086deec6f73979a49be73.js" async="async" data-turbo-track="reload"></script> inserts (replaces?) an import map just before <script type="module">import "application";</script>, which may explain why this bug happens intermittently — I think if this second import map is processed before the fetch starts for import "application", the fetch fails

Is it possible that the shim treats FF114 differently somehow compared to newer versions that have modulepreload support? Also, is this the expected behavior of processing a second import map after further import maps have been disallowed?

Comment 15

[zqianem]

Correction to the last bullet point: it's the dependent scripts that are racing against the second import map, since based on the console error, import "application" successfully resolves but import “@hotwired/turbo-rails” does not

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8fe09549077
disable speculative load of modulepreload if parser has seen importmap, r=jonco,allstarschh,peterv

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9343572 [details]
Bug 1842792, disable speculative load of modulepreload if parser has seen importmap, r=zqianem,jonco,allstarschh,peterv

Beta/Release Uplift Approval Request

• User impact if declined: Broken websites
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: (I tested this using denschub's testcase and he tested the patch too. Automated tests for this are hard since this all is very racy)
• List of other uplifts needed: NA
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This was the safest option I could think of, since it affects only speculative loads.
• String changes made/needed: NA
• Is Android affected?: Yes

Comment 18

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/a8fe09549077

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

(In reply to zqianem from comment #14)

I ran a debugger against Nightly, and found the following:

• The inline import map <script type="importmap" data-turbo-track="reload"> is always processed first before the link modulepreloads, as expected

speculative loading part of modulepreload gets processed every now and then before adding importmap to DOM

Comment 20

[Dianna Smith [:diannaS] - on Leave, back on August 9th]

Comment on attachment 9343572 [details]
Bug 1842792, disable speculative load of modulepreload if parser has seen importmap, r=zqianem,jonco,allstarschh,peterv

Approved for 116.0b6

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/e181cde22b3f

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9343572 [details]
Bug 1842792, disable speculative load of modulepreload if parser has seen importmap, r=zqianem,jonco,allstarschh,peterv

We aren't planning any more dot releases this cycle. This fix will ship in 116/115.1esr going out on August 1.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9343572 [details]
Bug 1842792, disable speculative load of modulepreload if parser has seen importmap, r=zqianem,jonco,allstarschh,peterv

Approved for 115.1esr.

Comment 24

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/80a254bffd37