node-api: Memory leak fix for RefBase::Delete(RefBase* reference)

[nagendra5]

Memory leak fix: Added missing_finalize_ran.

[nodejs-github-bot]

Review requested:

• @nodejs/node-api

[legendecas]

Would you mind sharing a re-production with the memory leak?

[nagendra5]

Would you mind sharing a re-production with the memory leak?

Usually, it takes a lot of time to create a sample app that follows a certain sequence to reproduce the memory leak. This leak is always reproducible in my app but I am not supposed to share the code base. Please check the usage of _finalize_ran variable. Don't you think after calling _env->CallFinalizer(fini, _finalize_data, _finalize_hint); we have set '_finalize_ran' to true?.

memory leak got fixed in our app after setting _finalize_ran to true.

[legendecas]

I am still skeptical about if this change is the actual solution that settled the memory leak you mentioned.

The _finalize_ran is set to true just after the procedure of calling into user finalizer: https://github.com/nodejs/node/blob/main/src/js_native_api_v8.cc#L570. If _delete_self or is_env_teardown is set, _finalize_ran is being ignored in RefBase::Delete (https://github.com/nodejs/node/blob/main/src/js_native_api_v8.cc#L509).

FWIW, we are working on simplifying the finalization procedures at #44141.

[legendecas]

This PR needs to wait for the question raised in #44339 (comment) to be resolved before proceeding.

[mhdawson]

@legendecas do you think your changes related to reference have addressed the issue and that this PR is no longer needed/applicable?

[legendecas]

Right, telling from the change sites I believe they are relevant. @nagendra5 would you mind verifying if the leak is still observable in the latest build of Node.js v18.14.0 or later versions? Thank you.

[mhdawson]

@nagendra5 are you able to validate the issue still occurs with recent versions?