Fix segfault when game is closed while card view window is open (#5507)

cockatrice/src/client/tabs/tab_game.cpp

@@ -189,14 +189,7 @@ void TabGame::emitUserEvent()
 
 TabGame::~TabGame()
 {
-    scene->clearViews();
-
     delete replay;
-
-    QMapIterator<int, Player *> i(players);
-    while (i.hasNext()) {
-        delete i.next().value();
-    }
 }
 
 void TabGame::updatePlayerListDockTitle()
@@ -1097,6 +1090,7 @@ void TabGame::eventLeave(const Event_Leave &event, int eventPlayerId, const Game
     players.remove(eventPlayerId);
     emit playerRemoved(player);
     player->clear();
+    scene->removePlayer(player);
     player->deleteLater();
 
     // Rearrange all remaining zones so that attachment relationship updates take place

cockatrice/src/game/cards/card_item.cpp

@@ -38,16 +38,9 @@ CardItem::CardItem(Player *_owner,
 
 CardItem::~CardItem()
 {
-    prepareDelete();
-
-    if (scene())
-        static_cast<GameScene *>(scene())->unregisterAnimationItem(this);
-
     delete cardMenu;
     delete ptMenu;
     delete moveMenu;
-
-    deleteDragItem();
 }
 
 void CardItem::prepareDelete()
@@ -74,6 +67,8 @@ void CardItem::prepareDelete()
 void CardItem::deleteLater()
 {
     prepareDelete();
+    if (scene())
+        static_cast<GameScene *>(scene())->unregisterAnimationItem(this);
     AbstractCardItem::deleteLater();
 }
 

cockatrice/src/game/game_scene.cpp

@@ -29,6 +29,13 @@ GameScene::GameScene(PhasesToolbar *_phasesToolbar, QObject *parent)
 GameScene::~GameScene()
 {
     delete animationTimer;
+
+    // DO NOT call clearViews() here
+    // clearViews calls close() on the zoneViews, which sends signals; sending signals in destructors leads to segfaults
+    // deleteLater() deletes the zoneView without allowing it to send signals
+    for (const auto &zoneView : zoneViews) {
+        zoneView->deleteLater();
+    }
 }
 
 void GameScene::retranslateUi()
@@ -76,7 +83,7 @@ void GameScene::rearrange()
     QListIterator<Player *> playersIter(players);
     while (playersIter.hasNext()) {
         Player *p = playersIter.next();
-        if (!p->getConceded()) {
+        if (p && !p->getConceded()) {
             playersPlaying.append(p);
             if (!firstPlayerFound && (p->getLocal())) {
                 firstPlayerIndex = playersPlaying.size() - 1;

cockatrice/src/game/player/player.cpp

@@ -564,9 +564,6 @@ Player::~Player()
 {
     qCDebug(PlayerLog) << "Player destructor:" << getName();
 
-    static_cast<GameScene *>(scene())->removePlayer(this);
-
-    clear();
     QMapIterator<QString, CardZone *> i(zones);
     while (i.hasNext())
         delete i.next().value();
@@ -2086,7 +2083,7 @@ void Player::eventShuffle(const Event_Shuffle &event)
             int length = view->getCards().length();
             // we want to close empty views as well
             if (length == 0 || length > absStart) { // note this assumes views always start at the top of the library
-                view->deleteLater();
+                view->close();
                 break;
             }
         } else {

cockatrice/src/game/zones/card_zone.cpp

@@ -40,17 +40,6 @@ CardZone::CardZone(Player *_p,
             &CardZone::refreshCardInfos);
 }
 
-CardZone::~CardZone()
-{
-    qCDebug(CardZoneLog) << "CardZone destructor: " << name;
-    for (auto *view : views) {
-        if (view != nullptr) {
-            view->deleteLater();
-        }
-    }
-    clearContents();
-}
-
 void CardZone::retranslateUi()
 {
     for (int i = 0; i < cards.size(); ++i)

cockatrice/src/game/zones/card_zone.h

@@ -67,7 +67,6 @@ public:
              bool _contentsKnown,
              QGraphicsItem *parent = nullptr,
              bool _isView = false);
-    ~CardZone();
     void retranslateUi();
     void clearContents();
     bool getHasCardAttr() const

cockatrice/src/game/zones/view_zone.cpp

@@ -40,13 +40,17 @@ ZoneViewZone::ZoneViewZone(Player *_p,
     }
 }
 
-ZoneViewZone::~ZoneViewZone()
+/**
+ * Deletes this ZoneView and removes it from the origZone's views.
+ * You should normally call this method instead of deleteLater()
+ */
+void ZoneViewZone::close()
 {
-    emit beingDeleted();
-    qDebug("ZoneViewZone destructor");
+    emit closed();
     if (!(revealZone && !writeableRevealZone)) {
         origZone->getViews().removeOne(this);
     }
+    deleteLater();
 }
 
 QRectF ZoneViewZone::boundingRect() const

cockatrice/src/game/zones/view_zone.h

@@ -65,7 +65,6 @@ public:
                  bool _writeableRevealZone = false,
                  QGraphicsItem *parent = nullptr,
                  bool _isReversed = false);
-    ~ZoneViewZone();
     QRectF boundingRect() const;
     void paint(QPainter *painter, const QStyleOptionGraphicsItem *option, QWidget *widget);
     void reorganizeCards();
@@ -94,13 +93,14 @@ public:
         return isReversed;
     }
 public slots:
+    void close();
     void setGroupBy(CardList::SortOption _groupBy);
     void setSortBy(CardList::SortOption _sortBy);
     void setPileView(int _pileView);
 private slots:
     void zoneDumpReceived(const Response &r);
 signals:
-    void beingDeleted();
+    void closed();
     void optimumRectChanged();
     void wheelEventReceived(QGraphicsSceneWheelEvent *event);
 

cockatrice/src/game/zones/view_zone_widget.cpp

@@ -133,7 +133,7 @@ ZoneViewWidget::ZoneViewWidget(Player *_player,
     setLayout(vbox);
 
     connect(zone, &ZoneViewZone::optimumRectChanged, this, &ZoneViewWidget::resizeToZoneContents);
-    connect(zone, &ZoneViewZone::beingDeleted, this, &ZoneViewWidget::zoneDeleted);
+    connect(zone, &ZoneViewZone::closed, this, &ZoneViewWidget::zoneDeleted);
     zone->initializeCards(cardList);
 
     // QLabel sizes aren't taken into account until the widget is rendered.
@@ -314,11 +314,12 @@ void ZoneViewWidget::handleScrollBarChange(int value)
 
 void ZoneViewWidget::closeEvent(QCloseEvent *event)
 {
-    disconnect(zone, &ZoneViewZone::beingDeleted, this, 0);
+    disconnect(zone, &ZoneViewZone::closed, this, 0);
+    // manually call zone->close in order to remove it from the origZones views
+    zone->close();
     if (shuffleCheckBox.isChecked())
         player->sendGameCommand(Command_Shuffle());
-    emit closePressed(this);
-    deleteLater();
+    zoneDeleted();
     event->accept();
 }