src: add check against non-weak BaseObjects at process exit

When a process exits cleanly, i.e. because the event loop ends up
without things to wait for, the Node.js objects that are left on
the heap should be:

 1. weak, i.e. ready for garbage collection once no longer
    referenced, or
 2. detached, i.e. scheduled for destruction once no longer
    referenced, or
 3. an unrefed libuv handle, i.e. does not keep the event loop
    alive, or
 4. an inactive libuv handle (essentially the same here)

There are a few exceptions to this rule, but generally,
if there are C++-backed Node.js objects on the heap
that do not fall into the above categories, we may be looking
at a potential memory leak. Most likely, the cause is a missing
`MakeWeak()` call on the corresponding object.

In order to avoid this kind of problem, we check the list
of BaseObjects for these criteria. In this commit, we only do so
when explicitly instructed to or when in debug mode
(where --verify-base-objects is always-on).

In particular, this avoids the kinds of memory leak issues
that were fixed in the PRs referenced below.

Refs: #35488
Refs: #35487
Refs: #35481

PR-URL: #35490
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

Author: addaleax

src/async_wrap.cc

@@ -99,6 +99,12 @@ struct AsyncWrapObject : public AsyncWrap {
     return tmpl;
   }
 
+  bool IsNotIndicativeOfMemoryLeakAtExit() const override {
+    // We can't really know what the underlying operation does. One of the
+    // signs that it's time to remove this class. :)
+    return true;
+  }
+
   SET_NO_MEMORY_INFO()
   SET_MEMORY_INFO_NAME(AsyncWrapObject)
   SET_SELF_SIZE(AsyncWrapObject)

src/base_object-inl.h

@@ -146,6 +146,13 @@ void BaseObject::ClearWeak() {
   persistent_handle_.ClearWeak();
 }
 
+bool BaseObject::IsWeakOrDetached() const {
+  if (persistent_handle_.IsWeak()) return true;
+
+  if (!has_pointer_data()) return false;
+  const PointerData* pd = const_cast<BaseObject*>(this)->pointer_data();
+  return pd->wants_weak_jsobj || pd->is_detached;
+}
 
 v8::Local<v8::FunctionTemplate>
 BaseObject::MakeLazilyInitializedJSTemplate(Environment* env) {

src/base_object.h

@@ -78,6 +78,11 @@ class BaseObject : public MemoryRetainer {
   // root and will not be touched by the garbage collector.
   inline void ClearWeak();
 
+  // Reports whether this BaseObject is using a weak reference or detached,
+  // i.e. whether is can be deleted by GC once no strong BaseObjectPtrs refer
+  // to it anymore.
+  inline bool IsWeakOrDetached() const;
+
   // Utility to create a FunctionTemplate with one internal field (used for
   // the `BaseObject*` pointer) and a constructor that initializes that field
   // to `nullptr`.
@@ -147,6 +152,10 @@ class BaseObject : public MemoryRetainer {
   virtual v8::Maybe<bool> FinalizeTransferRead(
       v8::Local<v8::Context> context, v8::ValueDeserializer* deserializer);
 
+  // Indicates whether this object is expected to use a strong reference during
+  // a clean process exit (due to an empty event loop).
+  virtual bool IsNotIndicativeOfMemoryLeakAtExit() const;
+
   virtual inline void OnGCCollect();
 
  private:

src/env.cc

@@ -1194,22 +1194,43 @@ void Environment::RemoveUnmanagedFd(int fd) {
   }
 }
 
-void Environment::ForEachBaseObject(BaseObjectIterator iterator) {
+void Environment::PrintAllBaseObjects() {
   size_t i = 0;
-  for (const auto& hook : cleanup_hooks_) {
-    BaseObject* obj = hook.GetBaseObject();
-    if (obj != nullptr) iterator(i, obj);
-    i++;
-  }
-}
-
-void PrintBaseObject(size_t i, BaseObject* obj) {
-  std::cout << "#" << i << " " << obj << ": " << obj->MemoryInfoName() << "\n";
+  std::cout << "BaseObjects\n";
+  ForEachBaseObject([&](BaseObject* obj) {
+    std::cout << "#" << i++ << " " << obj << ": " <<
+      obj->MemoryInfoName() << "\n";
+  });
 }
 
-void Environment::PrintAllBaseObjects() {
-  std::cout << "BaseObjects\n";
-  ForEachBaseObject(PrintBaseObject);
+void Environment::VerifyNoStrongBaseObjects() {
+  // When a process exits cleanly, i.e. because the event loop ends up without
+  // things to wait for, the Node.js objects that are left on the heap should
+  // be:
+  //
+  //   1. weak, i.e. ready for garbage collection once no longer referenced, or
+  //   2. detached, i.e. scheduled for destruction once no longer referenced, or
+  //   3. an unrefed libuv handle, i.e. does not keep the event loop alive, or
+  //   4. an inactive libuv handle (essentially the same here)
+  //
+  // There are a few exceptions to this rule, but generally, if there are
+  // C++-backed Node.js objects on the heap that do not fall into the above
+  // categories, we may be looking at a potential memory leak. Most likely,
+  // the cause is a missing MakeWeak() call on the corresponding object.
+  //
+  // In order to avoid this kind of problem, we check the list of BaseObjects
+  // for these criteria. Currently, we only do so when explicitly instructed to
+  // or when in debug mode (where --verify-base-objects is always-on).
+
+  if (!options()->verify_base_objects) return;
+
+  ForEachBaseObject([this](BaseObject* obj) {
+    if (obj->IsNotIndicativeOfMemoryLeakAtExit()) return;
+    fprintf(stderr, "Found bad BaseObject during clean exit: %s\n",
+            obj->MemoryInfoName().c_str());
+    fflush(stderr);
+    ABORT();
+  });
 }
 
 EnvSerializeInfo Environment::Serialize(SnapshotCreator* creator) {
@@ -1473,4 +1494,8 @@ Local<FunctionTemplate> BaseObject::GetConstructorTemplate(Environment* env) {
   return tmpl;
 }
 
+bool BaseObject::IsNotIndicativeOfMemoryLeakAtExit() const {
+  return IsWeakOrDetached();
+}
+
 }  // namespace node

src/env.h

@@ -934,9 +934,8 @@ class Environment : public MemoryRetainer {
   void CreateProperties();
   void DeserializeProperties(const EnvSerializeInfo* info);
 
-  typedef void (*BaseObjectIterator)(size_t, BaseObject*);
-  void ForEachBaseObject(BaseObjectIterator iterator);
   void PrintAllBaseObjects();
+  void VerifyNoStrongBaseObjects();
   // Should be called before InitializeInspector()
   void InitializeDiagnostics();
 #if HAVE_INSPECTOR

src/handle_wrap.cc

@@ -89,6 +89,13 @@ void HandleWrap::OnGCCollect() {
 }
 
 
+bool HandleWrap::IsNotIndicativeOfMemoryLeakAtExit() const {
+  return IsWeakOrDetached() ||
+         !HandleWrap::HasRef(this) ||
+         !uv_is_active(GetHandle());
+}
+
+
 void HandleWrap::MarkAsInitialized() {
   env()->handle_wrap_queue()->PushBack(this);
   state_ = kInitialized;

src/handle_wrap.h

@@ -87,6 +87,7 @@ class HandleWrap : public AsyncWrap {
              AsyncWrap::ProviderType provider);
   virtual void OnClose() {}
   void OnGCCollect() final;
+  bool IsNotIndicativeOfMemoryLeakAtExit() const override;
 
   void MarkAsInitialized();
   void MarkAsUninitialized();

src/inspector_js_api.cc

@@ -156,6 +156,10 @@ class JSBindingsConnection : public AsyncWrap {
   SET_MEMORY_INFO_NAME(JSBindingsConnection)
   SET_SELF_SIZE(JSBindingsConnection)
 
+  bool IsNotIndicativeOfMemoryLeakAtExit() const override {
+    return true;  // Binding connections emit events on their own.
+  }
+
  private:
   std::unique_ptr<InspectorSession> session_;
   Global<Function> callback_;

src/module_wrap.h

@@ -60,6 +60,12 @@ class ModuleWrap : public BaseObject {
   SET_MEMORY_INFO_NAME(ModuleWrap)
   SET_SELF_SIZE(ModuleWrap)
 
+  bool IsNotIndicativeOfMemoryLeakAtExit() const override {
+    // XXX: The garbage collection rules for ModuleWrap are *super* unclear.
+    // Do these objects ever get GC'd? Are we just okay with leaking them?
+    return true;
+  }
+
  private:
   ModuleWrap(Environment* env,
              v8::Local<v8::Object> object,

src/node_contextify.h

@@ -174,6 +174,8 @@ class CompiledFnEntry final : public BaseObject {
                   v8::Local<v8::ScriptOrModule> script);
   ~CompiledFnEntry();
 
+  bool IsNotIndicativeOfMemoryLeakAtExit() const override { return true; }
+
  private:
   uint32_t id_;
   v8::Global<v8::ScriptOrModule> script_;

src/node_http_parser.cc

@@ -880,6 +880,15 @@ class Parser : public AsyncWrap, public StreamListener {
     return HPE_PAUSED;
   }
 
+
+  bool IsNotIndicativeOfMemoryLeakAtExit() const override {
+    // HTTP parsers are able to emit events without any GC root referring
+    // to them, because they receive events directly from the underlying
+    // libuv resource.
+    return true;
+  }
+
+
   llhttp_t parser_;
   StringPtr fields_[kMaxHeaderFieldsCount];  // header fields
   StringPtr values_[kMaxHeaderFieldsCount];  // header values

src/node_main_instance.cc

@@ -168,6 +168,7 @@ int NodeMainInstance::Run(const EnvSerializeInfo* env_info) {
       }
 
       env->set_trace_sync_io(false);
+      if (!env->is_stopping()) env->VerifyNoStrongBaseObjects();
       exit_code = EmitExit(env.get());
     }
 

src/node_options.cc

@@ -475,6 +475,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
             "an error), 'warn' (enforce warnings) or 'none' (silence warnings)",
             &EnvironmentOptions::unhandled_rejections,
             kAllowedInEnvironment);
+  AddOption("--verify-base-objects",
+            "", /* undocumented, only for debugging */
+            &EnvironmentOptions::verify_base_objects,
+            kAllowedInEnvironment);
 
   AddOption("--check",
             "syntax check script without executing",

src/node_options.h

@@ -150,6 +150,12 @@ class EnvironmentOptions : public Options {
   bool trace_warnings = false;
   std::string unhandled_rejections;
   std::string userland_loader;
+  bool verify_base_objects =
+#ifdef DEBUG
+      true;
+#else
+      false;
+#endif  // DEBUG
 
   bool syntax_check_only = false;
   bool has_eval_string = false;

src/node_worker.cc

@@ -362,8 +362,10 @@ void Worker::Run() {
     {
       int exit_code;
       bool stopped = is_stopped();
-      if (!stopped)
+      if (!stopped) {
+        env_->VerifyNoStrongBaseObjects();
         exit_code = EmitExit(env_.get());
+      }
       Mutex::ScopedLock lock(mutex_);
       if (exit_code_ == 0 && !stopped)
         exit_code_ = exit_code;
@@ -714,6 +716,12 @@ void Worker::MemoryInfo(MemoryTracker* tracker) const {
   tracker->TrackField("parent_port", parent_port_);
 }
 
+bool Worker::IsNotIndicativeOfMemoryLeakAtExit() const {
+  // Worker objects always stay alive as long as the child thread, regardless
+  // of whether they are being referenced in the parent thread.
+  return true;
+}
+
 class WorkerHeapSnapshotTaker : public AsyncWrap {
  public:
   WorkerHeapSnapshotTaker(Environment* env, Local<Object> obj)

src/node_worker.h

@@ -50,6 +50,7 @@ class Worker : public AsyncWrap {
   void MemoryInfo(MemoryTracker* tracker) const override;
   SET_MEMORY_INFO_NAME(Worker)
   SET_SELF_SIZE(Worker)
+  bool IsNotIndicativeOfMemoryLeakAtExit() const override;
 
   bool is_stopped() const;
 

src/stream_base.h

@@ -422,6 +422,10 @@ class SimpleShutdownWrap : public ShutdownWrap, public OtherBase {
   SET_NO_MEMORY_INFO()
   SET_MEMORY_INFO_NAME(SimpleShutdownWrap)
   SET_SELF_SIZE(SimpleShutdownWrap)
+
+  bool IsNotIndicativeOfMemoryLeakAtExit() const override {
+    return OtherBase::IsNotIndicativeOfMemoryLeakAtExit();
+  }
 };
 
 template <typename OtherBase>
@@ -435,6 +439,10 @@ class SimpleWriteWrap : public WriteWrap, public OtherBase {
   SET_NO_MEMORY_INFO()
   SET_MEMORY_INFO_NAME(SimpleWriteWrap)
   SET_SELF_SIZE(SimpleWriteWrap)
+
+  bool IsNotIndicativeOfMemoryLeakAtExit() const override {
+    return OtherBase::IsNotIndicativeOfMemoryLeakAtExit();
+  }
 };
 
 }  // namespace node

test/parallel/test-process-env-allowed-flags-are-documented.js

@@ -88,6 +88,7 @@ assert(undocumented.delete('--experimental-report'));
 assert(undocumented.delete('--experimental-worker'));
 assert(undocumented.delete('--no-node-snapshot'));
 assert(undocumented.delete('--loader'));
+assert(undocumented.delete('--verify-base-objects'));
 
 assert.strictEqual(undocumented.size, 0,
                    'The following options are not documented as allowed in ' +