1860802 - Product quick view overlay no longer appears on consumentenbond.nl (vue.js)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Kestrel]

STR:

1. Visit https://www.consumentenbond.nl/tv/vergelijker.
2. Click on "Bekijk snel" ("Quick view") for a product.

Expected:
Overlay appears with a loading spinner followed by product details.

Actual:
Nothing.

Console error:

TypeError: can't access property "id", e.$store.state.data is undefined  vue.esm-browser.prod.js:1:15727
Hydration completed but contains mismatches.   vue.esm-browser.prod.js:1:47727
Uncaught TypeError: can't access property "id", e.children[0] is undefined  vergelijker line 312 > injectedScript:2:43056

Site uses vue.js.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=cd52a20c3eadcb6be9d11759fbfe6fe4a1f0c67f&tochange=ce70368fe145625608ba707f859b6262e49a3fde

Regressed by Bug 1842798.

Comment 1

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

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

For more information, please visit BugBot documentation.

Comment 2

[Jon Coppeard (:jonco)]

The consumentenbond.nl page creates four inline module script elements when "quick view" is clicked. Currently these are executed in a different order in FF to other browsers, and this leads to an exception being thrown and the overlay not appearing.

The first script has two direct imports and the other three have none. All the modules referenced have been previously loaded and are present in the module map. The are not fetched again.

Before bug 1842798 landed, the loading of an inline module scripts with no dependencies would complete synchronously whereas one with dependencies would complete via a MozPromise; currently both cases can complete synchronously if all dependencies are in the module map. This happens in ModuleLoaderBase::StartFetchingModuleDependencies:

https://searchfox.org/mozilla-central/source/js/loader/ModuleLoaderBase.cpp#850-902

To fix this issue we can try to revert to the previous behaviour. The module loader itself is no longer asynchronous itself, so the change must be to the derived DOM module loader.

So far I have been unable to come up with a spec justification for this. My reading is that module scripts should complete asynchronously via a microtask regardless of the number of imports, but implementing that did not fix this problem.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1860802 - Make non-parser-created inline module scripts with imports execute asynchronously r?smaug

This reverts to the previous behaviour where inline module scripts with
dependencies would complete asynchronously. Previously this would have been
internal to the module loader base class.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea3dba2fdcbe
Make non-parser-created inline module scripts with imports execute asynchronously r=smaug

Comment 5

[Atila Butkovits]

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

Comment 6

[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-firefox120 to wontfix.

For more information, please visit BugBot documentation.

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 9361184 [details]
Bug 1860802 - Make non-parser-created inline module scripts with imports execute asynchronously r?smaug

Beta/Release Uplift Approval Request

• User impact if declined: Web compat issues as described in the bug.
• 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 is a tricky area but the patch is small and reverts our behaviour to what it was before the regressing change so I'd say it was low risk.
• String changes made/needed: None
• Is Android affected?: Yes

Comment 8

[Dianna Smith [:diannaS]]

Comment on attachment 9361184 [details]
Bug 1860802 - Make non-parser-created inline module scripts with imports execute asynchronously r?smaug

Approved for 120.0b8

Comment 9

[Pulsebot]

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