1764239 - Crash at js::ModuleObject::getCycleRoot

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Alex Vincent [:WeirdAl]]

Possible duplicate of bug 1754892, based on https://crash-stats.mozilla.org/report/index/feab810d-b832-483b-8f72-507020220412

I have a ClassReviver module which attempts to recreate objects from stringified JSON. I've written it so that in reviving, if a subclass of a base class misses some property, the reviver throws in calling defineClass().

Well, the subclass did miss a property. The defineClass() method threw... and MOZ_RELEASE_ASSERT(cycleRoot.isObject()); failed. ddd says cycleRoot = $JS::UndefinedValue().

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: bug1764239.tar.bz2

Steps to reproduce:

(In a completely empty directory)

1. tar -xjf bug1764239.tar.bz2
2. npm install --only=prod
3. node playground.mjs
4. In Firefox, load http://localhost:3030/fixtures/book-grid-editable.html

Expected result:
JavaScript error: http://localhost:3030/objects/ClassReviver.mjs, line 45: Error: Class doesn't have a static fromJSON() method?

Actual result:
Assertion failure: cycleRoot.isObject(), at /home/ajvincent/compiled/central/mozilla-central/js/src/builtin/ModuleObject.cpp:1128

Comment 2

[Alex Vincent [:WeirdAl]]

Fixing the errors makes the crash go away.

Comment 3

[Nicolas B. Pierron [:nbp]]

Jon, Yulia, who might be the best person to look at this issue?

Also, knowing that this assertion is present in release builds, I wonder whether this crash should be considered as a security issue.

Comment 4

[Alex Vincent [:WeirdAl]]

(In reply to Nicolas B. Pierron [:nbp] from comment #3)

Also, knowing that this assertion is present in release builds, I wonder whether this crash should be considered as a security issue.

The only reason I marked it as such is that the bug this might be a duplicate of is also classified.

Comment 5

[Andrew McCreight (out of office until 8/21) [:mccr8]]

I'll give this the same rating as the other bug, then.

Comment 6

[Jon Coppeard (:jonco)]

I can reproduce this but I haven't managed to track it down yet. It's pretty strange that the cycle root is not set here.

Comment 7

[Alex Vincent [:WeirdAl]]

Is there anything I can do to assist? I tried posting an absolute-minimum testcase, but that failed to reproduce the bug.

Comment 8

[Jon Coppeard (:jonco)]

I can reproduce this failure with the following testcase:

let module1 = registerModule('module1', parseModule(
  `import {} from "module2";
   import {} from "module4";`));

let module2 = registerModule('module2', parseModule(
  `await import("module3");`));

let module3 = registerModule('module3', parseModule(
  ``));

let module4 = registerModule('module4', parseModule(
  `throw 1;`));

moduleLink(module1);

moduleEvaluate(module1);

drainJobQueue();

Comment 9

[Jon Coppeard (:jonco)]

Attachment: Bug 1764239 - Check for errors before accessing a module's CycleRoot field r?yulia

Like the changes in bug 1777972 concerning CycleRoot, this is not in the spec
but seems necessary due to the fact that this field isn't set when synchronous
module execution fails.

Comment 10

[Jon Coppeard (:jonco)]

This is a release assert failure and is not a security vulnerability.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a49298bd06fd
Check for errors before accessing a module's CycleRoot field r=yulia

Comment 12

[Cristina Cozmuta (:CrissCozmuta)]

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