Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ObjectWrap leak introduced in 8.2.0 #1602

Closed
arsnyder16 opened this issue Oct 25, 2024 · 4 comments · Fixed by #1607
Closed

ObjectWrap leak introduced in 8.2.0 #1602

arsnyder16 opened this issue Oct 25, 2024 · 4 comments · Fixed by #1607

Comments

@arsnyder16
Copy link

When using ObjectWrap that does not override a finalizer a memory leak is occurring.

I traced this back to #1514

Which introduced the following code which seems to trigger the leak. Removing this set eliminated the leak, but its not clear to me how this affects a basic or extended finalizer.

// Prevent ~ObjectWrap from calling napi_remove_wrap
instance->_ref = nullptr;
@arsnyder16
Copy link
Author

Repro steps

CFLAGS=-fsanitize=address CXXFLAGS=-fsanitize=address npm run pretest
LD_PRELOAD=`clang -print-file-name=libclang_rt.asan-x86_64.so` NODE_API_BUILD_CONFIG=Release node --expose-gc test/objectwrap.js

you will see the very first leak reported comes from a ObjectWrap<> construction

Direct leak of 72 byte(s) in 1 object(s) allocated from:
    #0 0x7fdf194265fd in operator new(unsigned long) (/usr/lib/llvm-19/lib/clang/19/lib/linux/libclang_rt.asan-x86_64.so+0x10e5fd) (BuildId: 897c70dd2d121c287b9879aab2db7e64324572a9)
    #1 0x000000c5dd9e in v8impl::ReferenceWithFinalizer::New(napi_env__*, v8::Local<v8::Value>, unsigned int, v8impl::ReferenceOwnership, void (*)(napi_env__*, void*, void*), void*, void*) (/root/.nvm/versions/node/v20.18.0/bin/node+0xc5dd9e) (BuildId: fbe09eeb702823fc4c9cc18fd5d44c32225e2ab6)
    #2 0x000000c6632f in napi_wrap (/root/.nvm/versions/node/v20.18.0/bin/node+0xc6632f) (BuildId: fbe09eeb702823fc4c9cc18fd5d44c32225e2ab6)
    #3 0x7fdeeaaa946c in Napi::cstm::ObjectWrap<Test>::ObjectWrap(Napi::cstm::CallbackInfo const&) (/root/node-addon-api/test/build/Release/binding_custom_namespace.node+0x15646c) (BuildId: 68f9f6c62e70d2753bc17857e6be2f35924283c2)
    #4 0x7fdeeaaa84bb in Test::Test(Napi::cstm::CallbackInfo const&) (/root/node-addon-api/test/build/Release/binding_custom_namespace.node+0x1554bb) (BuildId: 68f9f6c62e70d2753bc17857e6be2f35924283c2)
    #5 0x7fdeeaaa800f in Napi::cstm::ObjectWrap<Test>::ConstructorCallbackWrapper(napi_env__*, napi_callback_info__*)::'lambda0'()::operator()() const (/root/node-addon-api/test/build/Release/binding_custom_namespace.node+0x15500f) (BuildId: 68f9f6c62e70d2753bc17857e6be2f35924283c2)
    #6 0x7fdeeaaa7511 in Napi::cstm::ObjectWrap<Test>::ConstructorCallbackWrapper(napi_env__*, napi_callback_info__*) (/root/node-addon-api/test/build/Release/binding_custom_namespace.node+0x154511) (BuildId: 68f9f6c62e70d2753bc17857e6be2f35924283c2)
    #7 0x000000c5c524 in v8impl::(anonymous namespace)::FunctionCallbackWrapper::Invoke(v8::FunctionCallbackInfo<v8::Value> const&) js_native_api_v8.cc
    #8 0x000000f6e88e in v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) (/root/.nvm/versions/node/v20.18.0/bin/node+0xf6e88e) (BuildId: fbe09eeb702823fc4c9cc18fd5d44c32225e2ab6)
    #9 0x000000f6ee44 in 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::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, unsigned long*, int) builtins-api.cc
    #10 0x000000f6f562 in v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) (/root/.nvm/versions/node/v20.18.0/bin/node+0xf6f562) (BuildId: fbe09eeb702823fc4c9cc18fd5d44c32225e2ab6)
    #11 0x000001979df5 in Builtins_CEntry_Return1_ArgvOnStack_BuiltinExit /home/iojs/build/ws/out/Release/obj.target/v8_snapshot/geni/embedded.o
    #12 0x0000018e958e in Builtins_JSBuiltinsConstructStub /home/iojs/build/ws/out/Release/obj.target/v8_snapshot/geni/embedded.o
    #13 0x000001a35c01 in Builtins_ConstructHandler /home/iojs/build/ws/out/Release/obj.target/v8_snapshot/geni/embedded.o
    #14 0x0000018ebd1b in Builtins_InterpreterEntryTrampoline /home/iojs/build/ws/out/Release/obj.target/v8_snapshot/geni/embedded.o
    #15 0x0000018ebd1b in Builtins_InterpreterEntryTrampoline /home/iojs/build/ws/out/Release/obj.target/v8_snapshot/geni/embedded.o
    #16 0x000001923d42 in Builtins_AsyncFunctionAwaitResolveClosure /home/iojs/build/ws/out/Release/obj.target/v8_snapshot/geni/embedded.o
    #17 0x0000019dbbf0 in Builtins_PromiseFulfillReactionJob /home/iojs/build/ws/out/Release/obj.target/v8_snapshot/geni/embedded.o
    #18 0x000001913a33 in Builtins_RunMicrotasks /home/iojs/build/ws/out/Release/obj.target/v8_snapshot/geni/embedded.o
    #19 0x0000018ea002 in Builtins_JSRunMicrotasksEntry /home/iojs/build/ws/out/Release/obj.target/v8_snapshot/geni/embedded.o
    #20 0x000001066f9c in v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) execution.cc
    #21 0x0000010681ce in v8::internal::Execution::TryRunMicrotasks(v8::internal::Isolate*, v8::internal::MicrotaskQueue*) (/root/.nvm/versions/node/v20.18.0/bin/node+0x10681ce) (BuildId: fbe09eeb702823fc4c9cc18fd5d44c32225e2ab6)
    #22 0x00000109bb45 in v8::internal::MicrotaskQueue::RunMicrotasks(v8::internal::Isolate*) (/root/.nvm/versions/node/v20.18.0/bin/node+0x109bb45) (BuildId: fbe09eeb702823fc4c9cc18fd5d44c32225e2ab6)
    #23 0x00000109bedc in v8::internal::MicrotaskQueue::PerformCheckpoint(v8::Isolate*) (/root/.nvm/versions/node/v20.18.0/bin/node+0x109bedc) (BuildId: fbe09eeb702823fc4c9cc18fd5d44c32225e2ab6)
    #24 0x000000bd2c1a in node::InternalCallbackScope::Close() (/root/.nvm/versions/node/v20.18.0/bin/node+0xbd2c1a) (BuildId: fbe09eeb702823fc4c9cc18fd5d44c32225e2ab6)
    #25 0x000000bd312a in node::InternalMakeCallback(node::Environment*, v8::Local<v8::Object>, v8::Local<v8::Object>, v8::Local<v8::Function>, int, v8::Local<v8::Value>*, node::async_context) (/root/.nvm/versions/node/v20.18.0/bin/node+0xbd312a) (BuildId: fbe09eeb702823fc4c9cc18fd5d44c32225e2ab6)
    #26 0x000000bd325d in node::MakeCallback(v8::Isolate*, v8::Local<v8::Object>, v8::Local<v8::Function>, int, v8::Local<v8::Value>*, node::async_context) (/root/.nvm/versions/node/v20.18.0/bin/node+0xbd325d) (BuildId: fbe09eeb702823fc4c9cc18fd5d44c32225e2ab6)
    #27 0x000000c363a1 in node::Environment::CheckImmediate(uv_check_s*) (/root/.nvm/versions/node/v20.18.0/bin/node+0xc363a1) (BuildId: fbe09eeb702823fc4c9cc18fd5d44c32225e2ab6)
    #28 0x0000018d0028 in uv__run_check /home/iojs/build/ws/out/../deps/uv/src/unix/loop-watcher.c:67:1
    #29 0x0000018c864b in uv_run /home/iojs/build/ws/out/../deps/uv/src/unix/core.c:461:5

If you run this same sequence in on the tag node-addon-api-v8.1.0 the leak won't exist.

@mhdawson
Copy link
Member

@legendecas I see you have a PR which is related. A couple of questions:

  1. Does that fix this issue, or will additional changes be needed in node-addon-api, I'm assuming the later
  2. Is there a short term work around that people can use in their code, or possibly a change we could add to node-addon-api that would fix it transparently as the change in node itself works its way through to the LTS versions?

@legendecas
Copy link
Member

@mhdawson #1607 should be sufficient to fix the issue, without any changes in Node.js core.

nodejs/node#55620 allows a Napi::Reference to reclmain napi_reference memory earlier, without a post_finalizer call.

@mhdawson
Copy link
Member

mhdawson commented Nov 1, 2024

@legendecas thanks for the quick answer and fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants