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: https://github.com/musescore/MuseScore/blob/master/mscore/editstafftype…
because that line returns false, and the line below it does not execute.
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.
@Marc: when and where did tab_sample.mscx come into the picture? Somehow I seem to be missing some context here...
Anyway, that file had been created with some MuseScore pre 2.0.0 build and MuseScore 1.3 correctly refuses to open it, as being too new. MuseScore 1.3's file version was 1.14, see https://musescore.org/en/developers-handbook/version-information.
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.
If that line fails for ANY reason, MuseScore will crash later in this same procedure. Actually it's the very next line in this procedure that will crash. setValues() chokes on a null preview->score().
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 work.is 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.
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.
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"
It seems that the crash happens here (or at least corruption starts appearing here): https://github.com/musescore/MuseScore/blob/755bf05f247c1ad05dd66e17fce…
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
@Jojo-Schmitz:
the problem in the old file loading was that the MuseScore version of that file is no more supported, as found by sideways https://musescore.org/en/node/108146#comment-491341
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?)
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.
Comments
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:
https://github.com/musescore/MuseScore/blob/master/mscore/editstafftype…
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:
https://github.com/musescore/MuseScore/blob/master/mscore/file.cpp#L2014
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.
@Marc: when and where did tab_sample.mscx come into the picture? Somehow I seem to be missing some context here...
Anyway, that file had been created with some MuseScore pre 2.0.0 build and MuseScore 1.3 correctly refuses to open it, as being too new. MuseScore 1.3's file version was 1.14, see https://musescore.org/en/developers-handbook/version-information.
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.
Ah, thanks for clarifying. I had the feeling of missing something important, and indeed I did ;-)
See https://github.com/musescore/MuseScore/pull/2578, that change indeed fixes the crash
What about exiting if this line returns false:
https://github.com/musescore/MuseScore/blob/master/mscore/editstafftype…
?
If that line fails for ANY reason, MuseScore will crash later in this same procedure. Actually it's the very next line in this procedure that will crash. setValues() chokes on a null preview->score().
We could use Q_Assert here and crash right on the spot. But with that score fixed, why bother?
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 work.is 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.
As said, if that builtin part is missing there are more serious issue to worry about
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.
Sorry... :-( I thought you had abandoned my suggestion.
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.
on Werner's request I went back to my original approach to crash on the spot.
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.
Fixed in branch master, commit c67cd19801
Merge pull request #2578 from Jojo-Schmitz/advanced-staff-properties
fix #108146: crash when entering Advanced Staff Properties dialog
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
![crash.jpg]( "crash.jpg")
No crash here, Windows 7, 755bf05, self-build
Also no crash here, Linux, 755bf05.
Well, systematic crash here, with 755bf05
Ditto, after a "RevertToFactorySettings"
No crash here either, self-build. Maybe something went wrong with the nightly build and the fix wasn't incorporated?
In case someone else has an idea...
For the record, this issue occurs on last April 19.
- No issue with this nightly: a670a5f
- Crash with this one: 3b79d72
Release vs Debug build problem? The nightlies are release build, I guess your local builds are debug ones?
mine certainly is. Guess I could try to do a release build and test again.
@lasconic
That seems to be the problem. I just built a release build, and it crashes with the above steps.
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):
https://github.com/musescore/MuseScore/blob/755bf05f247c1ad05dd66e17fce…
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.
see also #113711: Staff properties crashes application
And here.
https://musescore.org/en/node/119561
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.
Possible PR, which keeps the Q_ASSERT (in debug mode):
https://github.com/musescore/MuseScore/pull/2755
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
Fixed in branch master, commit ee01950a56
fix #108146 crash in edit staff properties (release build)
Fixed in branch master, commit 0fd9ed2fd9
Merge pull request #2755 from AntonioBL/editstafftype_crash
fix #108146 crash in edit staff properties (release build)
@Jojo-Schmitz:
the problem in the old file loading was that the MuseScore version of that file is no more supported, as found by sideways https://musescore.org/en/node/108146#comment-491341
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?)
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.
Yep! Confirmed, issue solved here. Thanks ABL.
Automatically closed -- issue fixed for 2 weeks with no activity.