Segfault on shutdown with ObjectWrap

[Matt-Esch]

I am seeing a segfault with the [email protected] when the node processes tears down, and it appears related to the cleanup of a an ObjectWrap instance that is explicitly referenced, so either there is still an outstanding bug in the finalizer or I've misunderstood the requirements for the destructor implementation, or the purpose of SuppressDestruct .

My understanding is that the environment is in a torn-down state in the destructor, so if we want to modify references when an object goes out of scope and is garbage collected, we must call SuppressDestruct in the constructor. My interpretation of SuppressDestruct is to avoid running the destructor at shutdown because the destructor would access an invalid env if we did not. What I don't understand is why the destructor is still called and possibly multiple times on process shutdown. For example, on an object I have explicitly called SuppressDestruct, valgrind provides the following:

==11124== Thread 1:
==11124== Invalid read of size 8
==11124==    at 0x1203F63C: Napi::ObjectWrap<Consumer>::FinalizeCallback(napi_env__*, void*, void*) (napi-inl.h:4550)
==11124==    by 0xD6F28A: napi_env__::CallFinalizer(void (*)(napi_env__*, void*, void*), void*, void*)::{lambda(napi_env__*)#1}::operator()(napi_env__*) const (js_native_api_v8.h:107)
==11124==    by 0xD6FC2B: void napi_env__::CallIntoModule<napi_env__::CallFinalizer(void (*)(napi_env__*, void*, void*), void*, void*)::{lambda(napi_env__*)#1}, void (napi_env__*, v8::Local<napi_env__::CallFinalizer(void (*)(napi_env__*, void*, void*), void*, void*)::{lambda(napi_env__*)#1}::Value>)>(napi_env__::CallFinalizer(void (*)(napi_env__*, void*, void*), void*, void*)::{lambda(napi_env__*)#1}&&, void (&&)(napi_env__*, v8::Local<napi_env__::CallFinalizer(void (*)(napi_env__*, void*, void*), void*, void*)::{lambda(napi_env__*)#1}::Value>)) (js_native_api_v8.h:95)
==11124==    by 0xD6F2FD: napi_env__::CallFinalizer(void (*)(napi_env__*, void*, void*), void*, void*) (js_native_api_v8.h:106)
==11124==    by 0xD3D9CD: v8impl::(anonymous namespace)::RefBase::Finalize(bool) (js_native_api_v8.cc:273)
==11124==    by 0xD6F04D: v8impl::RefTracker::FinalizeAll(v8impl::RefTracker*) (js_native_api_v8.h:43)
==11124==    by 0xD6F17B: napi_env__::~napi_env__() (js_native_api_v8.h:66)
==11124==    by 0xD72B6B: node_napi_env__::~node_napi_env__() (node_api.cc:14)
==11124==    by 0xD72B87: node_napi_env__::~node_napi_env__() (node_api.cc:14)
==11124==    by 0xD4CE34: napi_env__::Unref() (js_native_api_v8.h:77)
==11124==    by 0xD69001: v8impl::(anonymous namespace)::NewEnv(v8::Local<v8::Context>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)::{lambda(void*)#1}::operator()(void*) const (node_api.cc:106)
==11124==    by 0xD69021: v8impl::(anonymous namespace)::NewEnv(v8::Local<v8::Context>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)::{lambda(void*)#1}::_FUN(void*) (node_api.cc:107)
==11124==  Address 0xb6c86d0 is 0 bytes inside a block of size 56 free'd
==11124==    at 0x4D4923B: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==11124==    by 0x1203A04B: Consumer::~Consumer() (Consumer.cc:465)
==11124==    by 0x1203F682: Napi::ObjectWrap<Consumer>::FinalizeCallback(napi_env__*, void*, void*) (napi-inl.h:4551)
==11124==    by 0xD6F852: node_napi_env__::CallFinalizer(void (*)(napi_env__*, void*, void*), void*, void*)::{lambda(node::Environment*)#1}::operator()(node::Environment*) const::{lambda(napi_env__*)#1}::operator()(napi_env__*) const (node_api.cc:43)
==11124==    by 0xD6FD95: void napi_env__::CallIntoModule<node_napi_env__::CallFinalizer(void (*)(napi_env__*, void*, void*), void*, void*)::{lambda(node::Environment*)#1}::operator()(node::Environment*) const::{lambda(napi_env__*)#1}, void (napi_env__*, v8::Local<{lambda(napi_env__*)#1}::Value>)>(node_napi_env__::CallFinalizer(void (*)(napi_env__*, void*, void*), void*, void*)::{lambda(node::Environment*)#1}::operator()(node::Environment*) const::{lambda(napi_env__*)#1}&&, void (&&)(napi_env__*, v8::Local<{lambda(napi_env__*)#1}::Value>)) (js_native_api_v8.h:95)
==11124==    by 0xD6F7EA: node_napi_env__::CallFinalizer(void (*)(napi_env__*, void*, void*), void*, void*)::{lambda(node::Environment*)#1}::operator()(node::Environment*) const (node_api.cc:42)
==11124==    by 0xD72C08: node::CallbackQueue<void, node::Environment*>::CallbackImpl<node_napi_env__::CallFinalizer(void (*)(napi_env__*, void*, void*), void*, void*)::{lambda(node::Environment*)#1}>::Call(node::Environment*) (callback_queue-inl.h:90)
==11124==    by 0xD12757: node::Environment::RunAndClearNativeImmediates(bool)::{lambda(node::CallbackQueue<void, node::Environment*>*)#1}::operator()(node::CallbackQueue<void, node::Environment*>*) const (env.cc:739)
==11124==    by 0xD12969: node::Environment::RunAndClearNativeImmediates(bool) (env.cc:752)
==11124==    by 0xD131C7: node::Environment::CheckImmediate(uv_check_s*) (env.cc:892)
==11124==    by 0x1BEE0F5: uv__run_check (loop-watcher.c:67)
==11124==    by 0x1BE3AFC: uv_run (core.c:394)
==11124==  Block was alloc'd at
==11124==    at 0x4D4817F: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==11124==    by 0x1204008C: Napi::ObjectWrap<Consumer>::ConstructorCallbackWrapper(napi_env__*, napi_callback_info__*)::{lambda()#1}::operator()() const (napi-inl.h:4473)
==11124==    by 0x12040E50: napi_value__* Napi::details::WrapCallback<Napi::ObjectWrap<Consumer>::ConstructorCallbackWrapper(napi_env__*, napi_callback_info__*)::{lambda()#1}>(Napi::ObjectWrap<Consumer>::ConstructorCallbackWrapper(napi_env__*, napi_callback_info__*)::{lambda()#1}) (napi-inl.h:83)
==11124==    by 0x1204028B: Napi::ObjectWrap<Consumer>::ConstructorCallbackWrapper(napi_env__*, napi_callback_info__*) (napi-inl.h:4471)
==11124==    by 0xD4BB81: v8impl::(anonymous namespace)::CallbackWrapperBase<v8::FunctionCallbackInfo<v8::Value>, &v8impl::(anonymous namespace)::CallbackBundle::function_or_getter>::InvokeCallback()::{lambda(napi_env__*)#1}::operator()(napi_env__*) const (js_native_api_v8.cc:510)
==11124==    by 0xD4C603: void napi_env__::CallIntoModule<v8impl::(anonymous namespace)::CallbackWrapperBase<v8::FunctionCallbackInfo<v8::Value>, &v8impl::(anonymous namespace)::CallbackBundle::function_or_getter>::InvokeCallback()::{lambda(napi_env__*)#1}, void (napi_env__*, v8::Local<v8::Value>)>(v8impl::(anonymous namespace)::CallbackWrapperBase<v8::FunctionCallbackInfo<v8::Value>, &v8impl::(anonymous namespace)::CallbackBundle::function_or_getter>::InvokeCallback()::{lambda(napi_env__*)#1}&&, void (&&)(napi_env__*, v8::Local<v8::Value>)) (js_native_api_v8.h:95)
==11124==    by 0xD4BC08: v8impl::(anonymous namespace)::CallbackWrapperBase<v8::FunctionCallbackInfo<v8::Value>, &v8impl::(anonymous namespace)::CallbackBundle::function_or_getter>::InvokeCallback() (js_native_api_v8.cc:509)
==11124==    by 0xD3E171: v8impl::(anonymous namespace)::FunctionCallbackWrapper::Invoke(v8::FunctionCallbackInfo<v8::Value> const&) (js_native_api_v8.cc:528)
==11124==    by 0x10EEBBD: v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) (api-arguments-inl.h:158)
==11124==    by 0x10EF851: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<true>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) (builtins-api.cc:111)
==11124==    by 0x10F5038: v8::internal::Builtins::InvokeApiFunction(v8::internal::Isolate*, bool, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*, v8::internal::Handle<v8::internal::HeapObject>) (builtins-api.cc:224)
==11124==    by 0x12A912F: v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) (execution.cc:191)

[Matt-Esch]

@mhdawson you mentioned in a previous issue that calling Ref on an ObjectWrap is not safe, I didn't quite understand your suggestion for an alternative. How would I prevent an object from being garbage collected otherwise?

[mhdawson]

@Matt-Esch can you point me to the previous issue you mentioned so that I can swap in the context?

[Matt-Esch]

@mhdawson https://github.com/nodejs/node-addon-api/pull/869

[mhdawson]

@Matt-Esch sorry for the late reply. My suggestion in https://github.com/nodejs/node-addon-api/pull/869 was not to use Ref/Unref on the ObjectWrap object itself. If you want to keep an object alive (including the JavaScript object for an ObjectWrap) create a new ObjectReference for the Object and call Ref/Unref on that.

[github-actions[bot]]

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.