Crash after undoing twice, first a slur, then other line when the Inspector is not displayed

• Mar 9, 2016 - 08:14
Type
Functional
Severity
S2 - Critical
Status
closed
Project
MuseScore

GIT commits 295fd33 (2.0.3) and 61d1a87 (2.1.0) / Windows 7

1) "My First Score"
2) Enter two quarter notes in the first measure -> Exit note input mode -> select the first note -> add a slur
3) Undo
4) Add a hairpin, or crescendo line, or trill line (the first quarter note was always selected, but same issue if you deselect -> select the entire measure eg, and add the hairpin):
5) Undo

Result: crash
crash.jpg
Notes:

- I have no checked all in detail (just discovers this issue), but I am pretty sure that the addition/removal of a slur is a prerequisite to this issue.

- The inverse case "works": add/undo a hairpin, then add/undo a slur. BUT, by adding a third step ie: add/undo once again a hairpin, there is a crash.
In fact, you must have always the couple slur and then other line involved, not matter the number of steps.

Comments

• Mar 9, 2016 - 08:18

Notice that this issue does not occurs with the 2.0.2.
Further investagation later to try to locate. But I have a little idea!

Reply

• Mar 9, 2016 - 08:41

Just downloaded the 8a71dcb

I can always reproduce, systematically. Please follow the steps neatly. If need other elements/details, feel free to tell me.

Image below produced with the last mentioned nightly:
new.jpg

Reply

• Mar 9, 2016 - 08:44

I tried to follow the steps, only difference is I don't use 'My First Score' but either the treble clef tempate or grand piano template

Reply

• Mar 9, 2016 - 08:49

Well, same issue here with the treble clef template.
Try by downloading the nightly on the download page of MuseScore, not with your self build nightly?

Reply

• Mar 9, 2016 - 09:22

Found a little moment to check further...
So, this issue occurs on October 19, 2015

- This nigthly works (the single of October 12): dd4530e

- Not this one on October 19 (no nightlies between these two days): 130c710
There is four commits this day. I don't see for now where the problem might be located. Please check.

Reply

• Mar 9, 2016 - 11:51

Can't reproduce with this nigthly 33de14b after a RevertToFactorySettings. May be the key, the explanation.
Check later and if it's confirmed, I close.

Reply

• Mar 9, 2016 - 12:03
Status (old) active ⇒ closed

Can't reproduce with the two last nightlies after revert to factory settings.
So, I close.

Reply

• Mar 9, 2016 - 16:10
Status (old) closed ⇒ active

I can reproduce the crash with the original steps.

The crash occurs while trying to display the Inspector for the currently selected element, but the element is no longer valid. It might happen to still containing valid values from earlier, but if it doesn't, that is what causes the crash. It's also possible it depends on exactly how the liens were added. I used the shortcuts "S" and "<".

Anyhow, the crash is likely be to be sporadic, explaining why it's not always reproducible. I rather doubt any of those commits are directly responsible; they might have just happened to change whether the values for the currently invalid "currently selected element" are bad enough to cause the crash. I guess the commit involving the Inspector is msot involved, but again, it's probably not the *fault* of that change.

Reply

• Mar 9, 2016 - 20:10
Title Crash after undoing twice, first a slur, then other line ⇒ Crash after undoing twice, first a slur, then other line when the Inspector is not displayed

Absolutely, I discovered this "variable" of the Inspector, displayed or not, two hours ago when I was at my work!
My habit is to not display the Inspector when I enter a new score, or when I do some tests... But when I tried after returning to the default settings, the inspector WAS displayed by default, and therefore, this was the reason why I could not reproduce.

Here, the crash is never sporadic. It is systematic, I repeat again, but only when the Inspector is not displayed. For my part, I used the lines palette, by a double-click, or drag and drop to add slurs and hairpins (or other lines)

EDIT: by using the shorcut ">", no issue indeed.

But systematic crash:
1) by adding slur:
- with the shorcut "S"
- by double-clicking the slur in the Lines palette (and drag and drop)

2) by adding the hairpins, and the crescendo lines, and the trill lines (maybe some others, I have not checked all) via the Lines palette by double-clicking (drag and drop)

EDIT1: ottavas lines, and Voltas, too... Well, after checking further, all the Lines in the palette are involved, in a "logical" way I presume.

Reply

• Mar 9, 2016 - 21:26

Actually, now that I realize the visiblity of the Inspector is the trigger, maybe this commit *could* be consider the cause, although I still suspect it would be more accurate to say the bug was already there just waiting to be tripped over, and this commit is what tripped over it first.

Apparently even though the Inspector is not being displayed, items are being added to the Inspector's list of elements, and after an undo, the elements are not being removed from the list. Probably we shouldn't bother adding the elements to the list in the first place if the inspector is not visible - which is perhaps to say the commit in question didn't go far enough. But I think trying to fix it that way would be hard and not worth the effort. Instead, we could take the existing code, and in the cases where the Inspector is not visible, instead of doing *nothing*, at least clear "_element".

I *think* we could do this by removing the "|| !inspector->visible()" from the code in the commit in question, and instead having updateInspector() itself check that, calling inspector->setElement(0) in that case. Since I'm still not 100% I understand the exactl conditions that lead to crashes (since it is essentially memory corruption, the details will differ from system to system, also perhaps according to whether you have a debug build or not, etc), I wouldn't mind if others could try that out. I'll play with this later myself as well.

Reply

• Mar 9, 2016 - 21:34

A fix for 2.0.3 and master could be different, for 2.0.3 we might just disable that commit, for master we'd have more time maybe to get a real fix done?

Reply

• Mar 9, 2016 - 23:12
Status (old) active ⇒ patch (code needs review)

Here's my proposed fix:

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

I modified updateInspector() to clear the currently selected element record if the Inspector is not visible. Because this means the Inspector no longer tracks the selection, I also modified showInspector() to be sure to set it again when the Inspector is re-enabled.

Reply