[MU4] Fix #330595: Crash on selecting entire score after XML import #10860
Conversation
src/engraving/libmscore/select.cpp
Outdated
| @@ -527,7 +527,7 @@ void Selection::appendChord(Chord* chord) | |||
| if (sp->endElement()->isNote()) { | |||
| Note* endNote = toNote(sp->endElement()); | |||
| Segment* s = endNote->chord()->segment(); | |||
| if (s->tick() < tickEnd()) { | |||
| if (s && s->tick() < tickEnd()) { | |||
There was a problem hiding this comment.
This particular crash happens here, dereferencng s when that is a nullptr
Alternative fix: if (!s || s->tick() < tickEnd()) {, avoids the crash too, but doesn't skip the _el.append(sp). Which would be more correct?
There was a problem hiding this comment.
I'd be more worried why a chord would ever not have a segment?
There was a problem hiding this comment.
Well, then check the sample score from that forum post.
Yes, this is curing the symptom rather than the cause. But that isn't a bad thing, or is it?
Just check your pointers before using them.
It is only happening directly after the XML import, not anymore once the score has been saved as mscz (and a close/reopen).
There was a problem hiding this comment.
Well the issue is as you noted it's not clear what the right behaviour is in this case if a chord doesn't have a segment, and it seems like the sort of thing that might have consequences elsewhere. I'd be trying to fix it in the XML importer first, that's all.
I'd also note that the MS codebase, and even the short sample above, would probably be twice as big if it had to check every pointer first, due to the lack of a null propagation operator in C++.
There was a problem hiding this comment.
Don't check for error conditions you can't handle... but do check those you can.
I've now tried both methods on the xml from the forum post: the resulting scores are 100% identical.
But yes, of course it'd be better to fix the XML import, I just don't know where to look. @lvinken ?
There was a problem hiding this comment.
Indeed, MusicXML import should not result in invalid data. Will try to find the root cause.
There was a problem hiding this comment.
See comment added to https://musescore.org/en/node/330595 for possible root cause, shouldn't be too hard to track down and fix in a structural way.
src/engraving/libmscore/select.cpp
Outdated
| @@ -518,7 +518,7 @@ void Selection::appendChord(Chord* chord) | |||
| if (note->tieFor()->endElement()->isNote()) { | |||
| Note* endNote = toNote(note->tieFor()->endElement()); | |||
| Segment* s = endNote->chord()->segment(); | |||
| if (s->tick() < tickEnd()) { | |||
| if (s && s->tick() < tickEnd()) { | |||
There was a problem hiding this comment.
This change is just for extra safety, and in analogy to the other (and so the same question applies).
Backport of musescore#10860, plus a fix on importing an empty XML file, which doesn't apply to master
Backport of musescore#10860, plus a fix on importing an empty XML file, which apparently doesn't apply to master. For master it got fix in musescore#8200, in 3.x it got fixed in musescore#8199, but broke again with the port of musescore#8530.
Jojo-Schmitz
force-pushed
the
crash-select-all
branch
from
b61bcb5 to
b07f0b8
Compare
Backport of musescore#10860, plus a fix on importing an empty XML file, which apparently doesn't apply to master. For master it got fix in musescore#8200, in 3.x it got fixed in musescore#8199, but broke again with the port of musescore#8530. Converting some `foreach(..., ...)` into `for (... : ...)` along the way.
Backport of musescore#10860, plus a fix on importing an empty XML file, which apparently doesn't apply to master. For master it got fix in musescore#8200, in 3.x it got fixed in musescore#8199, but broke again with the port of musescore#8530. Converting some `foreach(..., ...)` into `for (... : ...)` along the way.
Jojo-Schmitz
force-pushed
the
crash-select-all
branch
3 times, most recently
from
0471cd6 to
d65606e
Compare
Jojo-Schmitz
force-pushed
the
crash-select-all
branch
from
d65606e to
0723e13
Compare
Backport of musescore#10860, plus a fix on importing an empty XML file, which apparently doesn't apply to master. For master it got fix in musescore#8200, in 3.x it got fixed in musescore#8199, but broke again with the port of musescore#8530. Converting some `foreach(..., ...)` into `for (... : ...)` along the way.
Jojo-Schmitz
force-pushed
the
crash-select-all
branch
from
0723e13 to
fca90a4
Compare
Backport of musescore#10860, plus a fix on importing an empty XML file, which apparently doesn't apply to master. For master it got fix in musescore#8200, in 3.x it got fixed in musescore#8199, but broke again with the port of musescore#8530. Converting some `foreach(..., ...)` into `for (... : ...)` along the way.
Jojo-Schmitz
force-pushed
the
crash-select-all
branch
from
fca90a4 to
2765931
Compare
|
This one too is waiting to get reviewed and merged since almost a year now. |
Resolves: https://musescore.org/en/node/330595 (the 2nd part, the first part is not an issue with master, looking at its fix for 3.x seems to indicate that this is because #8530 hasn't been ported to master yet? For 3.x and master that crash had been fixed in #8199 resp. #8200, we'd need to make sure it doesn't break again when porting those backend fixes to master)
Applies to 3.x too.
Strange enough: There is no crash when saving the imported XML as an MSCZ, closing and reopening it and doing that "select all" then.