Crash when changing time signature in front of a corrupted measure

You're all but one. Crashes are Critical.

Relates to #270643: [EPIC] MusicXML import/export issues

For the record, the score is corrupt - not sure if that's because the MusicXML is or an error on import. Anyhow, with corrupt scores, crashes are not surprising, so if it was a corrupt MusicXML, that lessens the severity of this in my book.

Using a reasonably current 3.x (commit 379a44d of Jan 26) I get a consistent crash after importing the attached MusicMXL file when deleting the 2/4 time signature in measure 32.

Stack trace:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0 libsystem_kernel.dylib 0x00007fff5b001b66 __pthread_kill + 10
1 libsystem_pthread.dylib 0x00007fff5b1cc080 pthread_kill + 333
2 libsystem_c.dylib 0x00007fff5af5d1ae abort + 127
3 org.qt-project.QtCore 0x0000000112eb4749 qt_message_fatal(QtMsgType, QMessageLogContext const&, QString const&) + 9
4 org.qt-project.QtCore 0x0000000112eb60c0 QMessageLogger::fatal(char const*, ...) const + 224
5 org.musescore.MuseScore 0x000000010a005e3d Ms::checkRest(Ms::Fraction&, Ms::Measure&, Ms::Fraction const&) + 221 (range.cpp:361)
6 org.musescore.MuseScore 0x000000010a0055ef Ms::TrackList::write(Ms::Score, Ms::Fraction const&) const + 4415 (range.cpp:489)
7 org.musescore.MuseScore 0x000000010a006c0a Ms::ScoreRange::write(Ms::Score*, Ms::Fraction const&) const + 138 (range.cpp:679)
8 org.musescore.MuseScore 0x0000000109df92d9 Ms::Score::rewriteMeasures(Ms::Measure*, Ms::Measure*, Ms::Fraction const&, int) + 6665 (edit.cpp:555)
9 org.musescore.MuseScore 0x0000000109dfcc16 Ms::Score::rewriteMeasures(Ms::Measure*, Ms::Fraction const&, int) + 422 (edit.cpp:618)
10 org.musescore.MuseScore 0x0000000109dff514 Ms::Score::cmdRemoveTimeSig(Ms::TimeSig) + 500 (edit.cpp:951)
11 org.musescore.MuseScore 0x0000000109e06921 Ms::Score::deleteItem(Ms::Element) + 1201 (edit.cpp:1680)
12 org.musescore.MuseScore 0x0000000109e10446 Ms::Score::cmdDeleteSelection() + 2518 (edit.cpp:2526)
13 org.musescore.MuseScore 0x00000001093c143d Ms::Score::cmd(QAction const*, Ms::EditData&)::$_140::operator()(Ms::Score*, Ms::EditData&) const + 29 (cmd.cpp:4264)

So probably an assertion failure in Ms::checkRest(Ms::Fraction&, Ms::Measure&, Ms::Fraction const&) + 221 (range.cpp:361)

Well, almost, it exits with that qFatal():

static void checkRest(Fraction& rest, Measure*& m, const Fraction& d)
      {
      if (rest.isZero()) {
            if (m->nextMeasure()) {
                  m  = m->nextMeasure();
                  rest = m->ticks();
                  }
            else {
                  qFatal("premature end of measure list, rest %d/%d", d.numerator(), d.denominator());
                  }
            }
      }

So this is a planned crash, then, when an anomaly is encountered while rewriting the track list after a change in time signature. But instead of crashing, it could just back out of the rewrite, like it does in the case of a local time signature, or if a tuplet would end up crossing a barline in an unsupported manner. To accomplish this, checkRest() would become a bool function that returns true on success and false on failure. The qFatal() would become a qWarning() instead. And in the two places where checkRest() is called, if it returns false, then the caller would return false right away. But not before setting an appropriate error message. Then we would have to get rid of these lines, or else the wrong error message would be shown. Besides, at that point it is a little too late to be setting the error message anyway. Most likely an error has already been set, and if not, that in itself is a mistake.

Cause of crash found, minimal file reproducing the issue is attached.

Analysis: measure 34 (original file) contains in voice 2:
- 1/2 C#5 on beat 1
- a backup to beat 1
- 1/4 F#4 also on beat 1 (note: in the same voice as the C#5, but not in a chord with the C#5)
- 1/4 F#3 on beat 2
On import this results in corrupted timing in voice 2 (1/2 F#4/C#5 on beat 1, 1/4 F#3 on beat 2 instead of beat 3), which breaks rewriteMeasures().

Deleting the F#3 (or moving it to voice 3) prevents the crash.

Even though I find this MusicXML construction dubious, I still think it should be detected on import and should not lead to a crash.

It is detected on import. Hence all the warnings and red boxes around the measures. Are you saying it should be corrected on import so that the measure is not corrupted? That is another conversation entirely, and it pertains to malformed scores in any format, not just MusicXML.

Likewise, the issue at hand really has nothing to do with MusicXML either. The sample score just happened to be a MusicXML file.

Importers should never create incorrect data structures leading to a crash when editing. It is ok for an imported score to contain timing errors, provided the GUI can be used to fix these without crashing.

Importing the attached files leads to a measure with a 1/2 note at beat 1 and a 1/4 at beat 2 instead of beat 3. As MuseScore cannot handle that and crashes, the importer should preferably not do that. What it should do could be discussed (and may not be entirely trivial).

I have already described in great detail what I believe to be the best solution. All that is left is to come up with the wording of the error message.

See https://github.com/musescore/MuseScore/pull/7373.

Your proposed solution will prevent the crash (that is good), but it does not solve the underlying problem (corrupt data).

I agree that there is room for improvement regarding how we deal with corrupt data. But I believe that is a topic for another thread.

Fixed in branch 3.x, commit f6fb8fd570

_Fix #316441: Crash when changing time signature in front of a corrupted measure

Resolves: https://musescore.org/en/node/316441.

Modified checkRest() so that it returns true on success or false on failure, instead of causing the program to crash if the end of the measure list is reached prematurely. This allows us to safely back out of a track list rewrite in the case of a time signature change in front of a corrupted measure._

Fixed in branch master, commit ec5da46cab

_Fix #316441: Crash when changing time signature in front of a corrupted measure

Resolves: https://musescore.org/en/node/316441.

Modified checkRest() so that it returns true on success or false on failure, instead of causing the program to crash if the end of the measure list is reached prematurely. This allows us to safely back out of a track list rewrite in the case of a time signature change in front of a corrupted measure._

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