Crash when removing a section break, or by various editing work before it, in a score with parts and MM rests

• Jan 30, 2019 - 22:08
Reported version
3.0
Priority
P0 - Critical
Type
Functional
Frequency
Many
Severity
S1 - Blocker
Reproducibility
Always
Status
closed
Regression
Yes
Workaround
No
Project
MuseScore

3.0.2 /Windows10

1) Default score
2) Add a section break
3) Generate parts
4) In main score: Select and cut the section break

Result: crash

1) Default score
2) Enter a few notes in first measure
3) Add a section break measure 8 eg
4) Generate parts
5) In main score: copy-paste first measure before section break (eg in measure 4 or 5 on the test file)

Result: crash

  • Note in these scenarios: no issue if disabling mm rests in part before last crucial step(s).

Comments

• Feb 5, 2019 - 14:14

Ran the test through gdb and came up with this:

Thread 1 "mscore" received signal SIGSEGV, Segmentation fault.
0x000055555624b19c in Ms::LayoutBreak::pause (this=0x0) at libmscore/layoutbreak.h:65
65            qreal pause() const                 { return _pause;               }
(gdb) ba
#0  0x000055555624b19c in Ms::LayoutBreak::pause (this=0x0) at libmscore/layoutbreak.h:65
#1  0x00005555566ce176 in Ms::MeasureBase::pause (this=0x5555599e4a90) at libmscore/measurebase.cpp:267
#2  0x00005555566ab60e in Ms::Score::getNextMeasure (this=0x5555599dd770, lc=...)
at libmscore/layout.cpp:2567
#3  0x00005555566b002f in Ms::Score::collectSystem (this=0x5555599dd770, lc=...)
at libmscore/layout.cpp:3243
#4  0x00005555566b6172 in Ms::Score::doLayoutRange (this=0x5555599dd770, stick=0, etick=15360)
at libmscore/layout.cpp:4203
#5  0x000055555681ae66 in Ms::Score::update (this=0x5555599baac0) at libmscore/cmd.cpp:220
#6  0x000055555681ab2f in Ms::Score::endCmd (this=0x5555599baac0, rollback=false)
at libmscore/cmd.cpp:178

As can be seen in #0 'this' is null, so the pause method dereference will cause sigsegv.
If looking at the code in #1, measurebase.cpp around 267 we see:

qreal MeasureBase::pause() const
{
return sectionBreak() ? sectionBreakElement()->pause() : 0.0;
}

Now this code assumes sectionBreakElement() always returns an object and not NULL.

This is not the case as can be seen in measurebase.cpp:603 :

LayoutBreak* MeasureBase::sectionBreakElement() const
    {
   if (sectionBreak()) {
       for (Element* e : el()) {
           if (e->isLayoutBreak() &&
               toLayoutBreak(e)->isSectionBreak())
                   return toLayoutBreak(e);
                }
           }
    return 0;
    }

It SEEMS to be the cause in this case, but perhaps there is other ways to trigger the same bug.

EDIT: cleaned up formatting.

Reply

• Feb 5, 2019 - 14:22

Maybe fixing it as:

qreal MeasureBase::pause() const
      {
      return (sectionBreak() && sectionBreakElement()) ? sectionBreakElement()->pause() : 0.0;
      }

Reply

• Feb 5, 2019 - 15:14

sectionBreakElement() Seems to be used in a similar fashion at other places too.
It seems that they assume sectionBreakElement() always returns an object as long as sectionBreak() holds. But sectionBreakElement() also needs isLayoutBreak() on any element it returns.

Possible other places where (perhaps) similar bug could appear?

libmscore/edit.cpp:552:                        sectionBreak = new LayoutBreak(*lm->sectionBreakElement());
libmscore/edit.cpp:620:                              sectionBreak = measure->sectionBreakElement();
libmscore/measure.cpp:2793:      m->setSectionBreak(sectionBreak() ? new LayoutBreak(*sectionBreakElement()) : 0);
libmscore/layout.cpp:3186:            lc.startWithLongNames = lc.firstSystem && measure->sectionBreakElement()->startWithLongNames();
libmscore/layout.cpp:3449:            lc.startWithLongNames = lc.firstSystem && lm->sectionBreakElement()->startWithLongNames();

Reply

• Feb 6, 2019 - 15:00

It seems when pressing delete on the section-break element. The element is removed from a measure, and cascade-removed from others (master and foreach part). But on one of measure, it gets removed, but the flag is not resetted.
The removal is done by the element disappearing from the measure's ElementList.
See the kludge-comment in the patch below for more details.

The patch works by cleaning up the measure during layout, it is a bit kludgy and would need future cleanup (removal).

diff --git a/libmscore/measurebase.cpp b/libmscore/measurebase.cpp
index 80c37f89f..5d1b9589c 100644
--- a/libmscore/measurebase.cpp
+++ b/libmscore/measurebase.cpp
@@ -436,6 +447,9 @@ void  MeasureBase::cleanupLayoutBreaks(bool undo)
     {
     // remove unneeded layout breaks
     std::vector toDelete;
+      bool has_lb_section_elm = false;
+      // Remove layout-break-elements when no such ElementFlag
+      // is set on the measure.
@@ -448,6 +462,7 @@ void MeasureBase::cleanupLayoutBreaks(bool undo)
                                 toDelete.push_back(e);
                           break;
                     case LayoutBreak::SECTION:
+                              has_lb_section_elm = true;
                           if (!sectionBreak())
                                 toDelete.push_back(e);
                           break;
@@ -464,6 +479,20 @@ void  MeasureBase::cleanupLayoutBreaks(bool undo)
         else
               _el.remove(e);
         }
+
+      // KLUDGE: somehow the sectionBreak flag is left set,
+      // but no layout-break-section-elements are associated.
+      // somewhere, the l-b-s element is removed but the
+      // flag is never resetted.
+      // There is a desire to find out why the l-b-s element
+      // disappear. Because it could reveal the root-cause-bug,
+      // and remove this cleanup.
+      //
+       // Reset the ElementFlag::SECTION_BREAK when no
+      // l-b-s elements are found
+      if(sectionBreak() && (! has_lb_section_elm)){
+            setSectionBreak(false);
+            }
   }

diff --git a/libmscore/measurebase.h b/libmscore/measurebase.h
index 644b33c09..d7c8881dc 100644
--- a/libmscore/measurebase.h
+++ b/libmscore/measurebase.h
@@ -68,10 +68,8 @@ class MeasureBase : public Element {
   int _no                { 0 };       ///< Measure number, counting from zero
   int _noOffset          { 0 };       ///< Offset to measure number

-   protected:
-      void cleanupLayoutBreaks(bool undo);
-
public:
+      void cleanupLayoutBreaks(bool undo);
   MeasureBase(Score* score = 0);
   ~MeasureBase();
   MeasureBase(const MeasureBase&);

diff --git a/libmscore/layout.cpp b/libmscore/layout.cpp
index eb814866c..deea4ee61 100644
--- a/libmscore/layout.cpp
+++ b/libmscore/layout.cpp
@@ -2564,6 +2565,22 @@ void Score::getNextMeasure(LayoutContext& lc)
   //
   //  implement section break rest
   //
+      measure->cleanupLayoutBreaks(true);

Reply

• Feb 7, 2019 - 05:40

Other aspects and fatal scenarios:

1) Load the first attachment ie: test section break parts.mscz
2) Select whole rest let's say two measures before the section break
3) Insert new time signature
(same result if inserting key signature, or attempt to change the whole rest value in two half rests ,etc, etc.) and probably some others.

Result: crash

Reply

• Feb 7, 2019 - 05:44
Frequency Few ⇒ Many
Severity S2 - Critical ⇒ S1 - Blocker

Given the magnitude of the scenarios involved and in common use cases, and the consequences on scores and the degraded image on the stability of the program right now (eg: https://musescore.org/en/node/283639, following a bad experience related to this issue), I really think that this issue deserves the "Blocker" status.

Reply

• Feb 7, 2019 - 07:18

"and probably some others"

As articulations, ie fermatas, and repeat symbols (Fine, D.C. etc)

I don't think it is necessary to insist further: this issue must be fixed as soon as possible, really.

Reply

• Feb 7, 2019 - 19:57

I think I've found the root-cause, it calls for a much simpler patch, I have a more prepared patch with more safety checks and code doc, but show the most relevant change below.
One of the measures ( a MMR), gets in an inconsistent state during layout ( blame Score::createMMRest ).
It deletes the elementlist from the measure without updating the measure flag-cache.
That responsibility is moved to measurebase.cpp by the patch.

void MeasureBase::clearElements()
 ElementList MeasureBase::takeElements()
       {
       ElementList l = _el;
+      setSectionBreak(false);

Reply

• Feb 7, 2019 - 22:40

Confirming crash with file "test section break parts.mscz" and procedure described (pressing M, delete sec-brk, pree M).

Also with my patch applied, same procedure wont crash and works as expected as far as I can see.

Reply

• Feb 8, 2019 - 11:20
Title Delete section break or copy-paste before a section break in a score with parts/mm rests crashes the program ⇒ Crash when removal section break, or by various editing work before it, in a score with parts and MM rests

Title more adapted now to the variety of situations leading to crash the program.

Reply

• Feb 8, 2019 - 11:51

Sure I'll submit a github-pr, my username there is "lyrra", I've agreed to transfer copyright ownership, is that enough?

Regarding this issue, the very presence of MeasureBase::cleanupLayoutBreaks() hints of some underlying problem. My patch doesn't address that.

Reply

• Feb 15, 2019 - 17:24
Title Crash when removal section break, or by various editing work before it, in a score with parts and MM rests ⇒ Crash when removing a section break, or by various editing work before it, in a score with parts and MM rests

Fix grammar.

Reply

Fix version
3.0.4