1656248 - Dynamic import() with relative path fetches wrong path after navigation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jeroen Versteeg]

Attachment: firefox-import-bug-master.zip

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

Steps to reproduce:

index.html contains
<script defer src="js/util.js">

js/util.js contains
import('./modules/mod.js');

Let browser load index.html.

Please see the attachment or the GitHub repo: https://github.com/drjayvee/firefox-import-bug

Actual results:

The first several loads cause the browser to correctly load
js/modules/mod.js. (Unfortunately, the MDN docs are rather vague on how module-names are translated to paths).

After several refreshes (using F5 or other navigation; cache is enabled and effective) however, the browser instead loads modules/mod.js.

Continuing refreshing does not fix this. Disabling cache in dev tools does fix! After a few refreshes, the same behavior repeats.

Expected results:

The import should keep using the same relative path.

Comment 1

[Jeroen Versteeg]

Forgot to mention that the attachment zip also contains a HAR log file.

Comment 2

[Virginia (please send need info request to Brindusa Tot)]

Hi Jeroen,

Thanks for your report and for attaching detailed files.

I'll add this ticket to the Core: Javascript Engine component in the hope someone from their team can take a look and comment.

Regards,
Virginia

Comment 3

[Nicolas B. Pierron [:nbp]]

Jon, would you be the best person to answer these questions, or investigate this issue?

Comment 4

[Jason Orendorff [:jorendorff]]

I agree with Jeroen's assessment, and I can reproduce this behavior using the github repo (with a server on localhost—I used python3 -m http.server).

The JS spec for import() says to call a hook named HostImportModuleDynamically that is defined in the HTML spec. Step 4.3 says "Set base URL to referencing script's base URL."

It would be surprising if the spec actually says that navigation affects script base URLs (admittedly navigation is full of unpleasant surprises). A bug, then...?

Comment 5

[Jon Coppeard (:jonco)]

I'll take a look. I can reproduce this pretty easily.

Comment 6

[Jon Coppeard (:jonco)]

We're not associating the loader script object with the JSScript for scripts loaded from the bytecode cache, and as a result we're getting the base URL wrong when resolving imports.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Bug 1656248 - Associate the JS script with the loader script for scripts loaded from the bytecode cache like we do for compiled scripts r?smaug

The problem is that ResolveModuleSpecifier is being passed a null script and is falling back to using the document base URL to resolve the module import (which is the correct thing to do in some cases). This is happening because the referring JS script was not assoicated with a loader script when it was loaded since it came from the bytecode cache and the logic to do that is missing on this path.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: Bug 1656248 - Add testcase that scripts loaded from the bytecode cache are correctly associated with a base URL r?smaug

Comment 9

[Jon Coppeard (:jonco)]

I'm trying to write a test case (see attached file) but I'm having trouble getting the bytecode cache to kick in. I'm repeatedly reloading an iframe which has an external script. If I set strategy to -1 it successfully encodes the script, but then it never uses the cached bytecode. Any ideas how to make this work? I just want to make sure a script is loaded from the bytecode cache. (Apparently you're on PTO so needinfoing you here).

Comment 10

[Nicolas B. Pierron [:nbp]]

I will look more in details of the test case that you wrote, but here is a suggested read for inspiration:
https://searchfox.org/mozilla-central/source/dom/base/test/test_script_loader_js_cache.html

Comment 11

[Jon Coppeard (:jonco)]

(In reply to Nicolas B. Pierron [:nbp] from comment #10)
Thanks, I've got this working now using some of those script loader test events.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/243a8dc843c4
Associate the JS script with the loader script for scripts loaded from the bytecode cache like we do for compiled scripts r=smaug
https://hg.mozilla.org/integration/autoland/rev/12c08680c172
Add testcase that scripts loaded from the bytecode cache are correctly associated with a base URL r=smaug

Comment 13

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/243a8dc843c4
https://hg.mozilla.org/mozilla-central/rev/12c08680c172