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

Memory leak in 1-getting-started/6_object_wrap ? #580

Closed
graebm opened this issue Jan 17, 2025 · 2 comments
Closed

Memory leak in 1-getting-started/6_object_wrap ? #580

graebm opened this issue Jan 17, 2025 · 2 comments

Comments

@graebm
Copy link
Contributor

graebm commented Jan 17, 2025

MyObject* obj = new MyObject(value);
obj->env_ = env;
status = napi_wrap(env,
jsthis,
reinterpret_cast<void*>(obj),
MyObject::Destructor,
nullptr, // finalize_hint
&obj->wrapper_);

The code above allocates memory for MyObject via new (which allocates and calls MyObject::MyObject()), but I don't see where that memory is ever freed?

Here's the finalizer...

void MyObject::Destructor(napi_env env,
void* nativeObject,
void* /*finalize_hint*/) {
reinterpret_cast<MyObject*>(nativeObject)->~MyObject();
}

I believe that finalizer code should be like:

  MyObject* obj = reinterpret_cast<MyObject*>(nativeObject);
  delete obj;

delete will call MyObject::~MyObject() AND free the memory

@mhdawson
Copy link
Member

@graebm, we discussed in the node-api team meeting today and agree the example should use delete instead of calling ~MyObject(). Would you like to submit a PR to fix that?

@graebm
Copy link
Contributor Author

graebm commented Jan 28, 2025

I posted a PR, but didn't do any local testing. I leave it in your hands...

@github-project-automation github-project-automation bot moved this from Need Triage to Done in Node-API Team Project Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

3 participants