Segfaults with dynamic imports and mocha

[demurgos]

• Version: 12.1.0
• Platform: Linux 5.0.9 64-bit
• Subsystem: modules

I am testing the new ESM implementation on one of my projects. I am able to consistently get segfaults when using experimental modules and dynamic imports with mocha unit tests.

I extracted a reproducible example to demurgos/node-esm-sigsegv. I am working on reducing the example to be minimal.

The README.md has more details, here is a summary:

You can clone the repo, install the dependencies and run the following command:

node --experimental-modules --es-module-specifier-resolution=node node_modules/mocha/bin/_mocha build/test/test.esm.js --delay --async --no-config --no-package --no-opts --diff --extension js --reporter spec --slow 75 --timeout 2000 --ui bdd

The segfault occurs while evaluating the function in test.esm.js.
This function sequentially dynamically imports ESM spec files.

When importing a single file, the execution succeeds.
When importing 2 or 3 files, the execution segfaults 25% of the time.
When importing more files, the execution always segfaults.

I don't know the exact cause of the segfault yet. I am working on isolating it to a minimal example.
It is worth noting that Mocha injects global variables (which may interact badly with ES modules). It may also be a mocha bug, at the moment I am suspecting an ESM issue because it is a C++ crash (and not a JS exception).

[devsnek]

I will take a look at this soon.

@targos or @myles could you move this issue to the main repo?

[demurgos]

I removed most of the external dependencies: only mocha and chai remain. I also numbered the dynamic imports: the segfault occurs after 10 or 11 imports. I am still not sure if it is an ESM or mocha issue, but there's definitely a segfault.

[targos]

Debug backtrace (on master):

#0  0x0000000000d9d922 in v8::Local<v8::Function>::New (isolate=0x47887f0, that=...) at ../deps/v8/include/v8.h:9691
#1  0x0000000000d9c94f in v8::PersistentBase<v8::Function>::Get (this=0x10, isolate=0x47887f0) at ../deps/v8/include/v8.h:482
#2  0x0000000000d98cd7 in node::loader::ImportModuleDynamically (context=..., referrer=..., specifier=...) at ../src/module_wrap.cc:978
#3  0x0000000001696804 in v8::internal::Isolate::RunHostImportModuleDynamicallyCallback (this=this@entry=0x47887f0, referrer=referrer@entry=..., 
    specifier=..., specifier@entry=...) at ../deps/v8/include/v8.h:315
#4  0x00000000019678ac in v8::internal::__RT_impl_Runtime_DynamicImportCall (args=..., isolate=0x47887f0) at ../deps/v8/src/runtime/runtime-module.cc:28
#5  0x0000000001968df7 in v8::internal::Runtime_DynamicImportCall (args_length=2, args_object=0x7ffd6cc9a3d0, isolate=0x47887f0)
    at ../deps/v8/src/runtime/runtime-module.cc:15
#6  0x00000000024f2ee4 in Builtins_CEntry_Return1_DontSaveFPRegs_ArgvInRegister_NoBuiltinExit ()
    at ../deps/v8/src/builtins/builtins-async-iterator-gen.cc:339
#7  0x00000000026662f2 in Builtins_CallRuntimeHandler () at ../deps/v8/src/interpreter/interpreter-generator.cc:1733

[targos]

With the following diff, we get: https://gist.github.com/targos/7d9d1dc657a322519de758d8b29e9e21

diff --git a/src/module_wrap.cc b/src/module_wrap.cc
index a4e81dcc29..4ffb2534df 100644
--- a/src/module_wrap.cc
+++ b/src/module_wrap.cc
@@ -975,6 +975,7 @@ static MaybeLocal<Promise> ImportModuleDynamically(
     ModuleWrap* wrap = ModuleWrap::GetFromID(env, id);
     object = wrap->object();
   } else if (type == ScriptType::kFunction) {
+    printf("find: %i\n", id);
     object = env->id_to_function_map.find(id)->second.Get(iso);
   } else {
     UNREACHABLE();
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index f8d43e062e..fdd3d41381 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -289,7 +289,9 @@ void ContextifyContext::WeakCallback(
 void ContextifyContext::WeakCallbackCompileFn(
     const WeakCallbackInfo<CompileFnEntry>& data) {
   CompileFnEntry* entry = data.GetParameter();
+    printf("weakcallback %i\n", entry->id);
   if (entry->env->compile_fn_entries.erase(entry) != 0) {
+    printf("erase %i\n", entry->id);
     entry->env->id_to_function_map.erase(entry->id);
     delete entry;
   }
@@ -1110,6 +1112,7 @@ void ContextifyContext::CompileFunction(
     return;
   }
   Local<Function> fn = maybe_fn.ToLocalChecked();
+  printf("emplace %i\n", id);
   env->id_to_function_map.emplace(std::piecewise_construct,
                                   std::make_tuple(id),
                                   std::make_tuple(isolate, fn));

[targos]

So, if I understand correctly, the issue is that the main script's (test.esm.js) CommonJS wrapper function is garbage collected.

It can be reproduced immediately after the first import with:

diff --git a/build/test/test.esm.js b/build/test/test.esm.js
index ee16ed3..ce3c438 100644
--- a/build/test/test.esm.js
+++ b/build/test/test.esm.js
@@ -1,6 +1,7 @@
 (async () => {
   console.log("Before imports");
   await import("./test/builtins/sint8.spec.mjs");
+  gc();
   console.log("Imports: 1");
   await import("./test/builtins/uint8.spec.mjs");
   console.log("Imports: 2");
diff --git a/run.sh b/run.sh
index aff31ff..8fbaf07 100755
--- a/run.sh
+++ b/run.sh
@@ -1,4 +1,4 @@
 #!/usr/bin/env bash
 set -ex
 
-node --experimental-modules --es-module-specifier-resolution=node node_modules/mocha/bin/_mocha build/test/test.esm.js --delay --async --no-config --no-package --no-opts --diff --extension js --reporter spec --slow 75 --timeout 2000 --ui bdd
+node --expose-gc --experimental-modules --es-module-specifier-resolution=node node_modules/mocha/bin/_mocha build/test/test.esm.js --delay --async --no-config --no-package --no-opts --diff --extension js --reporter spec --slow 75 --timeout 2000 --ui bdd

[devsnek]

thanks for looking into this targos.

I've been planning a rewrite of module internals to fix a bunch of GC issues, I guess this is just the latest on the pile.

[kronthto]

I too experienced reproduceable segfaults with ESM imports after upgrading lately.

I'm not familiar with the NodeJS-source but I managed to narrow it down to something between the versions v10.15.2 (works) and v10.15.3 (segfault).
Maybe it helps you guys track the issue down, cheers!

[mattgodbolt]

This only puts off the inevitable, but I found using this snippet in my mocha loader:

(async () => {
    const matches = globSync(join(__dirname, "**/*-test.mjs"));
    await Promise.all(matches.map(x => import(pathToFileURL(resolve(x)).href)));
    run();
})();

lets me get a tiny bit further :) Here's hoping this bug can be fixed as it affects both v10 LTS and v12!

[devsnek]

I was trying to repro this, but it just freezes on the second import line now. cc @targos

'use strict';

(async () => {
  await import('./test1.mjs');
  console.log('1');
  gc();
  console.log('gc');
  await import('./test2.mjs');
  console.log('2');
})();

[addaleax]

@devsnek Could you rebase against the latest master? #27775 fixed some issues with segfault handling that were present in master.

[targos]

GC behavior probably changed with V8 7.5

[devsnek]

ok i can reproduce it after #27775 (although there seems to bug in 27775)