Merge pull request 'fix(ui): Improper Overlay Shutdown & Zombie Processes' (#89) from fix/zombie-overlays into main

Reviewed-on: https://git.citron-emu.org/Citron/Emulator/pulls/89

src/citron/configuration/configure_input_player_widget.cpp

@@ -3,6 +3,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 
 #include <algorithm>
+#include <QCoreApplication>
 #include <QMenu>
 #include <QPainter>
 #include <QTimer>
@@ -23,8 +24,16 @@ PlayerControlPreview::PlayerControlPreview(QWidget* parent) : QFrame(parent) {
 }
 
 PlayerControlPreview::~PlayerControlPreview() {
+    // If the app is closing down, the "controller" memory is likely garbage.
+    // We must NOT touch it or call UnloadController().
+    if (QCoreApplication::closingDown()) {
+        controller = nullptr;
+        is_controller_set = false;
+        return;
+    }
+
     UnloadController();
-};
+}
 
 void PlayerControlPreview::SetRawJoystickVisible(bool visible) {
     raw_joystick_visible = visible;
@@ -51,14 +60,22 @@ void PlayerControlPreview::SetController(Core::HID::EmulatedController* controll
 }
 
 void PlayerControlPreview::UnloadController() {
-    // Only try to access the controller if the pointer is valid.
+    // If the app is closing or the key is invalid, just reset pointers and quit.
+    if (QCoreApplication::closingDown() || callback_key < 0) {
+        is_controller_set = false;
+        controller = nullptr;
+        callback_key = -1;
+        return;
+    }
+
     if (controller) {
         controller->DeleteCallback(callback_key);
-        // Also clear the destruction callback we set.
         controller->SetDestructionCallback(nullptr);
     }
+
     is_controller_set = false;
     controller = nullptr;
+    callback_key = -1;
 }
 
 void PlayerControlPreview::BeginMappingButton(std::size_t button_id) {

src/citron/controller_overlay.cpp

@@ -9,6 +9,7 @@
 #include "hid_core/hid_core.h"
 
 #include <QApplication>
+#include <QCoreApplication>
 #include <QGridLayout>
 #include <QMouseEvent>
 #include <QPainter>
@@ -75,10 +76,19 @@ ControllerOverlay::ControllerOverlay(GMainWindow* parent)
     }
 }
 
-ControllerOverlay::~ControllerOverlay() = default;
+ControllerOverlay::~ControllerOverlay() {
+    update_timer.stop();
+}
 
 void ControllerOverlay::UpdateControllerState() {
-    if (!main_window || !is_enabled) return;
+    // If we're shutting down, kill the timer and hide.
+    if (QCoreApplication::closingDown() || !main_window || main_window->isHidden()) {
+        update_timer.stop();
+        if (!this->isHidden()) this->hide();
+        return;
+    }
+
+    if (!is_enabled) return;
 
     if (UISettings::IsGamescope()) {
         bool ui_active = false;

src/citron/main.cpp

@@ -343,8 +343,10 @@ bool GMainWindow::CheckDarkMode() {
 }
 
 GMainWindow::GMainWindow(std::unique_ptr<QtConfig> config_, bool has_broken_vulkan)
-    : ui{std::make_unique<Ui::MainWindow>()}, system{std::make_unique<Core::System>()},
-      input_subsystem{std::make_shared<InputCommon::InputSubsystem>()}, config{std::move(config_)},
+    : system{std::make_unique<Core::System>()},
+      input_subsystem{std::make_shared<InputCommon::InputSubsystem>()},
+      ui{std::make_unique<Ui::MainWindow>()},
+      config{std::move(config_)},
       vfs{std::make_shared<FileSys::RealVfsFilesystem>()},
       provider{std::make_unique<FileSys::ManualContentProvider>()} {
 #ifdef __unix__
@@ -5793,27 +5795,42 @@ void GMainWindow::closeEvent(QCloseEvent* event) {
         return;
     }
 
-    // This stops mirroring threads before we start saving configs.
+    // 1. STOP the emulation first
     if (emu_thread != nullptr) {
         ShutdownGame();
     }
 
-    // Now save settings
+    // 2. FORCE the UI to stop talking to controllers
+    // Do this BEFORE UnloadInputDevices
+    if (controller_overlay) {
+        // We delete it here so its destructor runs while 'system' is still healthy
+        delete controller_overlay;
+        controller_overlay = nullptr;
+    }
+
+    if (game_list) {
+        game_list->UnloadController();
+    }
+
+    if (controller_dialog) {
+        controller_dialog->UnloadController();
+    }
+
+    // 3. Save settings
     UpdateUISettings();
     config->SaveAllValues();
-
     game_list->SaveInterfaceLayout();
     UISettings::SaveWindowState();
     hotkey_registry.SaveHotkeys();
 
-    // Unload controllers
-    controller_dialog->UnloadController();
-    game_list->UnloadController();
-
+    // 4. NOW it is safe to kill the hardware devices
     render_window->close();
     multiplayer_state->Close();
-    system->HIDCore().UnloadInputDevices();
-    system->GetRoomNetwork().Shutdown();
+
+    if (system) {
+        system->HIDCore().UnloadInputDevices();
+        system->GetRoomNetwork().Shutdown();
+    }
 
     QWidget::closeEvent(event);
 }

src/citron/main.h

@@ -334,11 +334,11 @@ private:
     bool MakeShortcutIcoPath(const u64 program_id, const std::string_view game_file_name, std::filesystem::path& out_icon_path);
     bool CreateShortcutLink(const std::filesystem::path& shortcut_path, const std::string& comment, const std::filesystem::path& icon_path, const std::filesystem::path& command, const std::string& arguments, const std::string& categories, const std::string& keywords, const std::string& name);
     bool question(QWidget* parent, const QString& title, const QString& text, QMessageBox::StandardButtons buttons = QMessageBox::StandardButtons(QMessageBox::Yes | QMessageBox::No), QMessageBox::StandardButton defaultButton = QMessageBox::NoButton);
-    std::unique_ptr<Ui::MainWindow> ui;
     std::unique_ptr<Core::System> system;
+    std::shared_ptr<InputCommon::InputSubsystem> input_subsystem;
+    std::unique_ptr<Ui::MainWindow> ui;
     std::unique_ptr<DiscordRPC::DiscordInterface> discord_rpc;
     std::unique_ptr<PlayTime::PlayTimeManager> play_time_manager;
-    std::shared_ptr<InputCommon::InputSubsystem> input_subsystem;
     MultiplayerState* multiplayer_state = nullptr;
     GRenderWindow* render_window;
     GameList* game_list;

src/citron/util/controller_navigation.cpp

@@ -2,20 +2,33 @@
 // SPDX-FileCopyrightText: Copyright 2025 Citron Emulator Project
 // SPDX-License-Identifier: GPL-2.0-or-later
 
+#include <QCoreApplication>
 #include "common/settings_input.h"
 #include "hid_core/frontend/emulated_controller.h"
 #include "hid_core/hid_core.h"
 #include "citron/util/controller_navigation.h"
 
 ControllerNavigation::ControllerNavigation(Core::HID::HIDCore& hid_core, QWidget* parent) {
+    // Initialize keys to -1 immediately
+    player1_callback_key = -1;
+    handheld_callback_key = -1;
+    is_controller_set = false;
+
     player1_controller = hid_core.GetEmulatedController(Core::HID::NpadIdType::Player1);
     handheld_controller = hid_core.GetEmulatedController(Core::HID::NpadIdType::Handheld);
+
     Core::HID::ControllerUpdateCallback engine_callback{
         .on_change = [this](Core::HID::ControllerTriggerType type) { ControllerUpdateEvent(type); },
         .is_npad_service = false,
     };
-    player1_callback_key = player1_controller->SetCallback(engine_callback);
-    handheld_callback_key = handheld_controller->SetCallback(engine_callback);
+
+    if (player1_controller) {
+        player1_callback_key = player1_controller->SetCallback(engine_callback);
+    }
+    if (handheld_controller) {
+        handheld_callback_key = handheld_controller->SetCallback(engine_callback);
+    }
+
     is_controller_set = true;
 }
 
@@ -24,9 +37,27 @@ ControllerNavigation::~ControllerNavigation() {
 }
 
 void ControllerNavigation::UnloadController() {
+    // 1. If the app is already exiting, the controller memory is GONE.
+    // Touching the pointers will crash. Just reset and leave.
+    if (QCoreApplication::closingDown()) {
+        is_controller_set = false;
+        player1_controller = nullptr;
+        handheld_controller = nullptr;
+        return;
+    }
+
     if (is_controller_set) {
-        player1_controller->DeleteCallback(player1_callback_key);
-        handheld_controller->DeleteCallback(handheld_callback_key);
+        // 2. Only delete if the pointer exists AND the key is valid (>= 0)
+        if (player1_controller && player1_callback_key >= 0) {
+            player1_controller->DeleteCallback(player1_callback_key);
+            player1_callback_key = -1; // Prevent second deletion
+        }
+
+        if (handheld_controller && handheld_callback_key >= 0) {
+            handheld_controller->DeleteCallback(handheld_callback_key);
+            handheld_callback_key = -1; // Prevent second deletion
+        }
+
         is_controller_set = false;
     }
 }

src/citron/util/performance_overlay.cpp

@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 
 #include <QApplication>
+#include <QCoreApplication>
 #include <QPainter>
 #include <QPainterPath>
 #include <QScreen>
@@ -90,7 +91,9 @@ PerformanceOverlay::PerformanceOverlay(QWidget* parent) : QWidget(UISettings::Is
     UpdatePosition();
 }
 
-PerformanceOverlay::~PerformanceOverlay() = default;
+PerformanceOverlay::~PerformanceOverlay() {
+    update_timer.stop();
+}
 
 void PerformanceOverlay::SetVisible(bool visible) {
     is_enabled = visible;
@@ -167,7 +170,14 @@ void PerformanceOverlay::mouseReleaseEvent(QMouseEvent* event) {
 }
 
 void PerformanceOverlay::UpdatePerformanceStats() {
-    if (!main_window || !is_enabled) return;
+    // Stop the timer and hide if the app is closing
+    if (QCoreApplication::closingDown() || !main_window || main_window->isHidden()) {
+        update_timer.stop();
+        if (!this->isHidden()) this->hide();
+        return;
+    }
+
+    if (!is_enabled) return;
 
     if (UISettings::IsGamescope()) {
         bool ui_active = (QApplication::activePopupWidget() != nullptr);

src/citron/util/vram_overlay.cpp

@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 
 #include <QApplication>
+#include <QCoreApplication>
 #include <QPainter>
 #include <QPainterPath>
 #include <QScreen>
@@ -45,10 +46,10 @@ VramOverlay::VramOverlay(QWidget* parent) : QWidget(UISettings::IsGamescope() ?
 
     // Branching Typography and Sizing
     if (UISettings::IsGamescope()) {
-        title_font = QFont(QString::fromUtf8("Segoe UI"), 7, QFont::Bold);
-        value_font = QFont(QString::fromUtf8("Segoe UI"), 7, QFont::Medium);
-        small_font = QFont(QString::fromUtf8("Segoe UI"), 6, QFont::Normal);
-        warning_font = QFont(QString::fromUtf8("Segoe UI"), 8, QFont::Bold);
+        title_font = QFont(QString::fromUtf8("Segoe UI"), 8, QFont::Bold);
+        value_font = QFont(QString::fromUtf8("Segoe UI"), 8, QFont::Medium);
+        small_font = QFont(QString::fromUtf8("Segoe UI"), 7, QFont::Normal);
+        warning_font = QFont(QString::fromUtf8("Segoe UI"), 9, QFont::Bold);
         setMinimumSize(180, 140);
         resize(200, 160);
     } else {
@@ -81,7 +82,9 @@ VramOverlay::VramOverlay(QWidget* parent) : QWidget(UISettings::IsGamescope() ?
     UpdatePosition();
 }
 
-VramOverlay::~VramOverlay() = default;
+VramOverlay::~VramOverlay() {
+    update_timer.stop();
+}
 
 void VramOverlay::SetVisible(bool visible) {
     is_enabled = visible;
@@ -255,7 +258,14 @@ void VramOverlay::mouseReleaseEvent(QMouseEvent* event) {
 }
 
 void VramOverlay::UpdateVramStats() {
-    if (!main_window || !is_enabled) return;
+    // Stop the timer and hide if the app is closing
+    if (QCoreApplication::closingDown() || !main_window || main_window->isHidden()) {
+        update_timer.stop();
+        if (!this->isHidden()) this->hide();
+        return;
+    }
+
+    if (!is_enabled) return;
 
     if (UISettings::IsGamescope()) {
         bool ui_active = (QApplication::activePopupWidget() != nullptr);

src/hid_core/frontend/emulated_controller.cpp

@@ -1998,13 +1998,23 @@ int EmulatedController::SetCallback(ControllerUpdateCallback update_callback) {
 }
 
 void EmulatedController::DeleteCallback(int key) {
-    std::scoped_lock lock{callback_mutex};
-    const auto& iterator = callback_list.find(key);
-    if (iterator == callback_list.end()) {
-        LOG_ERROR(Input, "Tried to delete non-existent callback {}", key);
+    // 1. If the key is invalid, get out immediately.
+    if (key < 0) {
         return;
     }
-    callback_list.erase(iterator);
+
+    std::scoped_lock lock{callback_mutex};
+
+    // 2. Check if the list itself is valid.
+    if (callback_list.empty()) {
+        return;
+    }
+
+    // 3. Use a safe find. Do NOT use the result if it's the end of the map.
+    auto it = callback_list.find(key);
+    if (it != callback_list.end()) {
+        callback_list.erase(it);
+    }
 }
 
 void EmulatedController::StatusUpdate() {