Fix stale pointer entries in EID register when items are deleted#34170
Fix stale pointer entries in EID register when items are deleted#34170manolo wants to merge 3 commits into
Conversation
Deleting an item left its entry in the EID register, which maps raw pointers. A later allocation could reuse the freed address and collide with the stale entry, corrupting the register and asserting in Debug builds when elements are read in paste mode. Unregister items in Score::onElementDestruction, which already guards against scores under destruction via validScores. Fixes musescore#34167
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded EID register cleanup for destroyed engraving objects, removing both mapping directions. Score destruction now notifies the master score’s register, with validity checks for the master and current scores. Tests cover dynamic element deletion, excerpt cleanup, and clipboard-style element round trips. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Public OSS repositories can only analyze public repositories installed in this organization. No linked repositories were analyzed; skipped Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/engraving/dom/score.cpp (1)
375-393: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftCleanup is skipped during score teardown.
Score::~Score()removes the score fromvalidScoresbefore deleting its items, soonElementDestruction()returns early for teardown-owned elements. That leaves the master score'sEIDRegisterstale when an excerpt score is deleted viaExcerpt::~Excerpt()/MasterScore::removeExcerpt().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/engraving/dom/score.cpp` around lines 375 - 393, Update Score::onElementDestruction so score teardown still unregisters destroyed elements from the master score's EIDRegister even after the score is removed from validScores. Preserve the early return for null or unrelated scores, while ensuring excerpt-owned elements deleted through Excerpt::~Excerpt() or MasterScore::removeExcerpt() reach eidRegister()->onItemDestroyed(e).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/engraving/dom/score.cpp`:
- Around line 375-393: Update Score::onElementDestruction so score teardown
still unregisters destroyed elements from the master score's EIDRegister even
after the score is removed from validScores. Preserve the early return for null
or unrelated scores, while ensuring excerpt-owned elements deleted through
Excerpt::~Excerpt() or MasterScore::removeExcerpt() reach
eidRegister()->onItemDestroyed(e).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eef229fd-a5ac-4c72-a732-e60bbebaad65
📒 Files selected for processing (4)
src/engraving/dom/score.cppsrc/engraving/infrastructure/eidregister.cppsrc/engraving/infrastructure/eidregister.hsrc/engraving/tests/eid_tests.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/engraving/tests/eid_tests.cpp`:
- Line 98: Replace the post-delete assertion using EIDFromItem(measure) with an
itemFromEID(eid) lookup, asserting that the returned item is invalid or absent.
Update the assertion in the relevant EID test while preserving the existing
expectation that the deleted measure’s EID no longer resolves.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f2507fb8-5960-4303-bf11-0f139daecde7
📒 Files selected for processing (2)
src/engraving/dom/score.cppsrc/engraving/tests/eid_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/engraving/dom/score.cpp
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
Resolves: #34167
The EID register maps raw pointers, and deleting an item did not remove its entry. A later allocation could reuse the freed address and collide with the stale entry, corrupting the register and asserting in Debug builds when elements are read in paste mode. See the issue for the full analysis.
The fix unregisters items in
Score::onElementDestruction, which already guards against scores under destruction. The two new regression tests fail deterministically on all platforms without the fix, making the problem visible in CI even though the crash itself only manifested on macOS.