[MU4] Fix GH#10653: Crash converting to whole note

[Jojo-Schmitz]

Resolves: #10653

Same issue with (and originally reported for) MU3

[Tantacrul]

Cool! Just tested it and it seems to be solving the crash. @RomanPudashkin / @Eism - if you are happy with this, it would be nice to merge (removes a P0).

Thanks @Jojo-Schmitz !

[Jojo-Schmitz]

What about merging this?

[Jojo-Schmitz]

Any particular reason not to merge this?

[Tantacrul]

@bkunda - when you get a moment, can you test this and assuming all's good, recommend to merge?

I can't quite manage it at the mo.

[bkunda]

@bkunda - when you get a moment, can you test this and assuming all's good, recommend to merge?

I can't quite manage it at the mo.

The logs for this run have apparently expired so I can no longer download any artifacts.

[Jojo-Schmitz]

Downside of letting PRs sit for such a long time...
Rebased and force-pushed now.

[RomanPudashkin]

This looks very suspicious. Where do these null pointers come from? It looks like this PR fixes the consequence of the problem, not the cause

[Jojo-Schmitz]

Well, I don't quite understand the question, obviously this one comes from findSegment().

That in turn used findSegmentR()

//---------------------------------------------------------
//   findSegmentR
//    Search for a segment of type st at measure relative
//    position t.
//---------------------------------------------------------

Segment* Measure::findSegmentR(SegmentType st, const Fraction& t) const
{
    Segment* s;
    if (t > (ticks() * Fraction(1, 2))) {
        // search backwards
        for (s = last(); s && s->rtick() > t; s = s->prev()) {
        }
        while (s && s->prev() && s->prev()->rtick() == t) {
            s = s->prev();
        }
    } else {
        // search forwards
        for (s = first(); s && s->rtick() < t; s = s->next()) {
        }
    }
    for (; s && s->rtick() == t; s = s->next()) {
        if (s->segmentType() & st) {
            return s;
        }
    }
    return 0;
}

Which indeed may return a nullptr.

[RomanPudashkin]

Why does it return null in this case? It usually indicates some other, more complicated problem

[Jojo-Schmitz]

Dunno. The method does on occasions and on purpose return a nullptr. So callers should take care of those and not relying on it to never do that.

If it were illegal, an assert() would be due in that findSegmentR(), wouldn't it?

[RomanPudashkin]

It took me some time to figure out the real cause of the crash, and now I understand it.

For example, the user selects a measure with 4 quarter notes and clicks on the whole-note button in the note input bar:

1. Score::padToggle calls Score::changeCRlen for each selected chord.

2. changeCRlen can automatically deselect the score and select notes. So, on the first iteration (one iteration for each chord) it deselects the previously selected measure and selects the first chord in it.

3. changeCRlen removes all the chords in the measure (using undo(new RemoveElement(...))), but keeps a pointer to the first chord (that was selected in the previous step) and keeps re-selecting it at each iteration

4. Thus, after using Score::padToggle, the selection still contains the pointer to the already removed chord

[RomanPudashkin]

This PR fixes one symptom of the problem (crash in one scenario), but it doesn't fix the problem completely (removed element in the selection). For example, if I cherry-pick the changes from the PR, I can't select anything in the note input bar after the steps from the issue (due to the corrupted selection list). I guess we will be able to reproduce this crash in other scenarios as well (even with the fix from the PR)

[Jojo-Schmitz]

Ah, I see.
Don't see the fix for that though :-(
Still won't invalidate my pointer checking in this PR, would it?

Or should that function never return 0 and that needs to get checked and prevented right there?

[RomanPudashkin]

Let's try this fix #13183

It prevents removed elements from being added to the selected element list

[Jojo-Schmitz]

That's fine, and of course better than my attempt here, but #13183 still allows Measure::findSegmentR() to return 0, which still may cause a crash whenever the return value of this is dereferenced. Should be prevented by an assert() at least.