diff --git a/libcockatrice_card/libcockatrice/card/card_info.cpp b/libcockatrice_card/libcockatrice/card/card_info.cpp index c90027442..388c9ab57 100644 --- a/libcockatrice_card/libcockatrice/card/card_info.cpp +++ b/libcockatrice_card/libcockatrice/card/card_info.cpp @@ -270,6 +270,13 @@ void CardInfo::resetReverseRelatedCards2Me() reverseRelatedCardsToMe = QList(); } +void CardInfo::clearDirectRelations() +{ + relatedCards = QList(); + reverseRelatedCards = QList(); + reverseRelatedCardsToMe = QList(); +} + // Back-compatibility methods. Remove ASAP QString CardInfo::getCardType() const { diff --git a/libcockatrice_card/libcockatrice/card/card_info.h b/libcockatrice_card/libcockatrice/card/card_info.h index 77ddce5ce..eb1250a1c 100644 --- a/libcockatrice_card/libcockatrice/card/card_info.h +++ b/libcockatrice_card/libcockatrice/card/card_info.h @@ -453,6 +453,14 @@ public: return result; } void resetReverseRelatedCards2Me(); + /** + * @brief Clears the raw relation lists without deleting the CardRelation objects. + * + * Callers that schedule the relations for deletion (e.g. CardDatabase::removeCard()) + * must use this to drop the now-dangling pointers from this CardInfo, which may + * outlive them through external shared pointers. + */ + void clearDirectRelations(); void addReverseRelatedCards2Me(CardRelation *cardRelation) { reverseRelatedCardsToMe.append(cardRelation); diff --git a/libcockatrice_card/libcockatrice/card/database/card_database.cpp b/libcockatrice_card/libcockatrice/card/database/card_database.cpp index 7b0d5b2f9..946412606 100644 --- a/libcockatrice_card/libcockatrice/card/database/card_database.cpp +++ b/libcockatrice_card/libcockatrice/card/database/card_database.cpp @@ -168,6 +168,10 @@ void CardDatabase::removeCard(const CardInfoPtr &card) cardRelation->deleteLater(); } + // The relations are only scheduled for deletion; the CardInfo may outlive + // them through an external shared pointer, so drop the raw pointers now. + card->clearDirectRelations(); + QMutexLocker locker(removeCardMutex); cards.remove(card->getName()); simpleNameCards.remove(card->getSimpleName()); @@ -271,6 +275,9 @@ void CardDatabase::swapInDatabaseData(CardDatabaseData data) for (auto *rel : card->getReverseRelatedCards2Me()) { rel->deleteLater(); } + // Replacement cards take over the database; old CardInfo objects may + // still be referenced by the UI, so clear their dangling relations. + card->clearDirectRelations(); } cards = std::move(data.cards); diff --git a/tests/carddatabase/carddatabase_test.cpp b/tests/carddatabase/carddatabase_test.cpp index 1b68b8166..eb522434d 100644 --- a/tests/carddatabase/carddatabase_test.cpp +++ b/tests/carddatabase/carddatabase_test.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -124,6 +125,32 @@ TEST(CardDatabaseTest, FindPrintingWithIdOnNullCardInfoReturnsEmpty) EXPECT_TRUE(db.query()->findPrintingWithId(nullptr, "card_Not a Card_fixture").isEmpty()); } +TEST(CardInfoTest, ClearDirectRelationsDropsRawPointers) +{ + auto *forward = new CardRelation("Child"); + auto *reverse = new CardRelation("Parent"); + auto *toMe = new CardRelation("Child"); + + auto card = CardInfo::newInstance("Parent", "", false, QHash(), {forward}, {reverse}, + SetToPrintingsMap(), CardInfo::UiAttributes()); + card->addReverseRelatedCards2Me(toMe); + ASSERT_EQ(1, card->getRelatedCards().size()); + ASSERT_EQ(1, card->getReverseRelatedCards().size()); + ASSERT_EQ(1, card->getReverseRelatedCards2Me().size()); + ASSERT_EQ(2, card->getAllRelatedCards().size()); + + card->clearDirectRelations(); + + EXPECT_TRUE(card->getRelatedCards().isEmpty()); + EXPECT_TRUE(card->getReverseRelatedCards().isEmpty()); + EXPECT_TRUE(card->getReverseRelatedCards2Me().isEmpty()); + EXPECT_TRUE(card->getAllRelatedCards().isEmpty()); + + delete forward; + delete reverse; + delete toMe; +} + TEST(CardDatabaseTest, Xml4LocalizedDataRoundTrip) { NoopCardSetPriorityController controller;