From e6ff7e648ec79e8d06561d4ca169196b0e2c00c7 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 0b620ab864f67b..43bcdc6404fe79 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -207,7 +207,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 447c0842b90d99..d8582918bb616a 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -84,7 +84,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(); + } }