Crash after removing and undoing a staff in file with elements linked to themselves

Dies with an assertion failure in current self-built master (in DEBUG mode):
Fatal: ASSERT: "le" in file C:\Users\Jojo\Documents\GitHub\MuseScore\libmscore\undo.cpp, line 3565 (:0, )

Initial investigation shows the "le" (linked element) in question is null and we're trying to re-link it to a rest, but it's a rest that is completely bogus. We're trying to undo an "Unlink" command, where "e" is the bogus rest and "le" is null. Looking at the undo stack at that point, there are lots of operations there (to be expected), and *all* the unlink operations have a null "le" and bogus "e". This was just the first "Unlink" command we are trying to undo. So it's not just one bad element - the whole score is full of bad links.

The score apparently had linked staves at one time - all but the first staff is full of "lid" elements. That's not unusual in itself, adding them removing a linked part will do this. But normally it's harmless. So more investigation needed.

I'm going to take a shot at this. I'm posting stack trace when performing undo of remove flute:

1 raise 0x7fffee8dfa10
2 abort 0x7fffee8e113a
3 QMessageLogger::fatal(const char *, ...) const 0x7fffef282e21
4 qt_assert(const char *, const char *, int) 0x7fffef27e3de
5 Ms::Unlink::undo undo.cpp 3710 0x138b5a5
6 Ms::UndoCommand::undo undo.cpp 145 0x137c005
7 Ms::UndoStack::undo undo.cpp 319 0x137c6f0
8 Ms::MuseScore::undoRedo musescore.cpp 3362 0xbd3de0
9 Ms::MuseScore::cmd musescore.cpp 4502 0xbda74e
10 Ms::MuseScore::cmd musescore.cpp 4298 0xbd9606

As marc explained, le is null.

Studying what marc wrote...it sounds like the fact that le is null is a result of bad links. So sounds like the real problem is how did those links become bad? But I have no idea how to answer that question, so I probably can't help fix this issue at all.

What's weird is, it's not unusual to have elements linked to themselves. You get this by creating then deleting a linked part. And yet no crash of you then remove an instrument and undo. Furthermore, if you save such a file, there will be unnecessary "lid" tags for each element, just as in this score. But in the usual case, by the time you read in the score again, the links are gone. That is, object debugger shows no links. Somehow we detect the unneeded "lid" tags for other scores and end up with a score containing no links, but this score is different somehow. Right now it's just a guess that this relates to the crash somehow.

I'm wrong, actually I *can* reproduce this crash the same basic way I tried before:

1) untitled score
2) add instrument below original instrument
3) generate parts
4) delete parts
5) save
6) reload
7) delete second staff
8) undo

Result: crash / assertion failure (le != nullptr)

The common elements is that this creates a score with unnecessary lid. The bad links exist right after deleting the parts - we still have links to the deleted linked staves, which I've noted before sounds like trouble waiting to happen. But I can't get an actual crash after that unless I save and reload. Which to me suggests the problem could possibly be fixed by detecting what is happening during load and fixing things up then. Will look at that possibility if I can this weekend.

EDIT: and yes, if after the first save/reload cycle, I then do *another* save/reload, the lid's are gone, and so is the crash. So we don't get a crash if we do the instrument delete / undo right after we remove the parts, nor do we get it if we this after two or more save/reload cycles. Only if we do it the very next save/reload.

https://github.com/musescore/MuseScore/pull/3159

I basically just removed the assert statement, so if we try to re-establish a null link, instead of crashing, we do nothing. This works, and actually fixes the link to self in the process. I could have actually added more code to reestablish the links to self, but it's not helpful that I can see. This is enough to fix the crash and actually improve the health of the score in the same way that save/reload does.

Can this one still make it into 2.2?

My fix is "100% safe" in that by removing the assert statement that caused the termination, it only changes behavior in cases that were going to crash anyhow. It seems to actually leave you in a better position than you were, which is a nice plus. I can't guarantee something won't go wrong and lead to a crash further down the road, but again, this is a case where we were crashing anyhow.

No longer reproducible on master. But my fix should still be 100% safe and good should there ever be another 2.x release

The related PR has been closed.