From ade01e909152800604880e525e26893d268d31e8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 3 Oct 2020 22:45:08 +0200 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/35488 Refs: https://github.com/nodejs/node/pull/35487 Refs: https://github.com/nodejs/node/pull/35481 --- src/async_wrap.cc | 6 +++ src/base_object-inl.h | 7 +++ src/base_object.h | 9 ++++ src/env.cc | 51 ++++++++++++++----- src/env.h | 3 +- src/handle_wrap.cc | 7 +++ src/handle_wrap.h | 1 + src/inspector_js_api.cc | 4 ++ src/module_wrap.h | 6 +++ src/node_contextify.h | 2 + src/node_http_parser.cc | 9 ++++ src/node_main_instance.cc | 1 + src/node_options.cc | 4 ++ src/node_options.h | 6 +++ src/node_worker.cc | 10 +++- src/node_worker.h | 1 + src/stream_base.h | 8 +++ ...rocess-env-allowed-flags-are-documented.js | 1 + 18 files changed, 120 insertions(+), 16 deletions(-) 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 ' +