From d9745012776211fed1089e3d29061124d022a07b Mon Sep 17 00:00:00 2001
From: BruebachL <44814898+BruebachL@users.noreply.github.com>
Date: Tue, 1 Sep 2026 11:31:11 +0200
Subject: [PATCH] [GameScene] Sever connections properly. (#7191)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* [GameScene] Sever connections properly.

Took 2 minutes


Took 54 minutes

* [GameScene] Sever animated item destroy connections at teardown

Fix crash when a replay's board is closed (GameScene teardown abort).

The old QObject::disconnect(nullptr, nullptr, this, nullptr) is invalid per
Qt docs (the sender must never be nullptr), so it never severed the PMF
destroyed -> GameScene::removeAnimatedItem connections that fire when
QGraphicsScene::~QGraphicsScene -> clear() destroys the remaining items.

Store the QMetaObject::Connection handle for each animated item and
disconnect them all in ~GameScene via the connection-handle overload.
Dedup connections on the connection map rather than animatedItems, since
the animation timer clears animatedItems on completion, which let a
re-registered item (e.g. a life counter flashed repeatedly) accumulate
orphaned duplicate destroyed connections that survived teardown.

---------

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
---
 cockatrice/src/game_graphics/game_scene.cpp | 27 +++++++++++++++------
 cockatrice/src/game_graphics/game_scene.h   |  8 +++---
 2 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/cockatrice/src/game_graphics/game_scene.cpp b/cockatrice/src/game_graphics/game_scene.cpp
index 87af4c73c..457f1b3f7 100644
--- a/cockatrice/src/game_graphics/game_scene.cpp
+++ b/cockatrice/src/game_graphics/game_scene.cpp
@@ -44,11 +44,16 @@ GameScene::GameScene(PhasesToolbar *_phasesToolbar, QObject *parent)
 
 GameScene::~GameScene()
 {
-    // Sever all incoming connections (animated item destroy-tracking) before the
-    // members below are destroyed: the base QGraphicsScene destructor destroys the
-    // remaining items, and their destroyed() signals must not reach slots that
-    // reference members that no longer exist.
-    QObject::disconnect(nullptr, nullptr, this, nullptr);
+    // Sever all destroyed->removeAnimatedItem connections before the members below
+    // are destroyed: the base QGraphicsScene destructor destroys the remaining items,
+    // and their destroyed() signals must not reach slots that reference members that
+    // no longer exist. The connection handle overload is used because the string-based
+    // disconnect(nullptr, nullptr, this, nullptr) is invalid (the sender must never be
+    // nullptr) and would otherwise fail to sever these pointer-to-member connections.
+    for (auto it = animationItemConnections.constBegin(); it != animationItemConnections.constEnd(); ++it) {
+        QObject::disconnect(*it);
+    }
+    animationItemConnections.clear();
 
     delete animationTimer;
     animationTimer = nullptr;
@@ -777,8 +782,15 @@ void GameScene::registerAnimationItem(IAnimatedItem *item)
     if (!object) {
         return;
     }
-    if (!animatedItems.contains(object)) {
-        connect(object, &QObject::destroyed, this, &GameScene::removeAnimatedItem);
+    // Guard against duplicate connections using the connection map, not
+    // animatedItems: the animation timer removes entries from animatedItems when an
+    // animation completes, but the destroyed->removeAnimatedItem connection must
+    // persist until the object is destroyed. Relying on animatedItems here would let
+    // a re-registered item (e.g. a life counter that flashes repeatedly) accumulate
+    // duplicate destroyed connections, the older ones of which would survive teardown.
+    if (!animationItemConnections.contains(object)) {
+        animationItemConnections.insert(object,
+                                        connect(object, &QObject::destroyed, this, &GameScene::removeAnimatedItem));
     }
     animatedItems.insert(object, item);
     if (animationTimer && !animationTimer->isActive()) {
@@ -797,6 +809,7 @@ void GameScene::unregisterAnimationItem(IAnimatedItem *item)
 void GameScene::removeAnimatedItem(QObject *item)
 {
     animatedItems.remove(item);
+    animationItemConnections.remove(item);
     if (animationTimer && animatedItems.isEmpty()) {
         animationTimer->stop();
     }
diff --git a/cockatrice/src/game_graphics/game_scene.h b/cockatrice/src/game_graphics/game_scene.h
index c12696189..859d7a6eb 100644
--- a/cockatrice/src/game_graphics/game_scene.h
+++ b/cockatrice/src/game_graphics/game_scene.h
@@ -54,9 +54,11 @@ private:
     QPointer<CardItem> hoveredCard;                     ///< Currently hovered card
     QBasicTimer *animationTimer;                        ///< Timer for scene animations
     QHash<QObject *, IAnimatedItem *> animatedItems;    ///< Items currently animating
-    int playerRotation;                                 ///< Rotation offset for player layout
-    bool rearranging = false;                           ///< Guard against re-entrant rearrange
-    bool needsReArrange = false;                        ///< Pending rearrange requested during a pass
+    QHash<QObject *, QMetaObject::Connection>
+        animationItemConnections; ///< destroyed->removeAnimatedItem handles per animated item
+    int playerRotation;           ///< Rotation offset for player layout
+    bool rearranging = false;     ///< Guard against re-entrant rearrange
+    bool needsReArrange = false;  ///< Pending rearrange requested during a pass
 
     /**
      * @brief Updates which card is currently hovered based on scene coordinates.