Failing es-module test on latest canary

[targos]

Test:

$ ./node --experimental-modules test/es-module/test-esm-error-cache.js
(node:15637) ExperimentalWarning: The ESM module loader is experimental.
[1]    15637 segmentation fault (core dumped)  ./node --experimental-modules

Stack trace:

Thread 1 "node" received signal SIGSEGV, Segmentation fault.
0x0000000000e4de72 in v8::Local<v8::Function>::New (isolate=0x4006370, that=...) at ../../deps/v8/include/v8.h:9726
9726      return New(isolate, that.val_);
Missing separate debuginfos, use: dnf debuginfo-install libgcc-9.1.1-1.fc30.x86_64 libstdc++-9.1.1-1.fc30.x86_64
(gdb) bt
#0  0x0000000000e4de72 in v8::Local<v8::Function>::New (isolate=0x4006370, that=...) at ../../deps/v8/include/v8.h:9726
#1  0x0000000000e4cccd in v8::PersistentBase<v8::Function>::Get (this=0x10, isolate=0x4006370) at ../../deps/v8/include/v8.h:481
#2  0x0000000000e48f33 in node::loader::ImportModuleDynamically (context=..., referrer=..., specifier=...) at ../../src/module_wrap.cc:979
#3  0x000000000131a89e in v8::internal::Isolate::RunHostImportModuleDynamicallyCallback (this=this@entry=0x4006370, referrer=..., specifier=..., specifier@entry=...) at ../../deps/v8/include/v8.h:314
#4  0x000000000186e3ce in v8::internal::__RT_impl_Runtime_DynamicImportCall (args=..., isolate=0x4006370) at ../../deps/v8/src/runtime/runtime-module.cc:27
#5  0x000000000186f827 in v8::internal::Runtime_DynamicImportCall (args_length=2, args_object=0x7ffd9560d158, isolate=0x4006370) at ../../deps/v8/src/runtime/runtime-module.cc:15
#6  0x0000000001e4305b in Builtins_CEntry_Return1_DontSaveFPRegs_ArgvInRegister_NoBuiltinExit () at ../../deps/v8/src/builtins/builtins-async-iterator-gen.cc:254
#7  0x0000000001feb71f in Builtins_CallRuntimeHandler () at ../../deps/v8/src/interpreter/interpreter-generator.cc:1729

I think it's the same issue as nodejs/node#27492

/cc @devsnek @ryzokuken

[guybedford]

We should just make these strongly held references if it is causing too many issues, and then work out the right GC approach when someone has the time to do that.

…

On Mon, 17 Jun 2019 at 10:27, Michaël Zasso ***@***.***> wrote: Test: $ ./node --experimental-modules test/es-module/test-esm-error-cache.js (node:15637) ExperimentalWarning: The ESM module loader is experimental. [1] 15637 segmentation fault (core dumped) ./node --experimental-modules Stack trace: Thread 1 "node" received signal SIGSEGV, Segmentation fault. 0x0000000000e4de72 in v8::Local<v8::Function>::New (isolate=0x4006370, that=...) at ../../deps/v8/include/v8.h:9726 9726 return New(isolate, that.val_); Missing separate debuginfos, use: dnf debuginfo-install libgcc-9.1.1-1.fc30.x86_64 libstdc++-9.1.1-1.fc30.x86_64 (gdb) bt #0 0x0000000000e4de72 in v8::Local<v8::Function>::New (isolate=0x4006370, that=...) at ../../deps/v8/include/v8.h:9726 #1 0x0000000000e4cccd in v8::PersistentBase<v8::Function>::Get (this=0x10, isolate=0x4006370) at ../../deps/v8/include/v8.h:481 #2 0x0000000000e48f33 in node::loader::ImportModuleDynamically (context=..., referrer=..., specifier=...) at ../../src/module_wrap.cc:979 #3 0x000000000131a89e in v8::internal::Isolate::RunHostImportModuleDynamicallyCallback ***@***.***=0x4006370, referrer=..., specifier=..., ***@***.***=...) at ../../deps/v8/include/v8.h:314 #4 0x000000000186e3ce in v8::internal::__RT_impl_Runtime_DynamicImportCall (args=..., isolate=0x4006370) at ../../deps/v8/src/runtime/runtime-module.cc:27 #5 0x000000000186f827 in v8::internal::Runtime_DynamicImportCall (args_length=2, args_object=0x7ffd9560d158, isolate=0x4006370) at ../../deps/v8/src/runtime/runtime-module.cc:15 #6 0x0000000001e4305b in Builtins_CEntry_Return1_DontSaveFPRegs_ArgvInRegister_NoBuiltinExit () at ../../deps/v8/src/builtins/builtins-async-iterator-gen.cc:254 #7 0x0000000001feb71f in Builtins_CallRuntimeHandler () at ../../deps/v8/src/interpreter/interpreter-generator.cc:1729 [image: image] <https://user-images.githubusercontent.com/2352663/59589371-10327500-90ea-11e9-9969-29560e748679.png> I think it's the same issue as nodejs/node#27492 <nodejs/node#27492> /cc @devsnek <https://github.com/devsnek> @ryzokuken <https://github.com/ryzokuken> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#111?email_source=notifications&email_token=AAESFSSI2LMR2BBIQSECQXTP25DFJA5CNFSM4HYUJYM2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4GZ2JQUA>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAESFSWF7KDXNZEXFMLCQLTP25DFJANCNFSM4HYUJYMQ> .

[targos]

This also affects the build with V8 7.6: https://ci.nodejs.org/job/node-test-pull-request/23936/.

@guybedford do you know what to do exactly to mitigate that?

[guybedford]

@targos it should just be a couple of lines to ensure that the map is a map of Persistent references.

[targos]

From env.h: std::unordered_map<uint32_t, v8::Global<v8::Function>> id_to_function_map;

v8::Global, like v8::Persistent inherits from v8::PersistentBase. Are you saying that changing from Global to Persistent should prevent garbage collection of the functions? It's not obvious to me from reading V8's documentation.

[guybedford]

Hmm ok then it should already be persistent. Perhaps just remove the code paths that are attempting to clear this in https://github.com/nodejs/node/blob/master/src/node_contextify.cc#L1123 then.

[guybedford]

Where the actual clearing is being done in https://github.com/nodejs/node/blob/master/src/node_contextify.cc#L295. We can just allow these to accumulate rather than segfault.

[targos]

@guybedford the only thing I would be able to do is stop making the reference weak. I'm afraid this would lead to a major memory leak for programs that use vm.compileFunction.

[guybedford]

Yes that’s the best we can do without someone to put in the dev work to ensure proper GC behaviors for these maps. We’ve had some discussions with v8 and I believe we have the ideas for how such an approach might work, so if someone is interested in working on it I’d be happy to discuss the details here. Otherwise, perhaps we can condition these maps on the experimental modules flag to isolate the leak to that for now.

…

On Tue, 18 Jun 2019 at 08:49, Michaël Zasso ***@***.***> wrote: @guybedford <https://github.com/guybedford> the only thing I would be able to do is stop making the reference weak. I'm afraid this would leak to a major memory leak for programs that use vm.compileFunction. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#111?email_source=notifications&email_token=AAESFST3CSGKT5R2I2TFBPLP3CAPNA5CNFSM4HYUJYM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX5LXZQ#issuecomment-502971366>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAESFSQVKWHCQL2TORNSBSTP3CAPNANCNFSM4HYUJYMQ> .

[guybedford]

To update here - the discussions with v8 were specifically about the GC cases for Module, which we worked out can work as long as we tie the references to the v8::Module itself since that should be live for as long as any of its closures (which could reference dynamic import).

The problem here is that Function is being disposed even when it might have closures that are live (that use dynamic import), so we're then unable to find them in the map. The fix would likely be to change the approach to use a reference other than Function that ties to the right lifetime. Eg to use just an index, then have a weak callback on the outer ScriptOrigin GC event to do the cleanup or something like that.

I'm so sorry I can't jump in and fix this, I've been following it without trying to get too involved, but just don't have the time right now to take this on unfortunately. I'm happy to discuss further with anyone about it though as I say.

[targos]

/cc @devsnek or @ryzokuken would you like to try and fix this? Or do you know who else might?

[targos]

@guybedford feel free to ping anyone you might think of to help.

[guybedford]

Thanks @targos, I'm not sure who else may be able to assist here unfortunately. Maybe other core C++ contributors? What sort of timeline are we looking at for these upgrade paths?

[targos]

/cc @addaleax ?

[targos]

This is no longer reproducible with canary but it is with V8 7.6: nodejs/node#28016