Skip to content

Commit

Permalink
fix: missing napi_delete_reference on ObjectWrap ref
Browse files Browse the repository at this point in the history
  • Loading branch information
legendecas committed Oct 31, 2024
1 parent 19002d5 commit 388dd68
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 9 deletions.
13 changes: 4 additions & 9 deletions napi-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -3341,13 +3341,7 @@ template <typename T>
inline Reference<T>::~Reference() {
if (_ref != nullptr) {
if (!_suppressDestruct) {
#ifdef NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER
Env().PostFinalizer(
[](Napi::Env env, napi_ref ref) { napi_delete_reference(env, ref); },
_ref);
#else
napi_delete_reference(_env, _ref);
#endif
}

_ref = nullptr;
Expand Down Expand Up @@ -4595,7 +4589,7 @@ template <typename T>
inline ObjectWrap<T>::~ObjectWrap() {
// If the JS object still exists at this point, remove the finalizer added
// through `napi_wrap()`.
if (!IsEmpty()) {
if (!IsEmpty() && !_finalized) {
Object object = Value();
// It is not valid to call `napi_remove_wrap()` with an empty `object`.
// This happens e.g. during garbage collection.
Expand Down Expand Up @@ -5044,8 +5038,9 @@ inline void ObjectWrap<T>::FinalizeCallback(node_addon_api_basic_env env,
(void)env;
T* instance = static_cast<T*>(data);

// Prevent ~ObjectWrap from calling napi_remove_wrap
instance->_ref = nullptr;
// Prevent ~ObjectWrap from calling napi_remove_wrap.
// The instance->_ref should be deleted with napi_delete_reference in ~Reference.
instance->_finalized = true;

// If class overrides the basic finalizer, execute it.
if constexpr (details::HasBasicFinalizer<T>::value) {
Expand Down
1 change: 1 addition & 0 deletions napi.h
Original file line number Diff line number Diff line change
Expand Up @@ -2514,6 +2514,7 @@ class ObjectWrap : public InstanceWrap<T>, public Reference<Object> {
}

bool _construction_failed = true;
bool _finalized = false;
};

class HandleScope {
Expand Down

0 comments on commit 388dd68

Please sign in to comment.