Add system text and undo causes crash in score with multiple parts

I can reproduce. mmrests are not necessary, multiple parts are.

1) score for two instruments
2) add notes to first measure of each
3) generate parts
4) if you like, turn off mmrests in parts to remove them from consideration - doesn't matter
5) in score, add rehearsal mark, tempo mark, repeat text, or other system text to first measure
6) undo

Result: crash

Happens in LinkUnlink::doUnlink(), dereferencing "l", which is zero. I'm guessing the element gets removed early in the undo prcoess and that removes the links, then when we go to remove the element from the parts, we expect to see links but don't. Or something like that. FWIW, simply adding "if (l == 0) return;" fixes the crash, and things do basically seem to work, including subsequents undos etc. But I have no idea if there is something more fundamentally wrong that needs fixing.

There are two parts in the score. The first time we unlink the rehearsal mark, we do it on the one in the score. The _links list is put to nullptr in ScoreElement::unlink(). Then we want to unlink again and we do it on the same one. The _links list is null. We crash :(

So my proposal, we should unlink the "target" instead of the source. It will put the _links to 0 in the target, and it should be fine since we will get the links from the source again to unlink the second one.
And just to be sure, I'll add the if (l == 0) test suggested by Marc.

Fixed in fa0415be94

I'm afraid it's not fixed in d0e6b72 on Kubuntu 14.04.
I can still reproduce the crash using the steps given by Marc in #1.

Problem seems to be in libmscore/undo.cpp:3537

      else
            qDebug() << "Nothing to unlink !?";
      le->unlink();

le->unlink() will be called anyway, even if le has not been set.

I can't get a crash following *those* steps, but unfortunately I can with just one slight modification: I add the markings to measure 2, which is the first measure of the mmrest in the parts. Is that what you are doing as well? No crash for me adding and undo in measure 1.

For this new crash, the problem is now dereferencing "le" in the last line of the function; this is null. If I simple add a test for le before doing the unblink, the crash goes away, but you then get a crash on redo, as we are attempting to link to "le" which is still null.

If I add a test for null "le" in doLink, the crash on redo goes away, and again, it all *seems* to work OK. I added a few steps before and after adding the rehearsal mark, then undid all of the steps back to the beginning, the redid all the way to then end, and everything worked as expected, including setting up the links on the redo so the resulting rehearsal mark could be deleted from the score and it would be deleting from the parts, whether mmrests were on or off.

You're right. I didn't try the first measure. It doesn't crash there.

Here's an image of my test file, because there's only a single place, it crashes, which is m3. Adding a system text anywhere in m1, m2 or m4 works.

NOT FOUND: 01

Yes, I believe the marking needs to be placed on a measure that would start a multimeasure rest in order to crash.

BTW, I tried simply testing for null "le" just to see if it had a chance of working, and it does. But I haven't studied the code and cannot say I actually believe there would be no problems from this. I am leaving this fix in place on my system just to see if I see any other harmful effects.

If it helps, I can say that this issue occurred there is five days, on March 10:
- between this nigthly, correct: cd9e1be

- and this one: 39164ce
with a crash following the steps of the comment # 1 (EDIT: by adding a rehearsal mark -> undo)

There is a few intermediate commits between these two Nightlies, I haven't checked for the moment where might be the cause.

I have a fix I am testing, with a "medium" confidence level. Let me talk through what I think I have found, and what I do in my fix.

First, thanks again to cadiz1 for finding the commit at which this broke. The relevant part of that March 10 change is, the re-implementation of the Link & Unlink classes for undo commands. Much of that change was basically cosmetic. But the key here is the management of the e & le members (formerly e1 & e2). Previously, Link::undo always used e2, which corresponds to le, not e. And that, I think, is why lasconic's fix worked as well as it did - it changed doUnlink (which is called by Link::undo) to use le rather than e.

I think the reason it didn't work completely is le itself. In the past, we saved it when we created the undo command and just reused it as necessary when we undid a link or redid an unlink. The change on March 10 involved throwing away le after making a link, and trying to calculate a brand new value on unlink. I assume that was because there is some chance that le is no longer valid.

However, just randomly grabbing the first element (other than e) we find in the e's link list is not working for us. The issue here has to do with which element is the source & which is the target when creating the link and what order we do the link operations in. It seems to me that picking any old element to use for the le makes the order of operations for the unlink operations somewhat random. Well, not really random, but not what we need, either.

So, my fix: I don't throw away le on link. On unlink, I check to see if le is currently within e's link list, and only if not do I settle for the first available element.

I'd be lying if I said I knew this was the whole problem right there. But I think it's an improvement, and most importantly, it does seem to fix the problems here without breaking the original problem the March 10 change was designed to fix. Still testing to see if anything else breaks. But I've included more qDebug statements to help us understand when le might not be valid, and so far I've been running a whole bunch of undos and redos and haven't hit this yet.

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

Fixed in 89accfaf18

Automatically closed -- issue fixed for 2 weeks with no activity.