Closed Bug 1873417 Opened 2 years ago Closed 2 years ago

Import map sometimes prevents module script execution

Categories

(Core :: JavaScript Engine, defect, P2)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
124 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- unaffected
firefox122 --- fixed
firefox123 --- fixed
firefox124 --- fixed

People

(Reporter: mozilla-bugzilla, Assigned: allstars.chh)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

import-map-module-script.zip
1.34 KB, application/zip
Details
Bug 1873417 : Cancel the preloaded request only if the it has been fetched.
48 bytes, text/x-phabricator-request
pascalc
: approval-mozilla-beta+
dmeehan
: approval-mozilla-release+
Details | Review

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

Steps to reproduce:

Inserting an import map into the DOM from a classic script sometimes leads to subsequent module scripts failing to execute:

<!DOCTYPE html>
<html lang="en">

<head>
    <script>
        (function () {
            const script = document.createElement('script');
            script.type = 'importmap';
            script.textContent = '{}';
            document.head.appendChild(script);
        }());
    </script>
</head>

<body>
    <script src="test.js" type="module" onerror="console.error('Failed to load ES Module script:', arguments[0])"></script>
</body>

</html>

Mozregression points to https://phabricator.services.mozilla.com/D194767 (and thus https://bugzilla.mozilla.org/show_bug.cgi?id=1865410) and from my testing this bug is not present in Firefox 120.

Actual results:

test.js only executes successfully sometimes (upon failure the Console shows the following warning: "Loading failed for the module with source").

The most consistent way to reproduce is to disable the cache in the Network panel. It also seems to reproduce more consistently in Firefox Nightly.

Note that the content of the import map is irrelevant for the reproduction (as long as it's valid JSON).

I have not been able to find any other relevant logs.

Expected results:

test.js should always execute.

Keywords: regression
Regressed by: 1865410

:jonco, since you are the author of the regressor, bug 1865410, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Flags: needinfo?(jcoppeard)
Assignee: nobody → allstars.chh
Flags: needinfo?(jcoppeard)
Severity: -- → S3
Priority: -- → P2

Fist, the module script bug_1873417.mjs is preloaded, then an import map is
inserted. Then ScriptLoader will cancel the preloaded request 1,
and create a new ScriptLoadRequest.
The new ScriptLoadRequest will be added into WaitingRequests in
mFetchingModules 2.
Finally, ScriptLoader finishes fetching the original preloaded ScriptLoadRequest,
but since it is cancelled, NS_BINDING_ABORTED will be passed to
OnFetchComplete 3 4 5, and then call ResumeWaitingRequests for the
new created ScriptLoadRequest 6, which in turn calls LoadFailed() 7.

To fix this, since the preloaded request isn't fetched yet, so I add a check
for the mModuleScript, if it doesn't exist yet, then we should reuse the
preloaded request.

Pushed by allstars.chh@gmail.com: https://hg.mozilla.org/integration/autoland/rev/11efb9ab43f6 Cancel the preloaded request only if the it has been fetched. r=jonco

https://hg.mozilla.org/mozilla-central/rev/11efb9ab43f6

Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
status-firefox124: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 124 Branch

Is there any chance this can be uplifted to Firefox 122? Web pages that rely on import maps inserted in this way currently fail to load at all in Firefox (or require clearing the cache or a few page reloads).

Flags: needinfo?(allstars.chh)

Please try nightly for a few days to see if it fixes your problem. If it doesn't cause any regression then I'll uplift it to beta or release (Firefox 122.0.1)

Flags: needinfo?(allstars.chh)

Set release status flags based on info from the regressing bug 1865410

status-firefox122: --- → affected
status-firefox123: --- → affected
status-firefox-esr115: --- → unaffected

The patch landed in nightly and beta is affected.
:allstars.chh, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox123 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(allstars.chh)

Comment on attachment 9375423 [details]
Bug 1873417 : Cancel the preloaded request only if the it has been fetched.

Beta/Release Uplift Approval Request

  • User impact if declined: Scripts might not be executed if it will inject an import map script.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This patch prevents a not-fetched-yet module script from being canceled after an import map is inserted.
  • String changes made/needed: no
  • Is Android affected?: Yes
Flags: needinfo?(allstars.chh)
Attachment #9375423 - Flags: approval-mozilla-beta?

I've tested with Nightly (124.0a1 (2024-01-24)), and while I can't reproduce anymore the test case attached, the application I've extracted it from still occasionally fails to load (with the same root cause - removing the insert of the import map ensures that the scripts are always executed).

The difference is that is happens less often and that the warning "Loading failed for the module with source" is no longer printed in the console. I'll try to see if I can extract another small test case, but it does seem like there are other conditions that result in scripts failing to execute.

Yoshi, given comment #10, should we pursue with the uplift request? Thanks

Flags: needinfo?(allstars.chh)

Thanks for asking.
I cannot reproduce the problem in comment 10 so far, maybe because I am using a debug build?
I'll try different build configurations to see if I can reproduce it, meanwhile, I'll cancel the uplift request by now.

Flags: needinfo?(allstars.chh)
Attachment #9375423 - Flags: approval-mozilla-beta?

:yoshi, we have a scheduled dot release for Fx122 next week. It builds on 2024-02-05 and releases on 2024-02-06.
I see there is some doubt about the fix, though is this something we should aim to get a fix in for the Fx122 dot release?
Or, if not then at least in time for Fx123.

Flags: needinfo?(allstars.chh)

Thanks for asking.
I am fixing Bug 1877122, hopefully, it will be fixed within this week so this bug and bug 1877122 can be uplifted to Fx122 dot release.

Flags: needinfo?(allstars.chh)
See Also: → 1877703

Comment on attachment 9375423 [details]
Bug 1873417 : Cancel the preloaded request only if the it has been fetched.

Beta/Release Uplift Approval Request

  • User impact if declined: Scripts might not be executed if it will inject an import map script.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: Bug 1877122
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This patch prevents a module script which is still being fetched or being compiled from being canceled after an import map is inserted.
  • String changes made/needed: no
  • Is Android affected?: Yes
Attachment #9375423 - Flags: approval-mozilla-beta?

Comment on attachment 9375423 [details]
Bug 1873417 : Cancel the preloaded request only if the it has been fetched.

Beta/Release Uplift Approval Request

  • User impact if declined: Scripts might not be executed if it will inject an import map script.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: Bug 1877122
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This patch prevents a module script which is still being fetched or being compiled from being canceled after an import map is inserted.
  • String changes made/needed: no
  • Is Android affected?: Yes
Attachment #9375423 - Flags: approval-mozilla-release?

Comment on attachment 9375423 [details]
Bug 1873417 : Cancel the preloaded request only if the it has been fetched.

Approved for 123 beta 6, thanks.

Attachment #9375423 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment on attachment 9375423 [details]
Bug 1873417 : Cancel the preloaded request only if the it has been fetched.

Approved for 122.0.1

Attachment #9375423 - Flags: approval-mozilla-release? → approval-mozilla-release+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: