src: use cleanup hooks to tear down BaseObjects

Clean up after `BaseObject` instances when the `Environment`
is being shut down. This takes care of closing non-libuv resources
like `zlib` instances, which do not require asynchronous shutdown.

Many thanks for Stephen Belanger, Timothy Gu and Alexey Orlenko for
reviewing the original version of this commit in the Ayo.js project.

Refs: ayojs/ayo#88
PR-URL: #19377
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Author: addaleax

src/base_object-inl.h

@@ -37,10 +37,13 @@ BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> object)
   CHECK_EQ(false, object.IsEmpty());
   CHECK_GT(object->InternalFieldCount(), 0);
   object->SetAlignedPointerInInternalField(0, static_cast<void*>(this));
+  env_->AddCleanupHook(DeleteMe, static_cast<void*>(this));
 }
 
 
 BaseObject::~BaseObject() {
+  env_->RemoveCleanupHook(DeleteMe, static_cast<void*>(this));
+
   if (persistent_handle_.IsEmpty()) {
     // This most likely happened because the weak callback below cleared it.
     return;
@@ -80,6 +83,12 @@ T* BaseObject::FromJSObject(v8::Local<v8::Object> object) {
 }
 
 
+void BaseObject::DeleteMe(void* data) {
+  BaseObject* self = static_cast<BaseObject*>(data);
+  delete self;
+}
+
+
 void BaseObject::MakeWeak() {
   persistent_handle_.SetWeak(
       this,

src/base_object.h

@@ -71,6 +71,8 @@ class BaseObject {
  private:
   BaseObject();
 
+  static inline void DeleteMe(void* data);
+
   // persistent_handle_ needs to be at a fixed offset from the start of the
   // class because it is used by src/node_postmortem_metadata.cc to calculate
   // offsets and generate debug symbols for BaseObject, which assumes that the

src/env.cc

@@ -133,6 +133,10 @@ Environment::Environment(IsolateData* isolate_data,
 }
 
 Environment::~Environment() {
+  // Make sure there are no re-used libuv wrapper objects.
+  // CleanupHandles() should have removed all of them.
+  CHECK(file_handle_read_wrap_freelist_.empty());
+
   v8::HandleScope handle_scope(isolate());
 
 #if HAVE_INSPECTOR
@@ -245,6 +249,8 @@ void Environment::CleanupHandles() {
          !handle_wrap_queue_.IsEmpty()) {
     uv_run(event_loop(), UV_RUN_ONCE);
   }
+
+  file_handle_read_wrap_freelist_.clear();
 }
 
 void Environment::StartProfilerIdleNotifier() {

src/inspector_agent.cc

@@ -576,6 +576,8 @@ std::unique_ptr<InspectorSession> Agent::Connect(
 
 void Agent::WaitForDisconnect() {
   CHECK_NE(client_, nullptr);
+  // TODO(addaleax): Maybe this should use an at-exit hook for the Environment
+  // or something similar?
   client_->contextDestroyed(parent_env_->context());
   if (io_ != nullptr) {
     io_->WaitForDisconnect();

src/node.cc

@@ -4578,12 +4578,13 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
 
   const int exit_code = EmitExit(&env);
 
+  WaitForInspectorDisconnect(&env);
+
   env.RunCleanup();
   RunAtExit(&env);
 
   v8_platform.DrainVMTasks(isolate);
   v8_platform.CancelVMTasks(isolate);
-  WaitForInspectorDisconnect(&env);
 #if defined(LEAK_SANITIZER)
   __lsan_do_leak_check();
 #endif

src/req_wrap-inl.h

@@ -26,7 +26,6 @@ ReqWrap<T>::ReqWrap(Environment* env,
 
 template <typename T>
 ReqWrap<T>::~ReqWrap() {
-  CHECK_EQ(req_.data, this);  // Assert that someone has called Dispatched().
   CHECK_EQ(false, persistent().IsEmpty());
 }