diff --git a/src/engraving/dom/score.cpp b/src/engraving/dom/score.cpp index a8399daeba8bd..53add5cb0e3ce 100644 --- a/src/engraving/dom/score.cpp +++ b/src/engraving/dom/score.cpp @@ -376,8 +376,20 @@ void Score::onElementDestruction(EngravingItem* e) { Score* score = e->EngravingObject::score(); - if (!score || Score::validScores.find(score) == Score::validScores.end()) { - // No score or the score is already deleted + if (!score) { + return; + } + + // The EID register lives on the master score, so unregister the item even when + // its own score is being deleted (e.g. when an excerpt is removed), as long as + // the master score, and with it the register, is still alive + MasterScore* masterScore = score->masterScore(); + if (masterScore && Score::validScores.find(masterScore) != Score::validScores.end()) { + masterScore->eidRegister()->onItemDestroyed(e); + } + + if (Score::validScores.find(score) == Score::validScores.end()) { + // The score is already deleted return; } diff --git a/src/engraving/infrastructure/eidregister.cpp b/src/engraving/infrastructure/eidregister.cpp index f16fe608298d0..dc5f954736eb3 100644 --- a/src/engraving/infrastructure/eidregister.cpp +++ b/src/engraving/infrastructure/eidregister.cpp @@ -80,6 +80,22 @@ void EIDRegister::removeItem(const EngravingObject* item) m_eidToItem.erase(eidIter); } +void EIDRegister::onItemDestroyed(const EngravingObject* item) +{ + std::lock_guard lock(m_mutex); + // NOTE: most items never get an EID assigned, so a missing entry is the normal case. + // Items that do have one must be unregistered here: a later allocation can reuse + // the freed address, and a stale entry would corrupt the register. + + auto itemIter = m_itemToEid.find(const_cast(item)); + if (itemIter == m_itemToEid.end()) { + return; + } + + m_eidToItem.erase(itemIter->second); + m_itemToEid.erase(itemIter); +} + EngravingObject* EIDRegister::itemFromEID(const EID& eid) const { std::shared_lock lock(m_mutex); diff --git a/src/engraving/infrastructure/eidregister.h b/src/engraving/infrastructure/eidregister.h index 76dd07306ae29..25045a2a22e68 100644 --- a/src/engraving/infrastructure/eidregister.h +++ b/src/engraving/infrastructure/eidregister.h @@ -39,6 +39,7 @@ class EIDRegister EID newEIDForItem(const EngravingObject* item); void registerItemEID(const EID& eid, const EngravingObject* item); void removeItem(const EngravingObject* item); + void onItemDestroyed(const EngravingObject* item); EngravingObject* itemFromEID(const EID& eid) const; EID EIDFromItem(const EngravingObject* item) const; diff --git a/src/engraving/tests/eid_tests.cpp b/src/engraving/tests/eid_tests.cpp index 506762ebab84b..255e71b73534e 100644 --- a/src/engraving/tests/eid_tests.cpp +++ b/src/engraving/tests/eid_tests.cpp @@ -22,8 +22,13 @@ #include +#include "engraving/dom/dynamic.h" #include "engraving/dom/engravingitem.h" +#include "engraving/dom/factory.h" +#include "engraving/dom/masterscore.h" +#include "engraving/dom/measure.h" +#include "engraving/compat/scoreaccess.h" #include "utils/scorerw.h" using namespace mu::engraving; @@ -55,3 +60,65 @@ TEST_F(Engraving_EIDTests, testRegisteredItems) delete score; } + +TEST_F(Engraving_EIDTests, deletedItemIsUnregistered) +{ + MasterScore* score = compat::ScoreAccess::createMasterScore(nullptr); + + EngravingItem* item = new Dynamic(score->dummy()->segment()); + EID eid = item->assignNewEID(); + EXPECT_TRUE(eid.isValid()); + EXPECT_EQ(score->eidRegister()->itemFromEID(eid), item); + + delete item; + + // The register must not keep a stale entry for the deleted item; a later + // allocation could reuse the same address and collide with the stale entry + EXPECT_FALSE(score->eidRegister()->EIDFromItem(item).isValid()); + + delete score; +} + +TEST_F(Engraving_EIDTests, deletedExcerptScoreItemsAreUnregistered) +{ + MasterScore* master = compat::ScoreAccess::createMasterScore(nullptr); + Score* partScore = master->createScore(); + + Measure* measure = Factory::createMeasure(partScore->dummy()->system()); + partScore->measures()->append(measure); + + EID eid = measure->assignNewEID(); + EXPECT_TRUE(eid.isValid()); + EXPECT_EQ(master->eidRegister()->itemFromEID(eid), measure); + + // Deleting the part score (as when an excerpt is removed) must unregister + // its items from the master score's register, which stays alive. + // itemFromEID() asserts on a missing entry, so query by item instead. + delete partScore; + + EXPECT_FALSE(master->eidRegister()->EIDFromItem(measure).isValid()); + + delete master; +} + +TEST_F(Engraving_EIDTests, writeReadElementDoesNotLeakRegisterEntries) +{ + MasterScore* score = compat::ScoreAccess::createMasterScore(nullptr); + + Dynamic* dynamic = new Dynamic(score->dummy()->segment()); + dynamic->setDynamicType(DynamicType(1)); + + // writeReadElement round trips through clipboard write and paste read, + // which assigns a new EID to the newly created element + Dynamic* d = toDynamic(ScoreRW::writeReadElement(dynamic)); + EID eid = d->eid(); + EXPECT_TRUE(eid.isValid()); + EXPECT_EQ(score->eidRegister()->itemFromEID(eid), d); + + delete d; + + EXPECT_FALSE(score->eidRegister()->EIDFromItem(d).isValid()); + + delete dynamic; + delete score; +}