DCHECK crash on `!i_isolate->is_execution_terminating()`

[codebytere]

https://chromium-review.googlesource.com/c/v8/v8/+/3620285

was merged recently and causes a ton of failures in a variety of tests (~50 or so, parallel/test-worker-esm-exit as an example):

#
# Fatal error in ../../v8/src/api/api.cc, line 8345
# Debug check failed: !i_isolate->is_execution_terminating().
#
#
#
#FailureMessage Object: 0x700007e04e90
 1: 0x12e68e672 node::NodePlatform::GetStackTracePrinter()::$_3::__invoke() [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 2: 0x12ae43306 V8_Fatal(char const*, int, char const*, ...) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 3: 0x12ae42da5 v8::base::(anonymous namespace)::DefaultDcheckHandler(char const*, int, char const*) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 4: 0x1263561c5 v8::Integer::NewFromUnsigned(v8::Isolate*, unsigned int) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 5: 0x12e568a24 node::InternalCallbackScope::Close()::$_1::operator()() const [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 6: 0x12e568843 node::InternalCallbackScope::Close() [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 7: 0x12e56826e node::InternalCallbackScope::~InternalCallbackScope() [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 8: 0x12e6183f4 node::fs::FileHandle::CloseReq::Resolve() [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 9: 0x12e62d7d9 node::fs::FileHandle::ClosePromise()::$_0::__invoke(uv_fs_s*) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
10: 0x1248ab378 uv__work_done [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
11: 0x1248af2eb uv__async_io [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
12: 0x1248c21b8 uv__io_poll [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
13: 0x1248af705 uv_run [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
14: 0x12e65b25f node::SpinEventLoop(node::Environment*) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
15: 0x12e6cff7f node::worker::Worker::Run() [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
16: 0x12e6d3543 node::worker::Worker::StartThread(v8::FunctionCallbackInfo<v8::Value> const&)::$_3::__invoke(void*) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
17: 0x7ff80b5ad4e1 _pthread_start [/usr/lib/system/libsystem_pthread.dylib]
18: 0x7ff80b5a8f6b thread_start [/usr/lib/system/libsystem_pthread.dylib]
Command: /Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/MacOS/Electron /Users/codebytere/Developer/electron-gn/src/third_party/electron_node/test/parallel/test-worker-esm-exit.js
--- CRASHED (Signal: 5) ---
[00:00|% 100|+   0|-   1]: Done

I initially attempted to fix by adding in early returns in various places:

diff --git a/src/api/callback.cc b/src/api/callback.cc
index 1287eb466f..7ca10e35e7 100644
--- a/src/api/callback.cc
+++ b/src/api/callback.cc
@@ -102,6 +102,7 @@ void InternalCallbackScope::Close() {
 
   if (!env_->can_call_into_js()) return;
   auto perform_stopping_check = [&]() {
+    if (isolate->IsExecutionTerminating()) return;
     if (env_->is_stopping()) {
       MarkAsFailed();
       env_->async_hooks()->clear_async_id_stack();

but it ended up being a bit of a whac-a-mole game with new ones appearing with every early return I added so i'm not sure what the best approach is.

cc @targos and @camillobruni since you opened the CL :)

[targos]

Thanks for opening this. It also affect V8 10.3: nodejs/node#43195 (comment)

[camillobruni]

Sorry for the inconvenience here :/.

I think V8 has been a bit too lenient on this in the past (and it led to a bunch of issues within chrome).
The general idea is really that you don't use V8 anymore after marking it for termination, and I'm aware of how painful it can be to track down those issues.

I think having simple accessors still available would make sense (and it might well be that I totally missed a use case present in node).

[targos]

Example in CI: https://ci.nodejs.org/job/node-test-commit-arm-debug/2497/nodes=ubuntu1804-arm64/

Problematic code: https://github.com/nodejs/node/blob/6ac55fa337b57cecf36602d0acc3da25ee82a589/src/env-inl.h#L222-L237

/cc @nodejs/async_hooks

[Flarna]

Adding if (env()->can_call_into_js()) { around this code solves the async hooks problem but it seems there are other similar places where JS code is called but no longer allowed.

[mcollina]

@addaleax @jasnell @Qard, maybe you might want to chime in on this?

[addaleax]

Adding if (env()->can_call_into_js()) { around this code solves the async hooks problem

I’m not sure what else to do besides doing this and being consistent about this.

[Qard]

Problem seems to be the perform_stopping_check() lambda in callback.cc Close method. It tries to clean up async_hooks reactively to the env stopping, but at that point it's no longer safe to create the HandleScope for setting the length field on js_execution_async_resources_. If the env is already in shutdown, it should be safe to just skip that though. 🤔

I suspect we're going to need to just play that game of whack-a-mole to get any of those post-shutdown stuff filtered out.

[codebytere]

Alright - i can spin up a PR for that soon then!

[tony-go]

Hey 👋

I tried to reproduce the bug, but I struggled a bit as I'm fairly new to the codebase.

I have two questions:

• Should we checkout a specific branch to reproduce it? (like targos:v8-103)
• How could we run a single test file like parallel/test-worker-esm-exit? The documentation points out make jstest but it executes all the tests. Maybe I miss it somewhere?

[codebytere]

@tony-go tests can be run as a rule with just the node executable, so out/Debug/node /path/to/test/file. If you do want to use the test runner, you can also just do python3 tools/test.py test/parallel/test-worker-esm-exit.js (i personally prefer using the runner since it allows for much more customization)

[tony-go]

Thanks, @codebytere 🙌

Finally found this: https://github.com/nodejs/node/blob/main/BUILDING.md#running-tests

[codebytere]

@Flarna hmm - that's strange since theres a can_call_into_js() immediately prior: https://github.com/nodejs/node/blob/6932fb8bac3a4caf3f3fabee0a595a077f477f63/src/api/callback.cc#L103