nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
106.56k stars 29.05k forks source link

Crash during ThreadSafeFunction finalization #49171

Open stefandtu opened 1 year ago

stefandtu commented 1 year ago

Version

v.18.15.0

Platform

all (Win, MacOs)

Subsystem

node api

What steps will reproduce the bug?

electron_test_tsfn_node_integrated_mode.zip electrone_25_crash_tsfn

Close app window or press "Release" button

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

electron_node.log

[pid 628060, tid: 707296 ] --------------FreeEnvironment start : 000008820120C000 uv_loop: 00007FF740548E28 [pid 628060, tid: 707296 ] --------------FreeEnvironment set_stopping(true): 000008820120C000 [pid 628060, tid: 707296 ] --------------FreeEnvironment before RunCleanup : 000008820120C000 // Here, the released function is being finalized from a foreign context [pid 628060, tid: 707296 ] ThreadSafeFunction Finalize() f_name: test_tsfn Environment* :0000088200A9C000 Isolate: 0000088200834000 this=0000088200A7D400 [pid 628060, tid: 707296 ] env (param): 0000088200A9C000 env isolate ( env->isolate() ) : 0000088200834000 isolate to env ( Environment::GetCurrent(env->isolate()) ) : 000008820120C000 !!! Critical error Environment::GetCurrent(env->isolate()) not eq to env 0000088200A9C000 != 000008820120C000 .....

What is the expected behavior? Why is that the expected behavior?

No response

What do you see instead?

Absence of process abort

Additional information

My quick bug fix: /main/src/node_api.cc

void Finalize() {

v8::HandleScope scope(env->isolate); 
// <==  fix
v8::Context::Scope context_scope(env->context());    
// => fix
if (finalize_cb) {
  CallbackScope cb_scope(this);
  env->CallFinalizer<false>(finalize_cb, finalize_data, context);
}
EmptyQueueAndDelete();

}

stefandtu commented 1 year ago

Hello. It looks very much like this error is a result of Electron using multiple Envs in a single thread.

https://github.com/electron/electron/issues/36858 https://github.com/electron/electron/pull/38754

legendecas commented 11 months ago

@stefandtu hi, do you think this issue is resolved or is there anything we can do on Node.js side?

stefandt commented 11 months ago

Hi! I'm unsure if the problem has been resolved. At the moment, I've applied a workaround on the node.js side, given my limited control over the other code (electron and js). I'm not sure whether node.js should allow the creation of multiple environments within a single thread, or if this is an issue with the electron approach.

I'm trying to switch the context to the correct one (via the call to create_valid_context_scope_if_required) when I notice the current context breaking and executing tasks from the queue that belong to other contexts. node_api.zip