diff --git a/src/async_wrap.cc b/src/async_wrap.cc index b03c6600873b23..a45b34b12b06bf 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -99,6 +99,12 @@ struct AsyncWrapObject : public AsyncWrap { return tmpl; } + bool IsAllowedStrongObjectAtExit() 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 5ea2ea57d874f5..51e5725597ea69 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 IsAllowedStrongObjectAtExit() const; + virtual inline void OnGCCollect(); private: diff --git a/src/env.cc b/src/env.cc index 4aeabbde8868c2..d8de0915b4be09 100644 --- a/src/env.cc +++ b/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->IsAllowedStrongObjectAtExit()) 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 BaseObject::GetConstructorTemplate(Environment* env) { return tmpl; } +bool BaseObject::IsAllowedStrongObjectAtExit() const { + return IsWeakOrDetached(); +} + } // namespace node diff --git a/src/env.h b/src/env.h index 410d5c3fc9dd06..6021f0d9940798 100644 --- a/src/env.h +++ b/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 diff --git a/src/handle_wrap.cc b/src/handle_wrap.cc index e57369844688e3..d834173fe066c6 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -89,6 +89,13 @@ void HandleWrap::OnGCCollect() { } +bool HandleWrap::IsAllowedStrongObjectAtExit() 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 47eef32940bccc..83ad0e4422252b 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -87,6 +87,7 @@ class HandleWrap : public AsyncWrap { AsyncWrap::ProviderType provider); virtual void OnClose() {} void OnGCCollect() final; + bool IsAllowedStrongObjectAtExit() const override; void MarkAsInitialized(); void MarkAsUninitialized(); diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index 131d8d74b980d9..5f05abb091b4e8 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -156,6 +156,10 @@ class JSBindingsConnection : public AsyncWrap { SET_MEMORY_INFO_NAME(JSBindingsConnection) SET_SELF_SIZE(JSBindingsConnection) + bool IsAllowedStrongObjectAtExit() 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..ba6fc019a96682 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 IsAllowedStrongObjectAtExit() 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..8c3d2d0b788128 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 IsAllowedStrongObjectAtExit() 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 eada685f2d6427..09ff24e41cc18d 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 IsAllowedStrongObjectAtExit() 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 14e0d49ce959f0..3767e75c5682a2 100644 --- a/src/node_main_instance.cc +++ b/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()); } diff --git a/src/node_options.cc b/src/node_options.cc index abbb7bc82ab599..9c3d74c85ca100 100644 --- a/src/node_options.cc +++ b/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", diff --git a/src/node_options.h b/src/node_options.h index bd63fcb0485c77..e8051f64bc3f38 100644 --- a/src/node_options.h +++ b/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; diff --git a/src/node_worker.cc b/src/node_worker.cc index b3dd29bf9fb17b..cba21c546b80c6 100644 --- a/src/node_worker.cc +++ b/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::IsAllowedStrongObjectAtExit() 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 658018bb0661f8..e34ea96a0eae5d 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 IsAllowedStrongObjectAtExit() const override; bool is_stopped() const; diff --git a/src/stream_base.h b/src/stream_base.h index 7c6bcba81edd03..300ac1dd04bc0b 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -412,6 +412,10 @@ class SimpleShutdownWrap : public ShutdownWrap, public OtherBase { SET_NO_MEMORY_INFO() SET_MEMORY_INFO_NAME(SimpleShutdownWrap) SET_SELF_SIZE(SimpleShutdownWrap) + + bool IsAllowedStrongObjectAtExit() const override { + return OtherBase::IsAllowedStrongObjectAtExit(); + } }; template @@ -425,6 +429,10 @@ class SimpleWriteWrap : public WriteWrap, public OtherBase { SET_NO_MEMORY_INFO() SET_MEMORY_INFO_NAME(SimpleWriteWrap) SET_SELF_SIZE(SimpleWriteWrap) + + bool IsAllowedStrongObjectAtExit() const override { + return OtherBase::IsAllowedStrongObjectAtExit(); + } }; } // 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 ' +