Empty chord symbols causes crash

• May 5, 2021 - 10:07
Reported version
3.6
Type
Functional
Frequency
Once
Severity
S2 - Critical
Reproducibility
Always
Status
active
Regression
No
Workaround
Yes
Project
MuseScore

Score taken from #320818: Chord symbols with a bass extension don't export to MusicXML correctly (nor import back), shortened and attached here.

The empty chord symbols look like this in the mscx:

          <Harmony>
            <name></name>
            </Harmony>

The possible culprits for having added those empty chord symbols are ChordIdentifierPopJazz and/or, ChordIdentifierSp3_2
However, MuseScore certainly should not crash on those.

To reproduce: double click the (one and only) visible chord symbol, press space twice

Actually it is not even crashing, not when build in Debug mode at least, but dying of a failed assertion:
Debug: actual text is empty (...\libmscore\textedit.cpp:131, virtual void Ms::TextBase::endEdit(Ms::EditData&))
Fatal: ASSERT: "newlyAdded || textWasEdited" in file ...\libmscore\textedit.cpp, line 136

Code:

      bool newlyAdded = false;
 
      if (ted->oldXmlText.isEmpty()) {
            UndoCommand* ucmd = textWasEdited ? undo->prev() : undo->last();
            if (ucmd && ucmd->hasFilteredChildren(Filter::AddElement, this)) {
                  // We have just added this element to a score.
                  // Combine undo records of text creation with text editing.
                  newlyAdded = true;
                  undo->mergeCommands(ted->startUndoIdx - 1);
                  }
            }
 
      // TBox'es manage their Text themselves and are not removed if text is empty
      const bool removeTextIfEmpty = !(parent() && parent()->isTBox());
 
      if (actualPlainText.isEmpty() && removeTextIfEmpty) {
            qDebug("actual text is empty");
 
            // If this assertion fails, no undo command relevant to this text
            // resides on undo stack and reopen() would corrupt the previous
            // command. Text shouldn't happen to be empty in other cases though.
            Q_ASSERT(newlyAdded || textWasEdited);
Attachment Size
test.mscz 4.86 KB

Comments

• May 5, 2021 - 10:16

Simply commenting that Q_ASSERT() out results in another assertion failing:

Debug: actual text is empty (...\libmscore\textedit.cpp:131, virtual void Ms::TextBase::endEdit(Ms::EditData&))
Debug: curIdx 0 size 0 (...\libmscore\undo.cpp:430, void Ms::UndoStack::reopen())
Fatal: ASSERT: "curIdx > 0" in file ...\libmscore\undo.cpp, line 432

Reply

• May 5, 2021 - 10:21
Workaround No ⇒ Yes

Workaround: select the note/rest with the empty chord symbol and add a chord symbol (via Ctrl+K)
Or, after a the 1st Space, enter a chord symbol before pressing Space again.

Maybe the reason for that empty chord symbol having been added by those plugins is the fact that the 2nd, chord is tied to the first (and as such really should have gotten the same chord symbol as the 1st)?

Reply

• May 5, 2021 - 10:29

Code at that 2nd assertion is:

void UndoStack::reopen()
      {
      qDebug("curIdx %d size %d", curIdx, list.size());
      Q_ASSERT(curCmd == 0);
      Q_ASSERT(curIdx > 0);
      --curIdx;

Changing it like this:

diff --git a/libmscore/undo.cpp b/libmscore/undo.cpp
index 4ad7732c4..b34dc6e5d 100644
--- a/libmscore/undo.cpp
+++ b/libmscore/undo.cpp
@@ -428,8 +428,8 @@ void UndoStack::endMacro(bool rollback)
 void UndoStack::reopen()
       {
       qDebug("curIdx %d size %d", curIdx, list.size());
-      Q_ASSERT(curCmd == 0);
-      Q_ASSERT(curIdx > 0);
+      if (curCmd != 0 || curIdx <=0 )
+            return;
       --curIdx;
       curCmd = list.takeAt(curIdx);
       stateList.erase(stateList.begin() + curIdx);

along with keeping that 1st assertion commented out fixes the crash

Reply

• May 5, 2021 - 10:35

Maybe the problem is that 'editing' that empty chord symbol apparently is seen a newly adding it?

      bool newlyAdded = false;
 
      if (ted->oldXmlText.isEmpty()) {
            UndoCommand* ucmd = textWasEdited ? undo->prev() : undo->last();
            if (ucmd && ucmd->hasFilteredChildren(Filter::AddElement, this)) {
                  // We have just added this element to a score.
                  // Combine undo records of text creation with text editing.
                  newlyAdded = true;
                  undo->mergeCommands(ted->startUndoIdx - 1);
                  }
            }

Reply

• May 5, 2021 - 11:23

With both Q_ASSERT() being commented out it finally crashes at accessing a list item at an index of -1. Actually yet another failed assertion, but now in Qt code, in qlist.h:

inline T QList<T>::takeAt(int i)
{ Q_ASSERT_X(i >= 0 && i < p.size(), "QList<T>::take", "index out of range");

Here again the MuseScore code leading to that:

void UndoStack::reopen()
      {
      qDebug("curIdx %d size %d", curIdx, list.size());
      Q_ASSERT(curCmd == 0);
      Q_ASSERT(curIdx > 0);
      --curIdx;
      curCmd = list.takeAt(curIdx);

So indeed rather than the Q_ASSERT(), we should better leave the method right there, when curIdx <= 0.
Doing that alone doesn't fix the issue though, not in Debug mode (but would in Release mode)

Reply