await Promise.resolve("import('./mod.mjs')").then(eval) rejects/segfaults in REPL

[nicolo-ribaudo]

Version

v18.4.0

Platform

Linux nic-XPS-15-9570 5.15.0-40-generic #43-Ubuntu SMP Wed Jun 15 12:54:21 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

modules

What steps will reproduce the bug?

Have an empty mod.mjs file, and a main.mjs file with the following contents:

console.log(1, await import("./mod.mjs"));
console.log(2, await eval(`import("./mod.mjs")`));
console.log(3, await (0, eval)(`import("./mod.mjs")`));
console.log(4, await Promise.resolve(`import("./mod.mjs")`).then(s => eval(s)));
console.log(5, await Promise.resolve(`import("./mod.mjs")`).then(s => (0,eval)(s)));
console.log(6, await Promise.resolve(`import("./mod.mjs")`).then(eval));

If you run node main.mjs, it works.

However, if you open the REPL and copy-paste-run the first five lines one by one. Now kill the REPL, restart it, and run the sixth line: it throws

Uncaught TypeError: Invalid host defined options
    at eval (eval at processTicksAndRejections (node:internal/process/task_queues:95:5), <anonymous>:1:1)
    at eval (<anonymous>)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async REPL2:1:39

If you don't restart the REPL before running the sixth line, it almost always crashes with a segfault:

[1]    85373 segmentation fault (core dumped)  node

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

I assume that the last line should work in the REPL, because it works in a file and because all the other lines work in the REPL.
Even if the error is expected for some reason, it shouldn't segfault.

What do you see instead?

/

Additional information

No response

[benjamingr]

I can repro this on main, this is a neat find, thanks.

@nodejs/loaders @nodejs/repl

[legendecas]

When running the scripts sequentially, the Local<Script> created at https://github.com/nodejs/node/blob/main/src/node_contextify.cc#L947 can outlive the ContextifyScript so https://github.com/nodejs/node/blob/main/src/module_wrap.cc#L594 may crash because the ContextifyScript is released and the access can be invalid.

When running console.log(6, await Promise.resolve('import("./mod.mjs")').then(eval)); in a fresh REPL, the referrer script unexpectedly becomes the internal/process/task_queues.js instead of the REPL script. This can be problematic as the host_defined_options is not the expected one for the REPL script.

I'll try to dig into this.

[targos]

@legendecas I'm not sure whether it's related to this crash, but note that we are still using CompileFunctionInContext which is deprecated in V8.

[nicolo-ribaudo]

When running console.log(6, await Promise.resolve('import("./mod.mjs")').then(eval)); in a fresh REPL, the referrer script unexpectedly becomes the internal/process/task_queues.js instead of the REPL script. This can be problematic as the host_defined_options is not the expected one for the REPL script.

So, the reason I found this bug is that I was reading https://tc39.es/ecma262/#sec-hostmakejobcallback, which forces non-browser hosts to have a different behavior from browsers. HTML uses that host hook to implement support for Promise.resolve('import(`./example.mjs`)').then(eval) (https://html.spec.whatwg.org/#hostmakejobcallback), so I wanted to test how Node.js behaves.

So it's possible that the promise rejection (not the crash) is actually expected.

[bmeck]

This is actually expected in Node.js I argued at TC39 browsers should fail too in 2019 or so. However, they didn't like that. Notes term should be something about "incumbent realm". Node can allow it and ignore spec non-web expectations. We had to do same with JSON impoer assertions due to how web structured that semantics

…

On Tue, Jul 5, 2022, 3:59 PM Nicolò Ribaudo ***@***.***> wrote: When running console.log(6, await Promise.resolve('import("./mod.mjs")').then(eval)); in a fresh REPL, the referrer script unexpectedly becomes the internal/process/task_queues.js instead of the REPL script. This can be problematic as the host_defined_options is not the expected one for the REPL script. So, the reason I found this bug is that I was reading https://tc39.es/ecma262/#sec-hostmakejobcallback, which forces non-browser hosts to have a different behavior from browsers. HTML uses that host hook to implement support for Promise.resolve('import( ./example.mjs)').then(eval), so I wanted to test how Node.js behaves. So it's possible that the promise rejection (not the crash) is actually expected. — Reply to this email directly, view it on GitHub <#43681 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZJIZ3ZJXURUZJ7YW6Z2DVSSO2TANCNFSM52T5DMMQ> . You are receiving this because you are on a team that was mentioned.Message ID: ***@***.***>

[legendecas]

The crash was fixed by #48510. The uncovered issue behind the crash is being tracked at #49726.