diff --git a/src/async_wrap.cc b/src/async_wrap.cc index d6511a1dd8b3c3..2dfcf96e5620ec 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -98,6 +98,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) diff --git a/src/base_object-inl.h b/src/base_object-inl.h index 555063f4e23e67..0b620ab864f67b 100644 --- a/src/base_object-inl.h +++ b/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(this)->pointer_data(); + return pd->wants_weak_jsobj || pd->is_detached; +} v8::Local BaseObject::MakeLazilyInitializedJSTemplate(Environment* env) { diff --git a/src/base_object.h b/src/base_object.h index 105114caf4459a..9392e1baa14fe9 100644 --- a/src/base_object.h +++ b/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 FinalizeTransferRead( v8::Local 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: diff --git a/src/env.cc b/src/env.cc index 7155ae67ac625b..dc67a0e5f98196 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1043,6 +1043,36 @@ void Environment::RemoveUnmanagedFd(int fd) { } } +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(); + }); +} + void Environment::BuildEmbedderGraph(Isolate* isolate, EmbedderGraph* graph, void* data) { @@ -1135,4 +1165,8 @@ Local BaseObject::GetConstructorTemplate(Environment* env) { return tmpl; } +bool BaseObject::IsNotIndicativeOfMemoryLeakAtExit() const { + return IsWeakOrDetached(); +} + } // namespace node diff --git a/src/env.h b/src/env.h index 9c435f12db0e2d..512dc878049e9a 100644 --- a/src/env.h +++ b/src/env.h @@ -833,6 +833,7 @@ class Environment : public MemoryRetainer { void MemoryInfo(MemoryTracker* tracker) const override; void CreateProperties(); + void VerifyNoStrongBaseObjects(); // Should be called before InitializeInspector() void InitializeDiagnostics(); #if HAVE_INSPECTOR diff --git a/src/handle_wrap.cc b/src/handle_wrap.cc index 4e039d5dbf2d37..447c0842b90d99 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -88,6 +88,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; diff --git a/src/handle_wrap.h b/src/handle_wrap.h index a555da9479de93..560b2e718176fa 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -85,6 +85,7 @@ class HandleWrap : public AsyncWrap { AsyncWrap::ProviderType provider); virtual void OnClose() {} void OnGCCollect() final; + bool IsNotIndicativeOfMemoryLeakAtExit() const override; void MarkAsInitialized(); void MarkAsUninitialized(); diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index 8616575e066121..f1eb5fe7fd7448 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -155,6 +155,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 session_; Global callback_; diff --git a/src/module_wrap.h b/src/module_wrap.h index 3d8bb57750397f..dc7726f11d94fd 100644 --- a/src/module_wrap.h +++ b/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 object, diff --git a/src/node_contextify.h b/src/node_contextify.h index 1236613c73eeb8..357029d8e18312 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -174,6 +174,8 @@ class CompiledFnEntry final : public BaseObject { v8::Local script); ~CompiledFnEntry(); + bool IsNotIndicativeOfMemoryLeakAtExit() const override { return true; } + private: uint32_t id_; v8::Global script_; diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index ed92cc459a5a66..cb44320c380e02 100644 --- a/src/node_http_parser.cc +++ b/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 diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index f638e26dba5a04..0678d1a748edf6 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -147,6 +147,7 @@ int NodeMainInstance::Run() { } env->set_trace_sync_io(false); + if (!env->is_stopping()) env->VerifyNoStrongBaseObjects(); exit_code = EmitExit(env.get()); } diff --git a/src/node_options.cc b/src/node_options.cc index 0ed8dac7217753..e57e6d46c78552 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -483,6 +483,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { "exit code 1 unless 'unhandledRejection' hook is set).", &EnvironmentOptions::unhandled_rejections, kAllowedInEnvironment); + AddOption("--verify-base-objects", + "", /* undocumented, only for debugging */ + &EnvironmentOptions::verify_base_objects, + kAllowedInEnvironment); AddOption("--check", "syntax check script without executing", diff --git a/src/node_options.h b/src/node_options.h index c9624c04a6eff1..58a21e0d4f9bd6 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -151,6 +151,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; diff --git a/src/node_worker.cc b/src/node_worker.cc index dd132a87cfcbaa..e2f70f6b357766 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -366,8 +366,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; @@ -718,6 +720,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 obj) diff --git a/src/node_worker.h b/src/node_worker.h index a59bdc3f5bae17..2c65f0e1a83bbe 100644 --- a/src/node_worker.h +++ b/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; diff --git a/src/stream_base.h b/src/stream_base.h index e106b004753836..a5680ba8860d49 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -423,6 +423,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 @@ -436,6 +440,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 diff --git a/test/parallel/test-process-env-allowed-flags-are-documented.js b/test/parallel/test-process-env-allowed-flags-are-documented.js index 0e0af9471ce84e..6f4537bd3e88fa 100644 --- a/test/parallel/test-process-env-allowed-flags-are-documented.js +++ b/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 ' +