1787926 - dynamic import() blocks or fails silently

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[M8R-p7]

Attachment: demo_site.zip

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...".

Comment 1

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

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

Comment 2

[Jon Coppeard (:jonco)]

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

Comment 3

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

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

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1787926 - Don't add async dependency on modules that have finished evaluating r?yulia

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?

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40afb18a8db3
Don't add async dependency on modules that have finished evaluating r=yulia

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/40afb18a8db3

Comment 7

[M8R-p7]

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!"

Comment 8

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

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.

Comment 9

[Jon Coppeard (:jonco)]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/53bd7ef331db

Comment 12

[Jon Coppeard (:jonco)]

I filed a spec issue here: https://github.com/tc39/ecma262/issues/2895