From b6d4e2797a761794e292a664ee92823034c17c88 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 19 Jul 2021 11:52:58 +0200 Subject: [PATCH] src: close HandleWraps instead of deleting them in OnGCCollect() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When all strong `BaseObjectPtr`s to a `HandleWrap` are gone, we should not delete the `HandleWrap` outright, but instead close it and then delete it only once the libuv close callback has been called. Based on the valgrind output from the issue below, this has a good chance of fixing it. Fixes: https://github.com/nodejs/node/issues/39036 PR-URL: https://github.com/nodejs/node/pull/39441 Reviewed-By: Tobias Nießen Reviewed-By: Gireesh Punathil Reviewed-By: Colin Ihrig Reviewed-By: James M Snell --- src/base_object-inl.h | 2 +- src/handle_wrap.cc | 11 ++++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/base_object-inl.h b/src/base_object-inl.h index ff3610c60a822c..ad900b6399f149 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -201,7 +201,7 @@ void BaseObject::decrease_refcount() { unsigned int new_refcount = --metadata->strong_ptr_count; if (new_refcount == 0) { if (metadata->is_detached) { - delete this; + OnGCCollect(); } else if (metadata->wants_weak_jsobj && !persistent_handle_.IsEmpty()) { MakeWeak(); } diff --git a/src/handle_wrap.cc b/src/handle_wrap.cc index d2bd67a5e405d1..caad0e0554622a 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -85,7 +85,16 @@ void HandleWrap::Close(Local close_callback) { void HandleWrap::OnGCCollect() { - Close(); + // When all references to a HandleWrap are lost and the object is supposed to + // be destroyed, we first call Close() to clean up the underlying libuv + // handle. The OnClose callback then acquires and destroys another reference + // to that object, and when that reference is lost, we perform the default + // action (i.e. destroying `this`). + if (state_ != kClosed) { + Close(); + } else { + BaseObject::OnGCCollect(); + } }