Failed cctest EnvironmentTest.MultipleEnvironmentsPerIsolate in debug build

[joyeecheung]

[ RUN      ] EnvironmentTest.MultipleEnvironmentsPerIsolate


#
# Fatal error in ../deps/v8/src/heap/heap.cc, line 4778
# Debug check failed: gc_prologue_callbacks_.end() == std::find(gc_prologue_callbacks_.begin(), gc_prologue_callbacks_.end(), GCCallbackTuple(callback, gc_type, data)).
#
#
#
#FailureMessage Object: 0x7ffeefbfdd90make[1]: *** [cctest] Illegal instruction: 4

Stack trace

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0)
  * frame #0: 0x000000010197b512 cctest`v8::base::OS::Abort() at platform-posix.cc:400 [opt]
    frame #1: 0x0000000101973b75 cctest`v8::base::(anonymous namespace)::DefaultDcheckHandler(file=<unavailable>, line=<unavailable>, message=<unavailable>) at logging.cc:56 [opt]
    frame #2: 0x00000001006f70a3 cctest`v8::internal::Heap::AddGCPrologueCallback(this=0x00000000000012aa, callback=(0x00007ffeefbfe080), gc_type=<unavailable>, data=0x000000010215fb1d)(v8::Isolate*, v8::GCType, v8::GCCallbackFlags, void*), v8::GCType, void*) at heap.cc:4776 [opt]
    frame #3: 0x00000001015833f9 cctest`node::InitDTrace(env=0x000000010481ec00) at node_dtrace.cc:275
    frame #4: 0x0000000101298edd cctest`node::RunBootstrapping(env=0x000000010481ec00) at node.cc:258
    frame #5: 0x000000010114c4e7 cctest`EnvironmentTestFixture::Env::Env(this=0x00007ffeefbfedf8, handle_scope=0x00007ffeefbfee38, argv=0x00007ffeefbfee28) at node_test_fixture.h:135
    frame #6: 0x000000010114abf5 cctest`EnvironmentTestFixture::Env::Env(this=0x00007ffeefbfedf8, handle_scope=0x00007ffeefbfee38, argv=0x00007ffeefbfee28) at node_test_fixture.h:119
    frame #7: 0x00000001011547dc cctest`EnvironmentTest_MultipleEnvironmentsPerIsolate_Test::TestBody(this=0x00000001044002a0) at test_environment.cc:61
    frame #8: 0x00000001016d9a4e cctest`void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(object=0x00000001044002a0, method=21 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00, location="the test body")(), char const*) at gtest.cc:2364
    frame #9: 0x00000001016afc42 cctest`void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(object=0x00000001044002a0, method=21 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00, location="the test body")(), char const*) at gtest.cc:2417
    frame #10: 0x00000001016afb16 cctest`testing::Test::Run(this=0x00000001044002a0) at gtest.cc:2436
    frame #11: 0x00000001016b0b8d cctest`testing::TestInfo::Run(this=0x0000000104206170) at gtest.cc:2612
    frame #12: 0x00000001016b1e9c cctest`testing::TestCase::Run(this=0x0000000104205e00) at gtest.cc:2730
    frame #13: 0x00000001016c0ddc cctest`testing::internal::UnitTestImpl::RunAllTests(this=0x0000000104203f00) at gtest.cc:4726
    frame #14: 0x00000001016dd55e cctest`bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(object=0x0000000104203f00, method=50 0a 6c 01 01 00 00 00 00 00 00 00 00 00 00 00, location="auxiliary test code (environments or event listeners)")(), char const*) at gtest.cc:2364
    frame #15: 0x00000001016c09d2 cctest`bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(object=0x0000000104203f00, method=50 0a 6c 01 01 00 00 00 00 00 00 00 00 00 00 00, location="auxiliary test code (environments or event listeners)")(), char const*) at gtest.cc:2417
    frame #16: 0x00000001016c08c9 cctest`testing::UnitTest::Run(this=0x0000000102b7bca0) at gtest.cc:4341
    frame #17: 0x00000001016e1011 cctest`RUN_ALL_TESTS() at gtest.h:2326
    frame #18: 0x00000001016e0feb cctest`main(argc=1, argv=0x00007ffeefbff688) at gtest_main.cc:36
    frame #19: 0x0000000100001034 cctest`start + 52

[joyeecheung]

The issue is when we have multiple environments accessing the same isolate, we install the same dtrace GC callbacks twice which triggers the DCHECK. I believe dtrace and ETW tracing is, similar to trace events in core, essentially global, and the GC callbacks don't seem to actually rely on the isolate, they just log the GC type and flags. So the fix should be either

1. Moving this to NewIsolate
2. Making the subsequent calla a noop

I think 1 probably makes more sense

[addaleax]

Yeah, I think either variant makes sense – add this to SetIsolateUpForNode(), or track this via a field in IsolateData.

As another solution, the easiest thing would probably be to use the variant of AddGCPrologueCallback() that takes a void* argument and store the Environment* in it? (And then also, ideally, use that to remove the hooks as required, as in #25647)? (The downside of that would be calling the handler multiple times.)

[joyeecheung]

Maybe we should just call InitDtrace in StartNodeWithIsolate? Does it make sense to enable it for embedders who do not use node::Start or for cctest any way?

[addaleax]

I don’t think it makes sense for cctest, but for embedders I would assume that the answer is “yes”. /cc @cjihrig

[joyeecheung]

add this to SetIsolateUpForNode(), or track this via a field in IsolateData.

I tried with these two, but it seems too early call the current InitDtrace during SetIsolateUpForNode, because we can't know if it's on the main thread for certain by then.

As another solution, the easiest thing would probably be to use the variant of AddGCPrologueCallback() that takes a void* argument and store the Environment* in it?

We could probably also split the GC callback part out of InitDtrace, call it in SetIsolateUpForNode, and in the callback, do whatever state management necessary with Environment::GetCurrent. But that just fixes the test, the core issue is still whether the setup, including init_etw(), is necessary at all for uses outside of our binary. Since the tracing is global, initializing them multiple times for each isolate set up by Node may lead to unnecessary traces being written to the system.

[cjihrig]

I don’t think it makes sense for cctest, but for embedders I would assume that the answer is “yes”. /cc @cjihrig

That sounds fine to me.

[joyeecheung]

That sounds fine to me.

You mean the embedders may need dtrace and ETW, or they do not?

Also cc @nodejs/embedders

[cjihrig]

Sorry, I meant I think it makes sense to enable them for embedders even if they do not use node::Start (I think that was the original question).

[joyeecheung]

On a side note, it looks like we can't use Environment::GetCurrent in the dtrace callbacks because when the emebdder data slot is not set in debug mode, it triggers the fatal error handler, which er..triggers Environment::GetCurrent again, then it starts infinite recursion (probably a separate issue for our fatal error handler, we could just pass the Environment as a void * to the callback setter instead).