Closed Bug 1787926 Opened 2 years ago Closed 2 years ago

dynamic import() blocks or fails silently

Categories

(Core :: JavaScript Engine, defect, P1)

Firefox 105
defect

Tracking

()

RESOLVED FIXED
106 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox-esr102 --- unaffected
firefox104 --- unaffected
firefox105 + fixed
firefox106 --- fixed

People

(Reporter: mozbz, Assigned: jonco)

References

(Blocks 3 open bugs, Regression)

Details

(Keywords: regression)

Attachments

(2 files)

Attached file demo_site.zip (deleted) —

I noticed that one of my Firefox extensions stopped working after Developer Edition updated a couple of days ago. There are no errors thrown, but a script sits waiting for a Promise to resolve.

I've tried my best to boil the extension down to a small website (1 main js, 3 imports, status readout) which I've included with this report.

I've found three points - a top-level await, an import() function and a top-level import statement. Changing any one of these lines unblocks the demo's progress. I've marked these lines in the source files - one in each module in the /js folder.
I believe the sticking point is dynamically importing a module (lowlevel_api.js) which itself imports an already loaded module (state.js).

I can confirm that this site works in Firefox 104 and Chrome, but not in Developer Edition 105 or Nightly 106.

MozRegression points to this range including just bug1779421, which is why I'm filing the bug in this Component.

Steps to Reproduce:
Serve the included website and open /index.html.

Expected Results:
The site displays a status message, which should progress to a "Success!" message.

Actual Results:
The site gets stuck "Waiting for ready...".

Flags: needinfo?(jcoppeard)
Regressed by: 1779421

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

(In reply to M8R-p7 from comment #0)
Thanks for the bug report!

Assignee: nobody → jcoppeard

The bug has a release status flag that shows some version of Firefox is affected, thus it will be considered confirmed.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Severity: -- → S2
Status: NEW → ASSIGNED
Priority: -- → P1

The problem is that we're waiting for an import to finish evaluation after it
already has.

I think this a problem with the spec, related to the change from
AsyncEvaluating to AsyncEvaluation here:

https://github.com/tc39/proposal-top-level-await/commit/c2375dbd6ecb0d5e9a415f6a812fc0974a2935b7#diff-181371b08d71216599b0acccbaabd03c306da6de142ea6275c2135810999805aL588-R589

Previously we wouldn't add an async dependency when the required module's
AsyncEvaluating field was false, which was the case after evaluation had
finished. Now this is never set to false so we always add the dependency here
and there's nothing to trigger evaluation of the importing module.

What do you think?

Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/40afb18a8db3 Don't add async dependency on modules that have finished evaluating r=yulia
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 106 Branch
Flags: needinfo?(jcoppeard)

Reporter confirming that I'm seeing the Expected Results in the latest Nightly. Thank you!

Nightly 106.0a1 20220831093258 (rev/11e997d3cf78eb6a4f31a1e13a2509f4181f4b0a)

  • "Waiting for ready..."

Nightly 106.0a1 20220831215447 (rev/af1fc1e6eb24573a5ebad1754b9d4917e934a5f9)

  • "Success!"

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

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(jcoppeard)

Comment on attachment 9292255 [details]
Bug 1787926 - Don't add async dependency on modules that have finished evaluating r?yulia

Beta/Release Uplift Approval Request

  • User impact if declined: Possible web compat issue with dynamic module loading sometimes failing
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • 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 is a simple change that restores the original behaviour before bug 1779421 landed
  • String changes made/needed:
  • Is Android affected?: Yes
Flags: needinfo?(jcoppeard)
Attachment #9292255 - Flags: approval-mozilla-beta?

Comment on attachment 9292255 [details]
Bug 1787926 - Don't add async dependency on modules that have finished evaluating r?yulia

Approved for 105.0b7. Thanks for the report and Nightly verification, M8R-p7!

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

Attachment

General

Creator:
Created:
Updated:
Size: