From e1964f21de17d648f9e50bf98c55fa6c338df994 Mon Sep 17 00:00:00 2001 From: RickyRister <42636155+RickyRister@users.noreply.github.com> Date: Sun, 2 Mar 2025 15:57:30 -0800 Subject: [PATCH] Fix memory leaks from DeckLoader usage (#5665) * add comment * stack allocate DeckLoader for loading tags * deckModel now takes ownership of DeckLoader * fix remaining * add comment --- cockatrice/src/client/tabs/abstract_tab_deck_editor.cpp | 7 ++++++- cockatrice/src/client/tabs/tab_supervisor.cpp | 4 ++++ .../widgets/deck_editor/deck_editor_deck_dock_widget.cpp | 4 ++++ .../deck_preview_deck_tags_display_widget.cpp | 6 +++--- cockatrice/src/deck/deck_list_model.cpp | 8 ++++++-- cockatrice/src/dialogs/dlg_load_deck_from_clipboard.cpp | 4 ++-- cockatrice/src/dialogs/dlg_load_deck_from_clipboard.h | 8 ++++++++ 7 files changed, 33 insertions(+), 8 deletions(-) diff --git a/cockatrice/src/client/tabs/abstract_tab_deck_editor.cpp b/cockatrice/src/client/tabs/abstract_tab_deck_editor.cpp index 921f5d579..98f38bb23 100644 --- a/cockatrice/src/client/tabs/abstract_tab_deck_editor.cpp +++ b/cockatrice/src/client/tabs/abstract_tab_deck_editor.cpp @@ -128,6 +128,10 @@ void AbstractTabDeckEditor::actSwapCard(CardInfoPtr info, QString zoneName) deckDockWidget->deckModel->findCard(info->getName(), zoneName, providerId, collectorNumber)); } +/** + * Sets the currently active deck for this tab + * @param _deck The deck. Takes ownership of the object + */ void AbstractTabDeckEditor::setDeck(DeckLoader *_deck) { deckDockWidget->setDeck(_deck); @@ -289,12 +293,13 @@ void AbstractTabDeckEditor::openDeckFromFile(const QString &fileName, DeckOpenLo SettingsCache::instance().recents().updateRecentlyOpenedDeckPaths(fileName); if (deckOpenLocation == NEW_TAB) { emit openDeckEditor(l); + l->deleteLater(); } else { deckMenu->setSaveStatus(false); setDeck(l); } } else { - delete l; + l->deleteLater(); QMessageBox::critical(this, tr("Error"), tr("Could not open deck at %1").arg(fileName)); } deckMenu->setSaveStatus(true); diff --git a/cockatrice/src/client/tabs/tab_supervisor.cpp b/cockatrice/src/client/tabs/tab_supervisor.cpp index ac908de3a..5b89ab257 100644 --- a/cockatrice/src/client/tabs/tab_supervisor.cpp +++ b/cockatrice/src/client/tabs/tab_supervisor.cpp @@ -764,6 +764,10 @@ void TabSupervisor::talkLeft(TabMessage *tab) removeTab(indexOf(tab)); } +/** + * Creates a new deck editor tab + * @param deckToOpen The deck to open in the tab. Creates a copy of the DeckLoader instance. + */ TabDeckEditor *TabSupervisor::addDeckEditorTab(const DeckLoader *deckToOpen) { auto *tab = new TabDeckEditor(this); diff --git a/cockatrice/src/client/ui/widgets/deck_editor/deck_editor_deck_dock_widget.cpp b/cockatrice/src/client/ui/widgets/deck_editor/deck_editor_deck_dock_widget.cpp index 3aaa43978..1d6ac86bb 100644 --- a/cockatrice/src/client/ui/widgets/deck_editor/deck_editor_deck_dock_widget.cpp +++ b/cockatrice/src/client/ui/widgets/deck_editor/deck_editor_deck_dock_widget.cpp @@ -289,6 +289,10 @@ void DeckEditorDeckDockWidget::setBannerCard(int /* changedIndex */) emit deckChanged(); } +/** + * Sets the currently active deck for this tab + * @param _deck The deck. Takes ownership of the object + */ void DeckEditorDeckDockWidget::setDeck(DeckLoader *_deck) { deckModel->setDeckList(_deck); diff --git a/cockatrice/src/client/ui/widgets/visual_deck_storage/deck_preview/deck_preview_deck_tags_display_widget.cpp b/cockatrice/src/client/ui/widgets/visual_deck_storage/deck_preview/deck_preview_deck_tags_display_widget.cpp index fd5e34c4a..60b4d5b26 100644 --- a/cockatrice/src/client/ui/widgets/visual_deck_storage/deck_preview/deck_preview_deck_tags_display_widget.cpp +++ b/cockatrice/src/client/ui/widgets/visual_deck_storage/deck_preview/deck_preview_deck_tags_display_widget.cpp @@ -143,10 +143,10 @@ void DeckPreviewDeckTagsDisplayWidget::openTagEditDlg() auto *deckEditor = qobject_cast(currentParent); QStringList knownTags; QStringList allFiles = getAllFiles(SettingsCache::instance().getDeckPath()); - auto *loader = new DeckLoader(); + DeckLoader loader; for (const QString &file : allFiles) { - loader->loadFromFile(file, DeckLoader::getFormatFromName(file), false); - QStringList tags = loader->getTags(); + loader.loadFromFile(file, DeckLoader::getFormatFromName(file), false); + QStringList tags = loader.getTags(); knownTags.append(tags); knownTags.removeDuplicates(); } diff --git a/cockatrice/src/deck/deck_list_model.cpp b/cockatrice/src/deck/deck_list_model.cpp index e66633764..cd3c9bcf1 100644 --- a/cockatrice/src/deck/deck_list_model.cpp +++ b/cockatrice/src/deck/deck_list_model.cpp @@ -18,6 +18,7 @@ DeckListModel::DeckListModel(QObject *parent) : QAbstractItemModel(parent), lastKnownColumn(1), lastKnownOrder(Qt::AscendingOrder) { deckList = new DeckLoader; + deckList->setParent(this); connect(deckList, &DeckLoader::deckLoaded, this, &DeckListModel::rebuildTree); connect(deckList, &DeckLoader::deckHashChanged, this, &DeckListModel::deckHashChanged); root = new InnerDecklistNode; @@ -26,7 +27,6 @@ DeckListModel::DeckListModel(QObject *parent) DeckListModel::~DeckListModel() { delete root; - delete deckList; } void DeckListModel::rebuildTree() @@ -470,10 +470,14 @@ void DeckListModel::cleanList() setDeckList(new DeckLoader); } +/** + * @param _deck The deck. Takes ownership of the object + */ void DeckListModel::setDeckList(DeckLoader *_deck) { - delete deckList; + deckList->deleteLater(); deckList = _deck; + deckList->setParent(this); connect(deckList, &DeckLoader::deckLoaded, this, &DeckListModel::rebuildTree); connect(deckList, &DeckLoader::deckHashChanged, this, &DeckListModel::deckHashChanged); rebuildTree(); diff --git a/cockatrice/src/dialogs/dlg_load_deck_from_clipboard.cpp b/cockatrice/src/dialogs/dlg_load_deck_from_clipboard.cpp index e5e5b5951..47c1af242 100644 --- a/cockatrice/src/dialogs/dlg_load_deck_from_clipboard.cpp +++ b/cockatrice/src/dialogs/dlg_load_deck_from_clipboard.cpp @@ -63,13 +63,14 @@ void DlgLoadDeckFromClipboard::actOK() QTextStream stream(&buffer); auto *deckLoader = new DeckLoader; + deckLoader->setParent(this); + if (buffer.contains("")) { if (deckLoader->loadFromString_Native(buffer)) { deckList = deckLoader; accept(); } else { QMessageBox::critical(this, tr("Error"), tr("Invalid deck list.")); - delete deckLoader; } } else if (deckLoader->loadFromStream_Plain(stream)) { deckList = deckLoader; @@ -81,7 +82,6 @@ void DlgLoadDeckFromClipboard::actOK() accept(); } else { QMessageBox::critical(this, tr("Error"), tr("Invalid deck list.")); - delete deckLoader; } } diff --git a/cockatrice/src/dialogs/dlg_load_deck_from_clipboard.h b/cockatrice/src/dialogs/dlg_load_deck_from_clipboard.h index 337f8c7d3..19d8cfff8 100644 --- a/cockatrice/src/dialogs/dlg_load_deck_from_clipboard.h +++ b/cockatrice/src/dialogs/dlg_load_deck_from_clipboard.h @@ -24,6 +24,14 @@ private: public: explicit DlgLoadDeckFromClipboard(QWidget *parent = nullptr); + + /** + * Gets the loaded deck. Only call this method after this dialog window has been successfully exec'd. + * + * The returned DeckLoader is parented to this object; make sure to take ownership of the DeckLoader if you intend + * to use it, since otherwise it will get destroyed once this dlg is destroyed + * @return The DeckLoader + */ DeckLoader *getDeckList() const { return deckList;