Staff Properties -> Advanced Style properties causes crash in RELEASE build

• Apr 25, 2016 - 17:53
Reported version
S2 - Critical

nightly 994090f, linux
1. new score
2. right click staff -> Staff Properties... -> Advanced Style Properties...

Result: Crash


Confirmed on Windows 7

Stack trace:
1 QListData::size qlist.h 107 0xc4a7b0
2 QList::size qlist.h 160 0xc38c07
3 Ms::Score::staff score.h 568 0xc202cc
4 Ms::EditStaffType::updatePreview editstafftype.cpp 489 0x56d457
5 Ms::EditStaffType::setValues editstafftype.cpp 236 0x56c437
6 Ms::EditStaffType::EditStaffType editstafftype.cpp 78 0x56ab58
7 Ms::EditStaff::showStaffTypeDialog editstaff.cpp 516 0x4e3612
8 Ms::EditStaff::qt_static_metacall moc_editstaff.cpp 125 0x680251
9 ZN11QMetaObject8activateEP7QObjectiiPPv 0x68a07a62
10 ZN15QAbstractButton7clickedEb 0x199c10dc
11 ?? 0x20072cd0
12 ?? 0x1431e2
13 ?? 0xff302e2f
14 ?? 0x3e28164b
15 ?? 0x8c00d6cd
16 ?? 0xa
17 ZN15QPauseAnimation16staticMetaObjectE 0x68b29200
18 ?? 0x208bbc78

I don't feel comfortable fixing this, but I can point to the problem and maybe it will become obvious to someone else. The code fails to initialize the preview->score() here:…
because that line returns false, and the line below it does not execute.

If you step into that line, it returns Ms::Score::FileError::FILE_TOO_OLD (7) here:

It seems that the code is trying to initialize a blank or default score here when it should be using the selected score that is displayed in the edit staff dialog that launches this code/dialog. But I'm not at all familiar with this code, I just bumped into this issue while doing something else.

The tab_sample.mscx file has a file format number 1.24, which would be consistent with MuseScore 1. That's clearly not actually the case though - MuseScore 1 didn't support tab. The file must have been generated with a 2.0 nightly build from before the file format version number was updated. Simple fix would probably be to udpate the version string in that file to 2.07 or whatever we are using now.

Also: if the initial readscore() fails for whatever reason, and the preview-score() is not initialized, the code should exit and inform the user of the problem. Letting the code continue causes the crash. If the preview-score() is null, the dialog cannot open.

See response #2 again for the context. tab_sample.mscx is the file that provides the sample score used in the tab preview. This is the score MuseScore fails to load in the code referred to in response #2. Should be a simple matter of adjusting the version. Maybe ot a bad idea to regenerate the sample score while we're at it, but probably not crucial.

I'm just opposed to crashing in general, especially if there is a more graceful option. What if that file is missing or corrupted? Is crashing the right way to deal with an installation error, for example?

Put it this way: if this code exited gracefully, then this issue would not have been "critical".

That file is built in to MuseScore, if it'd be missing, there would be more serious things to worry about. And a graceful exit isn't any different than crash from a user prospective, current lost.

A Q_Assert has the advantage that the developers immediately know where to look

I must be imagining a different type of graceful exit than you. I'm thinking of an informational message telling the user that the file cannot be opened, and then back to the original state: the Edit Staff dialog is open and no work is lost.

Updated PR, it now not only updated that score, but in futur crashes with a good error message if that preview score can't get loaded for some reason. I tried to avoid that too, but this turned out to become to deeply nested and not really worth the effort.

I additionally fixed the crash in updatePreview, but then it crashed at another and pretty much unrelated strange place, so I gave up on it (and with @lasconic's Support ;-)). But as said, if that sample score can't get loaded, there got to be a more serious problem, so it is better to crash with a decent error message pointing at the real spot.

Status (old) patch (code needs review) fixed

Fixed in branch master, commit 5c96be9aeb

fix #108146: crash when entering Advanced Staff Properties dialog

by updating the tab_sample.mscx score to report `mscVersion() == 206`
(may need to get revisited later in the development process). If this
still fails (or again, in some futur version), crash on the spot.
Additionaly removed the check for `preview` in
`EditStaffType::updatePreview()` on @wschweers's request.

Status (old) fixed active

Not solved here (Windows7) with 755bf05 and c385632
Same steps:
1) "My First Score"
2) Right-click on a measure -> Staff properties
3) Click on "Advanced Style Properties"

Result: crash

Title Staff Properties -> Advanced Style properties causes crash Staff Properties -> Advanced Style properties causes crash in RELEASE build

Stack trace:
1 Ms::StaffType::isSameStructure(Ms::StaffType const&) const 0x79802c
2 Ms::StaffType::operator==(Ms::StaffType const&) const 0x7980e3
3 ZN15QPauseAnimation16staticMetaObjectE 0x68b29200
4 ?? 0xa
5 ZNK15QAbstractButton11isCheckableEv 0x1d6d3b0
6 ??

It seems that the crash happens here (or at least corruption starts appearing here):…
since in release mode preview->score()->staff(0) is 0x0 (both preview and preview->score() are valid; tested under Linux Mint 17.3 by inserting QMessages to output local values), while it is an apparently valid pointer when in debug mode. However, I see a blank preview in the advanced style properties windows in this latter case.

And the cause of the (new) problem is actually extremely simple: the instruction for reading the score is inside a Q_ASSERT, here , but the Q_ASSERTs are not evaluated in release mode, therefore the score is created but never populated when in release mode.

There's another place where we previously checked whether sc is valid, check the previous PR
Also the question is why the load of that file failed and whether it'd be better to fail right there, possibly with a suitable error message

Status (old) fixed patch (code needs review)

the problem in the old file loading was that the MuseScore version of that file is no more supported, as found by sideways
In release mode we can't have (or at least I can't have in my personal build, but I didn't try to run the release build inside a debugger) output debug message in recent code (similar to what happens under Windows), and Q_ASSERTs are ignored (left out at compilation time).
With this new PR at least in debug mode we have the error message "Error in opening sample file for preview" on the Q_ASSERT exit if there was an error loading the preview file.
Maybe the message can be improved by using also the kind of error as output (e.g. FILE_NOT_FOUND or FILE_TOO_OLD; how can I convert it to a string?)

Status (old) patch (code needs review) fixed

I saw that Nicolas merged the PR.
As I said in my previous comment (cross-posting), we could make the error message more verbose with also the type of error.