Repository navigation
(Typed)ThreadSafeFunction.Release() crashes renderer process on reload (& abort?) #1109
Description
Activity
- changed the title
[-](Typed)ThreadSafeFunction.Release() segfaults renderer process on reload [/-][+](Typed)ThreadSafeFunction.Release() crashes renderer process on reload [/+]on Dec 11, 2021 - changed the title
[-](Typed)ThreadSafeFunction.Release() crashes renderer process on reload [/-][+](Typed)ThreadSafeFunction.Release() crashes renderer process on reload (& abort?)[/+]on Dec 11, 2021 @robinchrist if you remove the
Release(), and since it now works, do you get memory leaks as you reload electron many times?@gabrielschulhof I am debugging what seems to be the same issue and I did
asanit and here is what happens:- There is some class which contains a TSF
- At some point its destructor calls
TSF::Release()before deleting the object that contains the TSF - This triggers a call to
details::ThreadSafeFinalize::FinalizeWrapperWithContextat some later moment - This function fails badly because the TSF object has already been deleted
If I remove the call to
Release()- yes, then it works, but my Node process is left hanging at the end since there are open file handlesCurrently the only way to make this work is to always have a finalizer and defer the deleting of the containing object. It is quite cumbersome, isn't it possible to skip
FinalizeWrapperWithContextwhen there is no finalizer?This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.
@gabrielschulhof What would be the best way to test this?
I have the same problem on mac, but I can't trigger the bug stably.
- electron: 15.1.0
- node-addon-api: 4.2.0
- macos: 12.3 x64
I wrote something like this. See the full version here.
MDQuery::~MDQuery() { if (_queryRef) { auto localCenter = CFNotificationCenterGetLocalCenter(); CFNotificationCenterRemoveObserver(localCenter, this, kMDQueryDidFinishNotification, _queryRef); CFNotificationCenterRemoveObserver(localCenter, this, kMDQueryDidUpdateNotification, _queryRef); } this->StopQuery(); if (_queryRef) { CFRelease(_queryRef); _queryRef = NULL; } if (_queryResultCallback) { _queryResultCallback.Release(); _queryResultCallback = NULL; } if (_updateCallback) { _updateCallback.Release(); _updateCallback = NULL; } }Here is my crash log.
Crashed Thread: 0 CrBrowserMain Dispatch queue: com.apple.main-thread Exception Type: EXC_CRASH (SIGABRT) Exception Codes: 0x0000000000000000, 0x0000000000000000 Exception Note: EXC_CORPSE_NOTIFY Application Specific Information: dyld: in dlopen_preflight() abort() called Thread 0 Crashed:: CrBrowserMain Dispatch queue: com.apple.main-thread 0 libsystem_kernel.dylib 0x00007fff70ec449a __pthread_kill + 10 1 libsystem_pthread.dylib 0x00007fff70f816cb pthread_kill + 384 2 libsystem_c.dylib 0x00007fff70e4ca1c abort + 120 3 com.github.Electron.framework 0x0000000103dd68c4 uv_mutex_lock + 20 4 com.github.Electron.framework 0x0000000107439696 napi_release_threadsafe_function + 38 5 node.napi.node 0x000000011037d67e MDQuery::~MDQuery() + 174 6 node.napi.node 0x000000011037d6de MDQuery::~MDQuery() + 14 7 node.napi.node 0x000000011037f287 Napi::ObjectWrap<MDQuery>::FinalizeCallback(napi_env__*, void*, void*) + 55 8 com.github.Electron.framework 0x0000000107439a9f node_api_get_module_file_name + 767 9 com.github.Electron.framework 0x00000001074218eb node::EmitAsyncDestroy(node::Environment*, node::async_context) + 382219 10 com.github.Electron.framework 0x00000001074398d6 node_api_get_module_file_name + 310 11 com.github.Electron.framework 0x000000010743a465 node_api_get_module_file_name + 3269 12 com.github.Electron.framework 0x000000010743a4ce node_api_get_module_file_name + 3374 13 com.github.Electron.framework 0x000000010743a7db node_api_get_module_file_name + 4155 14 com.github.Electron.framework 0x0000000103dca977 uv_run + 535 15 com.github.Electron.framework 0x000000010740f2a4 node::EmitAsyncDestroy(node::Environment*, node::async_context) + 306884 16 com.github.Electron.framework 0x000000010740f745 node::EmitAsyncDestroy(node::Environment*, node::async_context) + 308069 17 com.github.Electron.framework 0x00000001073c1244 node::FreeEnvironment(node::Environment*) + 164 18 com.github.Electron.framework 0x0000000103ebca95 ElectronInitializeICUandStartNode + 918277 19 com.github.Electron.framework 0x0000000103eab3d9 ElectronInitializeICUandStartNode + 846921 20 com.github.Electron.framework 0x0000000105231e12 v8::internal::SetupIsolateDelegate::SetupHeap(v8::internal::Heap*) + 3294482 21 com.github.Electron.framework 0x000000010523341a v8::internal::SetupIsolateDelegate::SetupHeap(v8::internal::Heap*) + 3300122 22 com.github.Electron.framework 0x000000010522f298 v8::internal::SetupIsolateDelegate::SetupHeap(v8::internal::Heap*) + 3283352 23 com.github.Electron.framework 0x0000000104600ab3 electron::fuses::IsOnlyLoadAppFromAsarEnabled() + 6674531 24 com.github.Electron.framework 0x000000010460054b electron::fuses::IsOnlyLoadAppFromAsarEnabled() + 6673147 25 com.github.Electron.framework 0x00000001045fef7b electron::fuses::IsOnlyLoadAppFromAsarEnabled() + 6667563 26 com.github.Electron.framework 0x00000001045ff819 electron::fuses::IsOnlyLoadAppFromAsarEnabled() + 6669769 27 com.github.Electron.framework 0x0000000103ddc756 ElectronMain + 134 28 org.tencent.xiaowei-desktop 0x0000000103d655e6 0x103d64000 + 5606 29 libdyld.dylib 0x00007fff70d752e5 start + 1This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.
Hi @robinchrist / @mmomtchev / @BB-fat ,
We discussed this in the 1 July Node API meeting. Do you have a repository that reproduces this issue? Since this is using Electron, it is a little cumbersome to try to create a repro on our own.
Thanks, Kevin
@KevinEady I don't have a repro, but this is not limited to Electron and it is not a bug but a design oversight.
From memory (this is the reason I don't use the C++
ThreadSafeFunctioninexprtk.js- but anapi_threadsafe_function):
an object containing aThreadSafeFunctioncannot be destroyed. CallingReleasewill trigger an async callback on theThreadSafeFunctionobject. You can't even use a dynamic object and delete it in the finalizer, since there is a wrapper that runs after your finalizer.Hi @mmomtchev ,
That logic does follow. However from what I'm seeing in the code for the finalize wrappers, these just call the finalizer with the data and context as decided in the
TSFN::Newcall. The node-addon-api finalizer wrappers are static that take as its data thisdetails::ThreadSafeFinalizetype, which encapsulates the user-provided finalizer and data passed that are passed intoNew(). They do not access theThreadSafeFunctions members themselves.Are you using a finalizer callback or data that is held by the C++ object you are
Release()ing from the destructor? If so, this makes sense to me that it will crash, as this memory is no longer valid when the finalizer wrapper gets called asynchronously after release. The TSFN finalizer callback and data must be alive throughout the existence of the TSFN.Unless I am misunderstanding something...?
2 remaining items
Hi @robinchrist ,
In my threadsafe-function
AsyncIteratorexample, I callRelease()at the end of thestd::thread's entry point.Yes, but that's not really a solution. The TSFN is held for a longer time and the objects which contain the TSFN are created and destroyed on the fly.
There must be a way to properly release a TSFN that is the member of an object inside the object's destructor?
This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.
We discussed this in the 7 Oct Node API meeting.
My theory: Node destroys the TSFNs on Node's side before it frees / destroys the objects of the addon.
Both of the stacktraces posted above have a call to
node::FreeEnvironment. We will need to look into node core to verify the above assumption. We will need to look at the process of how/when entities are cleaned up during environment destruction.@robinchrist is there any chance you could run under valgrind? That might give us more info on specifically what is leading to the crash. There are some instructions in https://github.com/nodejs/node/blob/main/doc/contributing/investigating-native-memory-leaks.md
Chiming in here to say I have this issue!
Reacted by Vlad the LadI've submitted an explicit document on the order of invocation of
napi_finalizecallbacks and the cleanup hooks at nodejs/node#45903. I'll compose an example of how to release nested resources properly at https://github.com/nodejs/node-addon-examples.We discussed this issue in the 24 Feb Node API team meeting.
There is a backing PR (nodejs/node#46692) in core to verify the order as described in nodejs/node#45903. Once the test PR has been merged, we can merge the documentation PR.
I think we need to thoroughly examine all the places where we NAPI_FATAL_IF_FAILED and decide if we need to add a NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS section. For example, at
, we raise a fatal error ifLine 2940 in 5adb896
napi_define_properties()fails.It seems feasible to call
TSFN.Release()withinObjectWrap::Finalize.// jsCallback is an Napi::ThreadSafeFunction void MyObject::Finalize(const Napi::Env env) { if (jsCallback) { jsCallback.Release(); jsCallback = nil; } }
This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.
We are using a native addon with Electron (Both v14 and v16 show the same behaviour)
There is a class, which roughly looks like this:
Everything works fine - Except when Electron reloads.
There are some situation when we need to reload the Electron application via
getCurrentWebContents().reloadIgnoringCache()- Which fails spectacularly with a renderer crash.This is an issue both for our users AND when in development when using hot reload.
I have tracked down the issue to the
.Release()callsIf I remove the
.Release()calls, everything seems to work fine, including reload.I have tried to debug the issue further with a debug build of Electron, but at least v14 (have not tested v16) triggers another assertion when reloading which can't be fixed (I stripped the addon to not contain any exports and just the register call and it still caused a renderer crash on reload with Debug Electron)
My theory: Node destroys the TSFNs on Node's side before it frees / destroys the objects of the addon. I can't say whether this is just a wild theory or a real explanation, but I think a couple of Node devs here could know?
Our fix for now: Don't free the TSNFs in the destructor. The
Fooobject is long-living and usually only created once and destroyed once in the lifecycle of the addon.FWIW, that's the stacktrace:
Electron versions: v14 & v16
Compiler: Clang 10+
OS: definitely Linux, but seems to happen on MacOS and Windows
Arch: AMD64, but seems to happen on ARM64 too
Any ideas?