From e9a574d919a89602d2868621576b2ccae54a5cb0 Mon Sep 17 00:00:00 2001 From: outfoxxed Date: Fri, 19 Sep 2025 00:16:26 -0700 Subject: [PATCH] core: derive incubation controllers from tracked windows list Replaces the attempts to track incubation controllers directly with a list of all known windows, then pulls the first usable incubation controller when an assignment is requested. This should finally fix incubation controller related use after free crashes. --- src/core/generation.cpp | 112 +++++++------------------------------ src/core/generation.hpp | 8 +-- src/window/proxywindow.cpp | 10 +--- 3 files changed, 27 insertions(+), 103 deletions(-) diff --git a/src/core/generation.cpp b/src/core/generation.cpp index fee9441..e15103a 100644 --- a/src/core/generation.cpp +++ b/src/core/generation.cpp @@ -11,12 +11,12 @@ #include #include #include -#include #include #include #include #include #include +#include #include #include "iconimageprovider.hpp" @@ -242,90 +242,6 @@ void EngineGeneration::onDirectoryChanged() { } } -void EngineGeneration::registerIncubationController(QQmlIncubationController* controller) { - // We only want controllers that we can swap out if destroyed. - // This happens if the window owning the active controller dies. - auto* obj = dynamic_cast(controller); - if (!obj) { - qCWarning(logIncubator) << "Could not register incubation controller as it is not a QObject" - << controller; - - return; - } - - QObject::connect( - obj, - &QObject::destroyed, - this, - &EngineGeneration::incubationControllerDestroyed, - Qt::UniqueConnection - ); - - this->incubationControllers.push_back(obj); - qCDebug(logIncubator) << "Registered incubation controller" << obj << "to generation" << this; - - // This function can run during destruction. - if (this->engine == nullptr) return; - - if (this->engine->incubationController() == &this->delayedIncubationController) { - this->assignIncubationController(); - } -} - -// Multiple controllers may be destroyed at once. Dynamic casts must be performed before working -// with any controllers. The QQmlIncubationController destructor will already have run by the -// point QObject::destroyed is called, so we can't cast to that. -void EngineGeneration::deregisterIncubationController(QQmlIncubationController* controller) { - auto* obj = dynamic_cast(controller); - if (!obj) { - qCCritical(logIncubator) << "Deregistering incubation controller which is not a QObject, " - "however only QObject controllers should be registered."; - } - - QObject::disconnect(obj, nullptr, this, nullptr); - - if (this->incubationControllers.removeOne(obj)) { - qCDebug(logIncubator) << "Deregistered incubation controller" << obj << "from" << this; - } else { - qCCritical(logIncubator) << "Failed to deregister incubation controller" << obj << "from" - << this << "as it was not registered to begin with"; - qCCritical(logIncubator) << "Current registered incuabation controllers" - << this->incubationControllers; - } - - // This function can run during destruction. - if (this->engine == nullptr) return; - - if (this->engine->incubationController() == controller) { - qCDebug(logIncubator - ) << "Destroyed incubation controller was currently active, reassigning from pool"; - this->assignIncubationController(); - } -} - -void EngineGeneration::incubationControllerDestroyed() { - auto* sender = this->sender(); - - if (this->incubationControllers.removeAll(sender) != 0) { - qCDebug(logIncubator) << "Destroyed incubation controller" << sender << "deregistered from" - << this; - } else { - qCCritical(logIncubator) << "Destroyed incubation controller" << sender - << "was not registered, but its destruction was observed by" << this; - - return; - } - - // This function can run during destruction. - if (this->engine == nullptr) return; - - if (dynamic_cast(this->engine->incubationController()) == sender) { - qCDebug(logIncubator - ) << "Destroyed incubation controller was currently active, reassigning from pool"; - this->assignIncubationController(); - } -} - void EngineGeneration::onEngineWarnings(const QList& warnings) { for (const auto& error: warnings) { const auto& url = error.url(); @@ -367,13 +283,27 @@ void EngineGeneration::exit(int code) { this->destroy(); } -void EngineGeneration::assignIncubationController() { - QQmlIncubationController* controller = nullptr; +void EngineGeneration::trackWindowIncubationController(QQuickWindow* window) { + if (this->trackedWindows.contains(window)) return; - if (this->incubationControllersLocked || this->incubationControllers.isEmpty()) { - controller = &this->delayedIncubationController; - } else { - controller = dynamic_cast(this->incubationControllers.first()); + QObject::connect(window, &QObject::destroyed, this, &EngineGeneration::onTrackedWindowDestroyed); + this->trackedWindows.append(window); + this->assignIncubationController(); +} + +void EngineGeneration::onTrackedWindowDestroyed(QObject* object) { + this->trackedWindows.removeAll(static_cast(object)); // NOLINT + this->assignIncubationController(); +} + +void EngineGeneration::assignIncubationController() { + QQmlIncubationController* controller = &this->delayedIncubationController; + + for (auto* window: this->trackedWindows) { + if (auto* wctl = window->incubationController()) { + controller = wctl; + break; + } } qCDebug(logIncubator) << "Assigning incubation controller" << controller << "to generation" diff --git a/src/core/generation.hpp b/src/core/generation.hpp index 3c0c4ae..fef8363 100644 --- a/src/core/generation.hpp +++ b/src/core/generation.hpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include "incubator.hpp" @@ -40,8 +41,7 @@ public: void setWatchingFiles(bool watching); bool setExtraWatchedFiles(const QVector& files); - void registerIncubationController(QQmlIncubationController* controller); - void deregisterIncubationController(QQmlIncubationController* controller); + void trackWindowIncubationController(QQuickWindow* window); // takes ownership void registerExtension(const void* key, EngineGenerationExt* extension); @@ -84,13 +84,13 @@ public slots: private slots: void onFileChanged(const QString& name); void onDirectoryChanged(); - void incubationControllerDestroyed(); + void onTrackedWindowDestroyed(QObject* object); static void onEngineWarnings(const QList& warnings); private: void postReload(); void assignIncubationController(); - QVector incubationControllers; + QVector trackedWindows; bool incubationControllersLocked = false; QHash extensions; diff --git a/src/window/proxywindow.cpp b/src/window/proxywindow.cpp index 618751a..ea2904b 100644 --- a/src/window/proxywindow.cpp +++ b/src/window/proxywindow.cpp @@ -153,13 +153,7 @@ void ProxyWindowBase::createWindow() { void ProxyWindowBase::deleteWindow(bool keepItemOwnership) { if (this->window != nullptr) emit this->windowDestroyed(); - if (auto* window = this->disownWindow(keepItemOwnership)) { - if (auto* generation = EngineGeneration::findObjectGeneration(this)) { - generation->deregisterIncubationController(window->incubationController()); - } - - window->deleteLater(); - } + if (auto* window = this->disownWindow(keepItemOwnership)) window->deleteLater(); } ProxiedWindow* ProxyWindowBase::disownWindow(bool keepItemOwnership) { @@ -185,7 +179,7 @@ void ProxyWindowBase::connectWindow() { if (auto* generation = EngineGeneration::findObjectGeneration(this)) { // All windows have effectively the same incubation controller so it dosen't matter // which window it belongs to. We do want to replace the delay one though. - generation->registerIncubationController(this->window->incubationController()); + generation->trackWindowIncubationController(this->window); } this->window->setProxy(this);