1965620 - JSON Import Crash

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[guybedford]

Steps to reproduce:

Import the following module in Firefox:

main.js

import { m } from './dep.js';
console.log(m);

dep.js

import json from './json.json' with { type: 'json' }
export { json as m }

Actual results:

The browser window crashes. Note that adding any execution lines into the dep.js avoids the issue, eg:

import json from './json.json' with { type: 'json' }
let p;
export { json as m }

Expected results:

This should not crash

Comment 1

[guybedford]

Correction to the workaround above, is to write:

import json from './json.json' with { type: 'json' };
let foo = json;
export { foo as m }

Comment 2

[Mayank Bansal]

Confirmed.

Bisection nightly:

• Bug 1950836 - Remove the pref in ini files. r=jonco,dminor
• Differential Revision: https://phabricator.services.mozilla.com/D240003

Bisection with javascript.options.experimental.import_attributes = True

• Bug 1877792 - Part 20: Fix URL encoding in JSON module WPT tests r=spidermonkey-reviewers,jonco
• Differential Revision: https://phabricator.services.mozilla.com/D218212

Comment 3

[Mayank Bansal]

Attachment: Reporters Standlone singlefile Testcase.HTML

just open the HTML file directly in the browser, or save locally and directly open.

AR: Crash

Comment 4

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

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

:jon4t4n, since you are the author of the regressor, bug 1877792, could you take a look?

For more information, please visit BugBot documentation.

Comment 5

[Jonatan Klemets [:jon4t4n]]

I think the problem is that we end up calling CyclicModuleResolveExport, but we need to check the module type and call SyntheticModuleResolveExport. If we refactor the code to call ModuleResolveExport then this is automatically handled.

Try run with a quick fix: https://treeherder.mozilla.org/jobs?repo=try&revision=9580b106a21a8708f2fbe4e4dbe4e495d5d81da7

Comment 6

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1965620 - Fix synthetic module handling in CyclicModuleResolveExport r=#spidermonkey-reviewers

Indirect export entries can contain synthetic modules, so we need to
check the type and call the correct method for resolving exports.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30d174015838
Fix synthetic module handling in CyclicModuleResolveExport r=spidermonkey-reviewers,jonco

Comment 8

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/30d174015838

Comment 9

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

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

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox139 to wontfix.

For more information, please visit BugBot documentation.

Comment 10

[Jon Coppeard (:jonco) (PTO until Monday 29th)]

Comment on attachment 9486753 [details]
Bug 1965620 - Fix synthetic module handling in CyclicModuleResolveExport r=#spidermonkey-reviewers

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Possible crashes when sites use JSON modules.
• 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 simple fix and has been on central for a week.
• String changes made/needed: None
• Is Android affected?: Yes

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9486753 [details]
Bug 1965620 - Fix synthetic module handling in CyclicModuleResolveExport r=#spidermonkey-reviewers

Approved for 139.0rc2.

Comment 12

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/b4c95d35f98f