From 7347ba88ac679a30da7b36cfba4f40b96e9de37c Mon Sep 17 00:00:00 2001 From: RickyRister <42636155+RickyRister@users.noreply.github.com> Date: Sat, 11 Jan 2025 19:19:45 -0800 Subject: [PATCH] fix segfault on disconnect (#5447) * add new param to closeRequest * don't emit signals in dtors * send closeRequest * fix build failure * fix build failure * see if we can get away with the overloaded triggered * fix build failure --- cockatrice/src/client/tabs/tab.h | 8 ++++- .../src/client/tabs/tab_deck_editor.cpp | 17 +++++----- cockatrice/src/client/tabs/tab_deck_editor.h | 3 +- cockatrice/src/client/tabs/tab_game.cpp | 32 ++++++++++++------- cockatrice/src/client/tabs/tab_game.h | 4 +-- cockatrice/src/client/tabs/tab_message.cpp | 13 +++----- cockatrice/src/client/tabs/tab_message.h | 3 +- cockatrice/src/client/tabs/tab_room.cpp | 19 +++-------- cockatrice/src/client/tabs/tab_room.h | 4 +-- cockatrice/src/client/tabs/tab_supervisor.cpp | 19 +++++------ .../tab_deck_storage_visual.cpp | 2 +- .../tab_deck_storage_visual.h | 2 +- 12 files changed, 59 insertions(+), 67 deletions(-) diff --git a/cockatrice/src/client/tabs/tab.h b/cockatrice/src/client/tabs/tab.h index 3b5d89bb1..b046d5c7a 100644 --- a/cockatrice/src/client/tabs/tab.h +++ b/cockatrice/src/client/tabs/tab.h @@ -50,7 +50,13 @@ public: } virtual QString getTabText() const = 0; virtual void retranslateUi() = 0; - virtual void closeRequest() + /** + * Sends a request to close the tab. + * Signals for cleanup should be emitted from this method instead of the destructor. + * + * @param forced whether this close request was initiated by the user or forced by the server. + */ + virtual void closeRequest(bool /*forced*/ = false) { } virtual void tabActivated() diff --git a/cockatrice/src/client/tabs/tab_deck_editor.cpp b/cockatrice/src/client/tabs/tab_deck_editor.cpp index f9a7632dc..d88c81b02 100644 --- a/cockatrice/src/client/tabs/tab_deck_editor.cpp +++ b/cockatrice/src/client/tabs/tab_deck_editor.cpp @@ -358,7 +358,7 @@ void TabDeckEditor::createMenus() analyzeDeckMenu->addAction(aAnalyzeDeckTappedout); aClose = new QAction(QString(), this); - connect(aClose, SIGNAL(triggered()), this, SLOT(closeRequest())); + connect(aClose, &QAction::triggered, this, [this] { closeRequest(); }); aClearFilterAll = new QAction(QString(), this); aClearFilterAll->setIcon(QPixmap("theme:icons/clearsearch")); @@ -721,11 +721,6 @@ TabDeckEditor::TabDeckEditor(TabSupervisor *_tabSupervisor, QWidget *parent) loadLayout(); } -TabDeckEditor::~TabDeckEditor() -{ - emit deckEditorClosing(this); -} - void TabDeckEditor::retranslateUi() { cardInfo->retranslateUi(); @@ -980,10 +975,14 @@ bool TabDeckEditor::confirmClose() return true; } -void TabDeckEditor::closeRequest() +void TabDeckEditor::closeRequest(bool forced) { - if (confirmClose()) - deleteLater(); + if (!forced && !confirmClose()) { + return; + } + + emit deckEditorClosing(this); + deleteLater(); } void TabDeckEditor::actNewDeck() diff --git a/cockatrice/src/client/tabs/tab_deck_editor.h b/cockatrice/src/client/tabs/tab_deck_editor.h index dec652d4c..7a3859413 100644 --- a/cockatrice/src/client/tabs/tab_deck_editor.h +++ b/cockatrice/src/client/tabs/tab_deck_editor.h @@ -164,7 +164,6 @@ private: public: explicit TabDeckEditor(TabSupervisor *_tabSupervisor, QWidget *parent = nullptr); - ~TabDeckEditor() override; void retranslateUi() override; QString getTabText() const override; void setDeck(DeckLoader *_deckLoader); @@ -179,7 +178,7 @@ public: void updateCardInfo(CardInfoPtr _card); public slots: - void closeRequest() override; + void closeRequest(bool forced = false) override; void showPrintingSelector(); signals: void openDeckEditor(const DeckLoader *deckLoader); diff --git a/cockatrice/src/client/tabs/tab_game.cpp b/cockatrice/src/client/tabs/tab_game.cpp index 1bc73eb5c..bd37f4bcc 100644 --- a/cockatrice/src/client/tabs/tab_game.cpp +++ b/cockatrice/src/client/tabs/tab_game.cpp @@ -581,16 +581,14 @@ void TabGame::emitUserEvent() TabGame::~TabGame() { + scene->clearViews(); + delete replay; QMapIterator i(players); while (i.hasNext()) { delete i.next().value(); } - - players.clear(); - - emit gameClosing(this); } void TabGame::updatePlayerListDockTitle() @@ -701,9 +699,15 @@ void TabGame::retranslateUi() scene->retranslateUi(); } -void TabGame::closeRequest() +void TabGame::closeRequest(bool forced) { - actLeaveGame(); + if (!forced && !leaveGame()) { + return; + } + + emit gameClosing(this); + + deleteLater(); } void TabGame::replayNextEvent(Player::EventProcessingOptions options) @@ -794,19 +798,23 @@ void TabGame::actConcede() } } -void TabGame::actLeaveGame() +/** + * Confirms the leave game and sends the leave game command, if applicable. + * + * @return True if the leave game is confirmed + */ +bool TabGame::leaveGame() { if (!gameClosed) { if (!spectator) if (QMessageBox::question(this, tr("Leave game"), tr("Are you sure you want to leave this game?"), QMessageBox::Yes | QMessageBox::No, QMessageBox::No) != QMessageBox::Yes) - return; + return false; if (!replay) sendGameCommand(Command_LeaveGame()); } - scene->clearViews(); - deleteLater(); + return true; } void TabGame::actSay() @@ -1598,7 +1606,7 @@ void TabGame::createMenuItems() aConcede = new QAction(this); connect(aConcede, SIGNAL(triggered()), this, SLOT(actConcede())); aLeaveGame = new QAction(this); - connect(aLeaveGame, SIGNAL(triggered()), this, SLOT(actLeaveGame())); + connect(aLeaveGame, &QAction::triggered, this, [this] { closeRequest(); }); aFocusChat = new QAction(this); connect(aFocusChat, SIGNAL(triggered()), sayEdit, SLOT(setFocus())); aCloseReplay = nullptr; @@ -1648,7 +1656,7 @@ void TabGame::createReplayMenuItems() aFocusChat = nullptr; aLeaveGame = nullptr; aCloseReplay = new QAction(this); - connect(aCloseReplay, SIGNAL(triggered()), this, SLOT(actLeaveGame())); + connect(aCloseReplay, &QAction::triggered, this, [this] { closeRequest(); }); phasesMenu = nullptr; gameMenu = new QMenu(this); diff --git a/cockatrice/src/client/tabs/tab_game.h b/cockatrice/src/client/tabs/tab_game.h index 5de957474..ec3366dda 100644 --- a/cockatrice/src/client/tabs/tab_game.h +++ b/cockatrice/src/client/tabs/tab_game.h @@ -189,6 +189,7 @@ private: void startGame(bool resuming); void stopGame(); void closeGame(); + bool leaveGame(); void eventSpectatorSay(const Event_GameSay &event, int eventPlayerId, const GameEventContext &context); void eventSpectatorLeave(const Event_Leave &event, int eventPlayerId, const GameEventContext &context); @@ -242,7 +243,6 @@ private slots: void actGameInfo(); void actConcede(); - void actLeaveGame(); void actRemoveLocalArrows(); void actRotateViewCW(); void actRotateViewCCW(); @@ -278,7 +278,7 @@ public: ~TabGame() override; void retranslateUi() override; void updatePlayerListDockTitle(); - void closeRequest() override; + void closeRequest(bool forced = false) override; const QMap &getPlayers() const { return players; diff --git a/cockatrice/src/client/tabs/tab_message.cpp b/cockatrice/src/client/tabs/tab_message.cpp index b39126083..7aa2d2a27 100644 --- a/cockatrice/src/client/tabs/tab_message.cpp +++ b/cockatrice/src/client/tabs/tab_message.cpp @@ -38,7 +38,7 @@ TabMessage::TabMessage(TabSupervisor *_tabSupervisor, vbox->addWidget(sayEdit); aLeave = new QAction(this); - connect(aLeave, SIGNAL(triggered()), this, SLOT(actLeave())); + connect(aLeave, &QAction::triggered, this, [this] { closeRequest(); }); messageMenu = new QMenu(this); messageMenu->addAction(aLeave); @@ -53,7 +53,6 @@ TabMessage::TabMessage(TabSupervisor *_tabSupervisor, TabMessage::~TabMessage() { - emit talkClosing(this); delete ownUserInfo; delete otherUserInfo; } @@ -86,9 +85,10 @@ QString TabMessage::getTabText() const return tr("%1 - Private chat").arg(QString::fromStdString(otherUserInfo->name())); } -void TabMessage::closeRequest() +void TabMessage::closeRequest(bool /*forced*/) { - actLeave(); + emit talkClosing(this); + deleteLater(); } void TabMessage::sendMessage() @@ -114,11 +114,6 @@ void TabMessage::messageSent(const Response &response) "This user is ignoring you, they cannot see your messages in main chat and you cannot join their games.")); } -void TabMessage::actLeave() -{ - deleteLater(); -} - void TabMessage::processUserMessageEvent(const Event_UserMessage &event) { auto userInfo = event.sender_name() == otherUserInfo->name() ? otherUserInfo : ownUserInfo; diff --git a/cockatrice/src/client/tabs/tab_message.h b/cockatrice/src/client/tabs/tab_message.h index ede73d497..12a4188a2 100644 --- a/cockatrice/src/client/tabs/tab_message.h +++ b/cockatrice/src/client/tabs/tab_message.h @@ -29,7 +29,6 @@ signals: void maximizeClient(); private slots: void sendMessage(); - void actLeave(); void messageSent(const Response &response); void addMentionTag(QString mentionTag); void messageClicked(); @@ -41,7 +40,7 @@ public: const ServerInfo_User &_otherUserInfo); ~TabMessage() override; void retranslateUi() override; - void closeRequest() override; + void closeRequest(bool forced = false) override; void tabActivated() override; QString getUserName() const; QString getTabText() const override; diff --git a/cockatrice/src/client/tabs/tab_room.cpp b/cockatrice/src/client/tabs/tab_room.cpp index a76b9851a..810041244 100644 --- a/cockatrice/src/client/tabs/tab_room.cpp +++ b/cockatrice/src/client/tabs/tab_room.cpp @@ -101,7 +101,7 @@ TabRoom::TabRoom(TabSupervisor *_tabSupervisor, hbox->addWidget(userList, 1); aLeaveRoom = new QAction(this); - connect(aLeaveRoom, SIGNAL(triggered()), this, SLOT(actLeaveRoom())); + connect(aLeaveRoom, &QAction::triggered, this, [this] { closeRequest(); }); roomMenu = new QMenu(this); roomMenu->addAction(aLeaveRoom); @@ -135,11 +135,6 @@ TabRoom::TabRoom(TabSupervisor *_tabSupervisor, setCentralWidget(mainWidget); } -TabRoom::~TabRoom() -{ - emit roomClosing(this); -} - void TabRoom::retranslateUi() { gameSelector->retranslateUi(); @@ -175,9 +170,11 @@ void TabRoom::actShowPopup(const QString &message) } } -void TabRoom::closeRequest() +void TabRoom::closeRequest(bool /*forced*/) { - actLeaveRoom(); + sendRoomCommand(prepareRoomCommand(Command_LeaveRoom())); + emit roomClosing(this); + deleteLater(); } void TabRoom::tabActivated() @@ -216,12 +213,6 @@ void TabRoom::sayFinished(const Response &response) chatView->appendMessage(tr("You are flooding the chat. Please wait a couple of seconds.")); } -void TabRoom::actLeaveRoom() -{ - sendRoomCommand(prepareRoomCommand(Command_LeaveRoom())); - deleteLater(); -} - void TabRoom::actClearChat() { chatView->clearChat(); diff --git a/cockatrice/src/client/tabs/tab_room.h b/cockatrice/src/client/tabs/tab_room.h index a172d8277..32152e933 100644 --- a/cockatrice/src/client/tabs/tab_room.h +++ b/cockatrice/src/client/tabs/tab_room.h @@ -70,7 +70,6 @@ signals: private slots: void sendMessage(); void sayFinished(const Response &response); - void actLeaveRoom(); void actClearChat(); void actOpenChatSettings(); void addMentionTag(QString mentionTag); @@ -91,9 +90,8 @@ public: AbstractClient *_client, ServerInfo_User *_ownUser, const ServerInfo_Room &info); - ~TabRoom() override; void retranslateUi() override; - void closeRequest() override; + void closeRequest(bool forced = false) override; void tabActivated() override; void processRoomEvent(const RoomEvent &event); int getRoomId() const diff --git a/cockatrice/src/client/tabs/tab_supervisor.cpp b/cockatrice/src/client/tabs/tab_supervisor.cpp index 27320218b..03611c1cf 100644 --- a/cockatrice/src/client/tabs/tab_supervisor.cpp +++ b/cockatrice/src/client/tabs/tab_supervisor.cpp @@ -304,30 +304,27 @@ void TabSupervisor::stop() tabAdmin = 0; tabLog = 0; - for (const auto tab : deckEditorTabs) { - disconnect(tab, nullptr, this, nullptr); - tab->deleteLater(); - } - deckEditorTabs.clear(); + QList tabsToDelete; for (auto i = roomTabs.cbegin(), end = roomTabs.cend(); i != end; ++i) { - disconnect(i.value(), nullptr, this, nullptr); - i.value()->deleteLater(); + tabsToDelete << i.value(); } roomTabs.clear(); for (auto i = gameTabs.cbegin(), end = gameTabs.cend(); i != end; ++i) { - disconnect(i.value(), nullptr, this, nullptr); - i.value()->deleteLater(); + tabsToDelete << i.value(); } gameTabs.clear(); for (const auto tab : replayTabs) { - disconnect(tab, nullptr, this, nullptr); - tab->deleteLater(); + tabsToDelete << tab; } replayTabs.clear(); + for (const auto tab : tabsToDelete) { + tab->closeRequest(true); + } + delete userInfo; userInfo = 0; } diff --git a/cockatrice/src/client/tabs/visual_deck_storage/tab_deck_storage_visual.cpp b/cockatrice/src/client/tabs/visual_deck_storage/tab_deck_storage_visual.cpp index 831e7ed97..7858e4e45 100644 --- a/cockatrice/src/client/tabs/visual_deck_storage/tab_deck_storage_visual.cpp +++ b/cockatrice/src/client/tabs/visual_deck_storage/tab_deck_storage_visual.cpp @@ -58,7 +58,7 @@ TabDeckStorageVisual::TabDeckStorageVisual(TabSupervisor *_tabSupervisor, Abstra retranslateUi(); } -void TabDeckStorageVisual::closeRequest() +void TabDeckStorageVisual::closeRequest(bool /*forced*/) { this->close(); } diff --git a/cockatrice/src/client/tabs/visual_deck_storage/tab_deck_storage_visual.h b/cockatrice/src/client/tabs/visual_deck_storage/tab_deck_storage_visual.h index 4d8538826..689061be9 100644 --- a/cockatrice/src/client/tabs/visual_deck_storage/tab_deck_storage_visual.h +++ b/cockatrice/src/client/tabs/visual_deck_storage/tab_deck_storage_visual.h @@ -33,7 +33,7 @@ public: } public slots: void cardUpdateFinished(int exitCode, QProcess::ExitStatus exitStatus); - void closeRequest() override; + void closeRequest(bool forced = false) override; void actOpenLocalDeck(QMouseEvent *event, DeckPreviewCardPictureWidget *instance); void actDeleteLocalDeck(); signals: