1463373 - Self-hosted JavaScript assertion info: "js/src/builtin/Module.js:354: Bad module status in ModuleDeclarationInstantiation" with ES6 modules

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision dc1868d255be (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --no-threads):

setModuleResolveHook(function(module, specifier) { return lfLastModule; });
let m = parseModule(`
  let moduleRepo = {};
  let c = moduleRepo["c"] = parseModule(\`
    import "a";
  \`);
  c.declarationInstantiation();
`);
m.declarationInstantiation();
lfLastModule = m;
m.evaluation();


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000000000c4f58b in intrinsic_AssertionFailed (cx=<optimized out>, argc=<optimized out>, vp=<optimized out>) at js/src/vm/SelfHosting.cpp:423
#0  0x0000000000c4f58b in intrinsic_AssertionFailed (cx=<optimized out>, argc=<optimized out>, vp=<optimized out>) at js/src/vm/SelfHosting.cpp:423
#1  0x00000000005b59be in js::CallJSNative (cx=0x7ffff5f17000, native=0xc4f540 <intrinsic_AssertionFailed(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/JSContext-inl.h:280
#2  0x00000000005aa8af in js::InternalCallOrConstruct (cx=<optimized out>, cx@entry=0x7ffff5f17000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:467
#3  0x00000000005aac8d in InternalCall (cx=0x7ffff5f17000, args=...) at js/src/vm/Interpreter.cpp:516
#4  0x00000000005aadda in js::CallFromStack (cx=<optimized out>, args=...) at js/src/vm/Interpreter.cpp:522
#5  0x0000000000692343 in js::jit::DoCallFallback (cx=<optimized out>, frame=0x7fffffffbb98, stub_=<optimized out>, argc=<optimized out>, vp=0x7fffffffbb28, res=...) at js/src/jit/BaselineIC.cpp:2372
#6  0x00002b1b3f9cc5ac in ?? ()
[...]
#32 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7fffffffb450	140737488335952
rcx	0x7ffff6c282ad	140737333330605
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffb490	140737488336016
rsp	0x7fffffffb430	140737488335920
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4780	140737354024832
r10	0x58	88
r11	0x7ffff6b9e7a0	140737332766624
r12	0x7ffff5d5b7c0	140737317812160
r13	0xc4f540	12907840
r14	0x7fffffffb4b0	140737488336048
r15	0x7fffffffbb40	140737488337728
rip	0xc4f58b <intrinsic_AssertionFailed(JSContext*, unsigned int, JS::Value*)+75>
=> 0xc4f58b <intrinsic_AssertionFailed(JSContext*, unsigned int, JS::Value*)+75>:	movl   $0x0,0x0
   0xc4f596 <intrinsic_AssertionFailed(JSContext*, unsigned int, JS::Value*)+86>:	ud2

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/2e4748827cda
user:        Jon Coppeard
date:        Wed Aug 09 18:05:15 2017 +0100
summary:     Bug 1374239 - Store and re-throw module instantiation and evaluation errors r=shu

This iteration took 1.253 seconds to run.

Comment 2

[Jon Coppeard (:jonco)]

I think the best thing to do is to make this throw rather than assert.

There are several places where the spec asserts the current module status, but those assertions only hold if the module resolve hook obeys its contract.  We're allowing the fuzzer to provide this function so this is testing what happens if that is not the case and the browser supplies a broken hook.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1463373-module-status

Patch to turn this assertion into a test.  The assertion can fail if the embedding's module resolve hook is wrong.

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) from comment #3)
> The assertion can fail if the embedding's module resolve hook is wrong.

...having said that, I'm not sure this is true.  The problem is exposing parseModule() to the script and letting it call declarationInstantiation() on it.  This doesn't happen outside the shell.

Anyway, we still probably want to throw here.

Comment 5

[André Bargull [:anba]]

Comment on attachment 8980228 [details] [diff] [review]
bug1463373-module-status

Review of attachment 8980228 [details] [diff] [review]:
-----------------------------------------------------------------

Agreed, let's throw here for now.

(The test case makes me wonder if dynamic import combined with toplevel await will trigger the same issue. Well, that's all future talk for now...)

Comment 6

[Jon Coppeard (:jonco)]

(In reply to André Bargull [:anba] from comment #5)
Yes I wondered the same thing, but we'll cross that bridge when we come to it.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ab83bc8a02d
Throw rather than assert if buggy module resolve hook returns a module we didn't expect r=anba

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/9ab83bc8a02d

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Is this something that we should consider backporting or can it ride the trains?

Comment 10

[Jon Coppeard (:jonco)]

This is only a problem if we the browser's module resolve hook is buggy and there's no reason to think this is the case.  This can ride the trains.