Fix stale pointer entries in EID register when items are deleted

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;
     }
 

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<EngravingObject*>(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);

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;

src/engraving/tests/eid_tests.cpp

@@ -22,8 +22,13 @@
 
 #include <gtest/gtest.h>
 
+#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;
+}