From 8a011617f1c899c48693135103031e97b493cd16 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 24 Jul 2026 14:00:37 -0700 Subject: [PATCH] io/ipchandler: deregister handlers through the registry they registered with An IpcHandler destroyed as part of a larger teardown - a Loader subtree being replaced, a plugin swapped out, the root object itself - can leave its registration behind. The registry then holds a pointer to a freed handler, and the next `qs ipc call` on that target takes the process down. ~IpcHandler asks updateRegistration() to remove the registration, and updateRegistration() re-resolves the registry by way of EngineGeneration::findObjectGeneration(). That walk needs a QML context, and a context is invalidated before the objects created in it are destroyed, so for a handler going down with its context the lookup returns null and the function returns before removing anything. The code already anticipates the lookup failing during destruction - that is why the "Unable to identify engine generation" warning is suppressed when `destroying` is set - but the early return also skips the removal the destructor asked for. Nothing complains at that point. The dead handler keeps the map entry, so the next handler registering for the same target is refused with "Handler was registered but will not be used because another handler is registered for target ...", and the crash lands later, on whichever caller reaches that target first: #4 IpcHandler::findFunction (this=0x7f605cab7140) at src/io/ipchandler.cpp:441 #5 StringCallCommand::exec (conn=...) at src/io/ipccomm.cpp:146 #12 IpcServerConnection::onReadyRead () at src/ipc/ipc.cpp:76 `this` there reads back as unrelated UTF-16 string data - the allocation had been reused - and quickshell.ipchandler logs show that handler registered once, during a reload three hours earlier, and never deregistered. Seen on 0.3.0 (10b439f) with Qt 6.11.1, on a config whose bar rebuilds its widgets when plugins reload. Remember the registry a handler registered with and remove through that, so removal no longer depends on the generation still being reachable. Registration continues to resolve the generation, since a handler that has never registered has no registry to remember. The registry can also be destroyed before its handlers: destroy() deletes a generation's extensions before it destroys the root object, so ~IpcHandlerRegistry clears the back-pointer of every handler it still knows about, and a handler with no registry simply drops its registration state. destroy() also left the deleted extension pointers in `extensions`, where findExtension() would hand one back to any object reaching for an extension on its way down. Clear the hash, and refuse registration once the generation is being destroyed. Verified on a shell with 20 IPC targets across ~24 handlers: repeated plugin reloads now produce one deregistration per destroyed handler (105 registrations, 81 deregistrations, the difference being exactly the live handlers), where before some reloads left handlers registered forever. Shutdown, config reload, and subtree teardown all leave the registry consistent, and a handler that disables itself from Component.onDestruction - which crashed on the freed registry before - no longer does. Co-Authored-By: Claude Opus 5 (1M context) --- src/core/generation.cpp | 6 ++++ src/core/generation.hpp | 4 +++ src/io/ipchandler.cpp | 61 +++++++++++++++++++++++++++++++---------- src/io/ipchandler.hpp | 9 ++++++ 4 files changed, 66 insertions(+), 14 deletions(-) diff --git a/src/core/generation.cpp b/src/core/generation.cpp index 21febc3..9a3fc13 100644 --- a/src/core/generation.cpp +++ b/src/core/generation.cpp @@ -82,6 +82,12 @@ void EngineGeneration::destroy() { delete extension; } + // The root object below is destroyed after this point, and objects can still + // reach for an extension while going down - a binding re-evaluating during + // destruction, for instance. Drop the deleted pointers so findExtension() + // cannot hand one back out. + this->extensions.clear(); + if (this->root != nullptr) { QObject::connect(this->root, &QObject::destroyed, this, [this]() { // prevent further js execution between garbage collection and engine destruction. diff --git a/src/core/generation.hpp b/src/core/generation.hpp index 4543408..2723c24 100644 --- a/src/core/generation.hpp +++ b/src/core/generation.hpp @@ -72,6 +72,10 @@ public: void destroy(); void shutdown(); + // True once destroy() has begun. Extensions are deleted at that point while + // the object tree is still alive, so objects going down must not ask for one. + [[nodiscard]] bool isDestroying() const { return this->destroying; } + signals: void filesChanged(); void reloadFinished(); diff --git a/src/io/ipchandler.cpp b/src/io/ipchandler.cpp index d4429fd..68fa368 100644 --- a/src/io/ipchandler.cpp +++ b/src/io/ipchandler.cpp @@ -304,24 +304,43 @@ void IpcHandler::onSignalTriggered(const QString& signal, const QString& value) void IpcHandler::updateRegistration(bool destroying) { if (!this->complete) return; - auto* generation = EngineGeneration::findObjectGeneration(this); - - if (!generation) { - if (!destroying) { - qmlWarning(this) << "Unable to identify engine generation, cannot register."; - } - - return; - } - - auto* registry = IpcHandlerRegistry::forGeneration(generation); - if (this->registeredState.enabled) { - registry->deregisterHandler(this); - qCDebug(logIpcHandler) << "Deregistered" << this << "from registry" << registry; + // Removal must not depend on findObjectGeneration(): a QML context is + // invalidated before the objects created in it are destroyed, so a handler + // that goes down as part of a larger teardown can no longer reach its + // generation, and returning early there would leave the registry holding a + // pointer to this handler after it is freed. The registry it registered + // with is still reachable, so remove through that. + if (this->registry) { + auto* registry = this->registry; + registry->deregisterHandler(this); + qCDebug(logIpcHandler) << "Deregistered" << this << "from registry" << registry; + } else { + // The registry was destroyed first and dropped this handler on its way + // out, so there is nothing left to remove the registration from. + this->registeredState = RegistrationState(false); + } } if (this->targetState.enabled && !this->targetState.target.isEmpty()) { + // Registration still resolves the generation: a handler that has never + // registered has no registry to remember. + auto* generation = EngineGeneration::findObjectGeneration(this); + + if (!generation) { + if (!destroying) { + qmlWarning(this) << "Unable to identify engine generation, cannot register."; + } + + return; + } + + // Extensions, the registry among them, are deleted at the start of + // EngineGeneration::destroy(), while the object tree is still alive. + // Nothing may register after that point. + if (generation->isDestroying()) return; + + auto* registry = IpcHandlerRegistry::forGeneration(generation); registry->registerHandler(this); qCDebug(logIpcHandler) << "Registered" << this << "to registry" << registry; } @@ -362,6 +381,7 @@ void IpcHandlerRegistry::registerHandler(IpcHandler* handler) { handler->registeredState = handler->targetState; handler->registeredState.enabled = true; + handler->registry = this; } void IpcHandlerRegistry::deregisterHandler(IpcHandler* handler) { @@ -377,6 +397,19 @@ void IpcHandlerRegistry::deregisterHandler(IpcHandler* handler) { } handler->registeredState = IpcHandler::RegistrationState(false); + handler->registry = nullptr; +} + +IpcHandlerRegistry::~IpcHandlerRegistry() { + // Handlers outlive the registry when a generation is torn down: + // EngineGeneration::destroy() deletes its extensions before it destroys the + // root object. Drop the back-pointers so the handlers destroyed afterwards + // don't reach into this registry. + for (const auto& targetVec: this->knownHandlers) { + for (auto* handler: targetVec) { + handler->registry = nullptr; + } + } } QString IpcHandler::listMembers(qsizetype indent) { diff --git a/src/io/ipchandler.hpp b/src/io/ipchandler.hpp index ab41dc4..449228d 100644 --- a/src/io/ipchandler.hpp +++ b/src/io/ipchandler.hpp @@ -266,6 +266,11 @@ private: RegistrationState registeredState; RegistrationState targetState {true}; bool complete = false; + // The registry this handler is registered with, or null if it is not + // registered or the registry has been destroyed. Deregistration goes through + // this rather than re-resolving the engine generation, which a handler + // cannot always reach while it is being destroyed. + IpcHandlerRegistry* registry = nullptr; QHash functionMap; QHash propertyMap; @@ -276,6 +281,10 @@ private: class IpcHandlerRegistry: public EngineGenerationExt { public: + IpcHandlerRegistry() = default; + ~IpcHandlerRegistry() override; + Q_DISABLE_COPY_MOVE(IpcHandlerRegistry); + static IpcHandlerRegistry* forGeneration(EngineGeneration* generation); void registerHandler(IpcHandler* handler); -- 2.55.0