Crash when undo after save in a score with parts and courtesy key sig

• Feb 5, 2016 - 15:44
Reported version
2.1
Type
Functional
Severity
S2 - Critical
Status
closed
Project
MuseScore

To reproduce:

1. Open attached score.
2. Go to measure 58 and double-click the slur in the second violin. Click the left handle, [Shift]+[Left].
3. Leave edit mode. Result 1: slur disappears.
4. Save the score. Undo the last command.
5. Result 2: Crash.

There are multiple other ways to cause a crash here, but this is the first one I was able to pin down reproducibly.

Attachment Size
Slur crashing.mscz 159.15 KB

Comments

• Feb 5, 2016 - 16:11

Can you reproduce from scratch? (I begin to try)
Eg I can't reproduce the crash (with the same steps) if the parts are deleted in your score

Reply

• Feb 9, 2016 - 19:35

The slurs in this score have become corrupted somehow, which of course shouldn't lead to crash, but it would be very good to understasnd how they got this way.

In particular, looking at the MSCX file in a text editor, there are quite a few slurs with a start but no stop. For example, second staff, first measure, there are two slurs that start on the first note - id numbers 131 and 132 - but only 132 has a "stop" element. And a bunch more just like this. These are what produce the "zero slur" warnings on the console. Probably we should delete them on read, since they won't render anyhow.

As for what might have caused this, I could believe it is another artifact of #95636: Lines extended with Shift+Left over more than one measure not saved/restored - like maybe somehow using Shift+left on a slur that is inside another slur (like we see in measure 58) could possibly lead to this condition.

Reply

• Feb 25, 2017 - 12:36
Title Slur disappears and leads to crash on undo after saving ⇒ Crash by editing slurs in a score with parts and courtesy key sig, and on Undo after Save

Version 2.0.3, and current 2.1 dev. f30d2ad / Windows7

Steps from scratch:

1) "My First Score"

2) Enter a whole note in first measure -> then two half notes in second measure + add a slur

3) Add a new key signature in measure 5: so, a courtesy key signature appears as expected (first check if this option is ticked in General/Style/Page)

4) Generate parts (new all -> Ok)

The test file at this step: 1 TEST Slur Key sig Parts Whole note.mscz

Then:
5) Edit (double-click) the slur
6) Select left handle -> Shift + left arrow

7) Escape (the slur disappears)

8) Save
9) Undo

Result: crash

- Other files for testing :
- 2 TEST Slur Key sig Parts half rest.mscz
- 2 bis TEST Slur Key sig Parts half note.mscz
- 3 TEST Slur Key sig Parts Quarter note.mscz

- Note: parts and courtesy key signature are two necessary conditions to get a crash. In the original file, by deleting parts, or by unticking courtesy key sig in measure 137, the issue does'nt exist.

Reply

• Feb 25, 2017 - 12:46

But now, I still have to understand and check for why with eg the main test file, and so with the involved configuration (ie Parts and Courtesy key sig), the editing of the slur then Escape, also causes its disappearing, even in September/October 2014, while the location of the other related issue clearly leads us into March 2015.
More investigation needed I fear:(

Reply

• Feb 25, 2017 - 15:29
Title Crash by editing slurs in a score with parts and courtesy key sig, and on Undo after Save ⇒ Crash when undo after save in a score with parts and courtesy key sig

Yes, it's unrelated with the slur
In 2.1

  1. Create a score, Add a keysig change with courtesy keysig
  2. Create parts
  3. Add a note, save
  4. Undo

Reply

• Feb 25, 2017 - 15:31
Title Crash when undo after save in a score with parts and courtesy key sig ⇒ Crash by editing a score with parts and courtesy key sig, and on Undo after Save

Indeed.
So, new steps:
1) Load this file: new test.mscz
2) Edit the score, eg change the value of the whole note (to half note), or add a note in measure 3.
3) Escape (or not, not necessary)
4) Save
5) Undo

Result: crash

EDIT: oups... crossed message! :)

Reply

• Feb 25, 2017 - 15:34
Title Crash by editing a score with parts and courtesy key sig, and on Undo after Save ⇒ Crash when undo after save in a score with parts and courtesy key sig

Reply

• Apr 24, 2017 - 03:37

This one seems worrisome, seems like we should be running into this one a lot. But I guess it's been around a few years.

The crash is an assertion failure in Segment::remove(). We are removing a key signature but the key signature is not actually present. The fact that we are removing a key signature in the first place seems odd at first glance, but we constantly remove and re-add the generated key signatures at the start of each system, so that's not necessarily an issue in itself.

Another clue: on the save operation, we hit a debug statement at the top of doLayout(), "layout outside cmd and dirty undo". This gets triggered twice - once for the score, once for the part.

Reply

• Apr 24, 2017 - 18:06

Following up on the debug statement clue:

Yes, the undo stack is non-empty - it has the edit operation we just performed on it. Back in Score::saveFile(), just before calling update() (which in turn calls doLayout()) we do an undo()->setClean(), but that's not the same as making the undo stack *empty*, nor should we empty the undo stack on each save - it would be impossible to undo after saving if we did. But presumably the debug message exists for a reason.

The debug statement was added here:

https://github.com/musescore/MuseScore/commit/0af0091e11c7c437d3e36898e…)

Now, I see that at that point in time, there was no update() call within Score::saveFile(). That was only added a month later:

https://github.com/musescore/MuseScore/commit/98fa634571dca115a1c41d324…

So my first inclination is to say, maybe we need to surround the update() call with beginMacro/endMacro calls so doLayout(0 can be in undo mode while doing its job. But unfortunately that doesn't do it - it eliminates the qDebug message in doLayout() but not the eventual assert failure and abort. Maybe this is a good thing to do for other reasons, but I'm not going to worry about that for now.

At the point where we perform the layout on the save, the undo stack has a single top-level command on it (assuming you did only a single edit), containing the list of individual steps that make up that command. Among those steps are two removeElement commands: one for a keysig, one for the KeySigAnnounce segment (hence, the one for the courtesy signature at the end of the first system).

The KeySigAnnounce segment shows as being empty - it does not contain the keysig. Maybe that's normal - the keysig was removed already - but the fact that the assertion failure is about this very situation gives me pause.

Another curious thing is that if there is no linked part, the update() call doesn't actually call doLayout() - the _layoutAll flag is not set. However, if there is a linked part, then _layoutAll *is* set, for both the score and the part. And we actually get the debug message in doLayout() twice as well.

So, now I'm down to wondering what the update() call is really for. Simply removing it solves the crash. Unfortunately the commit doesn't reference an issue, so I can't tell what it was trying to fix. Presumably some sort of display glitch that was happening on save?

Reply

• Apr 24, 2017 - 19:11

OK, a couple more things:

- The only reason linked parts are relevant here is that without them, it seems _layoutAll is not set at this point. Meaning, without linked parts, the update() call does nothing, it doesn't layout the score, so $m is not updated either. If I add a call to setLayoutAll(true) right before the update() call, then the $m gets updated like it is supposed to, and undo crashes just as it does with linked parts.

- I was wrong when I said startMacro/endMacro doesn't solve the problem. It does, but it adds an extra null step to the undo stack. It was when I tried to get around that by setting the "rollback" parameter to endMacro() that I was still getting the crash. I could live with that, but I do plan to investigate some more and see if I can get around that.

Reply

• Apr 25, 2017 - 21:27

Bad problem with my fix - it forces an undo on every save for scores that *don't* contain courtesy elements. Oops! Also, it breaks redo after save for scores that *do* contain courtesy elements.

The first of these problems is due to my desire to not add an extra null step on the undo stack. I can see how to solve the first problem easily enough, but I don't see a quick fix for the second - by basically making the layout operation into an undoable command, it breaks the ability to redo. Addressing that means probably getting deeper into the root of the problem - the generation and removal of courtesy elements - and even then it might not actually be possible given how we do this.

Still, I'll take a broken redo after save over a crash on undo, so I'll submit a fix for the first problem (the forced undo on save).

Reply

• Apr 26, 2017 - 17:34

Not until the next layout. So, if you close and reopen the score, or do any other edit, or use Style / General or Measure Properties or possible other dialogs that force relayout. But if you export to PDF immediately after the save, it will still have the old time.

Unfortunate, I agree, but I would still say it's better than breaking redo after save. And I don't see any other options without a redesign of how layout deals with generated / courtesy elements.

FWIW, though, I am not convinced $m is actually expanding correctly even when it does get updated. I would have though it should reflect the actual modification time on the *file*, but it doesn't - it reflects the time of the last layout (eg, the last edit), even if the file has not been saved. In fact it appears to update itself spontaneously, perhaps as a result of auto-save although I'm not sure about that.

Reply

• Apr 26, 2017 - 18:13

OK, I continue to wonder about this. Still not sure what the expected effect would be - to me, last modified should mean the file, and that's not the case, maybe that's deliberate? So I was wondering why it is that $m keeps updating itself. Turns out this happens only if the score is "dirty" (meaning it has pending changes that have not been saved, as per this code:

https://github.com/musescore/MuseScore/blob/2.1/libmscore/page.cpp#L565…

So I guess that's deliberate, if not what I personally expect. Anyhow, the interesting thing is that this code is *not* being hit as a result of a layout operation - it is being hit as part of the drawing, called eventually from ScoreView::paint() (which is to say, it probably actually only updates on pages that are actually being displayed).

Anyhow, what this suggests to me is this:

If you can force the ScoreView to redraw without actually doing a relayout, you might be good. I would think you'd need to force all pages to be redrawn, and I'd think that exisitng releases would have a bug where each page of a dirty score would potentially show a different time in the header on export, but this doesn't seem to be the case, so I'm still not really understanding everything about the situation.

Reply