Fix realtime noteentry 8th crash (fixes #321716) #9593

fxthomas · 2021-10-27T18:04:01Z

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

To reproduce (on MuseScore 3.6.2):

Connect MIDI keyboard (via JACK/Pipewire in my case)
Enter Realtime (Manual) note input mode
Select "8th notes"
Enter 1 note and hit the manual input button 4 times on the MIDI keyboard, the 4th makes MuseScore crash

[?] I signed CLA
I made sure the code in the PR follows the coding rules
I made sure the code compiles on my machine
I made sure there are no unnecessary changes in the code
I made sure the title of the PR reflects the core meaning of the issue you are solving
I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
I made sure the commit message title starts with "fix #424242:" if there is a related issue
[?] I created the test (mtest, vtest, script test) to verify the changes I made

When comparing a Fraction (ticks2measureEnd) to the TDuration of the note being entered (is.duration()), the Fraction is implicitly converted by the TDuration(const Fraction&) ctor. Because of extra logic in TDuration, this can trigger a Q_ASSERT and crash Musescore in case the remaining Fraction cannot be exactly converted into a proper TDuration. In this case, this point is reached when ticks2measureEnd = 5/8 and is.duration() = 1/8. Since what we really want here is to compare the Fraction to the exact note duration (and not the other way around), we can just do an exact comparison between is.duration().fraction() and ticks2measureEnd to achieve the same goal.

fxthomas · 2021-10-27T18:11:14Z

The linked issue should be pretty self-explanatory, and this definitely fixes the issue for me, but I'm not familiar at all with MuseScore development.

How should I go about getting this included in the next point release? In particular, is it OK if I just check the checkmark here to signify that I agree to the CLA for this commit only? How should I go about adding tests here?

Jojo-Schmitz · 2021-10-27T18:42:42Z

Better rebase the PR to the master branch. There won't be any merges to the 3.6.2 branch and most probably not to the 3.x branch either (I've git cherry-picked it into my PR #9000 though)

You do need to sign the CLA though, on musescore.org.

When comparing a Fraction (ticks2measureEnd) to the TDuration of the note being entered (is.duration()), the Fraction is implicitly converted by the TDuration(const Fraction&) ctor. Because of extra logic in TDuration, this can trigger a Q_ASSERT and crash Musescore in case the remaining Fraction cannot be exactly converted into a proper TDuration. In this case, this point is reached when ticks2measureEnd = 5/8 and is.duration() = 1/8. Since what we really want here is to compare the Fraction to the exact note duration (and not the other way around), we can just do an exact comparison between is.duration().fraction() and ticks2measureEnd to achieve the same goal. Intgration of PR musescore#9593

When comparing a Fraction (ticks2measureEnd) to the TDuration of the note being entered (is.duration()), the Fraction is implicitly converted by the TDuration(const Fraction&) ctor. Because of extra logic in TDuration, this can trigger a Q_ASSERT and crash Musescore in case the remaining Fraction cannot be exactly converted into a proper TDuration. In this case, this point is reached when ticks2measureEnd = 5/8 and is.duration() = 1/8. Since what we really want here is to compare the Fraction to the exact note duration (and not the other way around), we can just do an exact comparison between is.duration().fraction() and ticks2measureEnd to achieve the same goal. Integration of PR musescore#9593

fxthomas · 2021-10-27T19:03:38Z

Thank you, that's one big PR but I'm glad some of the issues of 3.6.3 are getting fixed while 4.0 is being worked on!

I'm very much okay with it in the context of this small bugfix, hence the PR, but I was a bit iffy about signing the CLA irrevocably, that particular bit sounds like it just ignores the GPL for any contribution (which is never actually defined in that document...) that I might later do close to MuseScore:

You grant MuseScore BVBA the ability to use the Contributions in any way.

Trivial contributions (which this is IMO) should be fine, according to this:

All past and future contributors of non-trivial amounts of code (more than just a line or two) to MuseScore are required to sign the CLA. If somebody is unable to sign the document, their contribution will not be accepted or will be removed from MuseScore.

When comparing a Fraction (ticks2measureEnd) to the TDuration of the note being entered (is.duration()), the Fraction is implicitly converted by the TDuration(const Fraction&) ctor. Because of extra logic in TDuration, this can trigger a Q_ASSERT and crash Musescore in case the remaining Fraction cannot be exactly converted into a proper TDuration. In this case, this point is reached when ticks2measureEnd = 5/8 and is.duration() = 1/8. Since what we really want here is to compare the Fraction to the exact note duration (and not the other way around), we can just do an exact comparison between is.duration().fraction() and ticks2measureEnd to achieve the same goal. Integration of PR musescore#9593

Jojo-Schmitz · 2021-10-28T08:47:43Z

The CLA doesn't change anything about the MuseScore code being under GPL (GPL2 for 3.x, GPL3 for master), only whether parts of the code can also get used in other closed source stuff, like the mobile apps. At least that's my understanding.
Whether or not this code changes gets accepted (with or withoug the CLA) is not my call though.
It for sure won't get merged into the 3.6.2 branch, which is a dead end road (and none of the tests of builds on GitHub CI are run against it). No real need for it in the 3.x branch either (there's unfortunately, no futher 3.x version planned), only usefull thing would be the master branch, so it'd be in MuseScore 4.

fxthomas · 2021-11-06T21:27:38Z

Gotcha, I've thought about it and don't want to sign the CLA. Thanks for the help in any case!

Jojo-Schmitz · 2021-11-15T09:40:00Z

It'd still need to get rebased onto master.

When comparing a Fraction (ticks2measureEnd) to the TDuration of the note being entered (is.duration()), the Fraction is implicitly converted by the TDuration(const Fraction&) ctor. Because of extra logic in TDuration, this can trigger a Q_ASSERT and crash Musescore in case the remaining Fraction cannot be exactly converted into a proper TDuration. In this case, this point is reached when ticks2measureEnd = 5/8 and is.duration() = 1/8. Since what we really want here is to compare the Fraction to the exact note duration (and not the other way around), we can just do an exact comparison between is.duration().fraction() and ticks2measureEnd to achieve the same goal. Integration of PR musescore#9593

yajo · 2021-12-29T17:24:04Z

I'm just here to say thanks and please merge if possible, this issue is very annoying... 😓

Jojo-Schmitz · 2021-12-29T17:40:48Z

There won't be any further 3.x release, but you can try the artifacts from PR #9000.

As this PR here has not yet been ported to master (which will become MuseSDcore 4), the fix won't be there either

Edit: I've now submitted #10199 to get this fix into master

Port of musescore#9593

yajo · 2021-12-29T17:45:03Z

So you mean that #9000 will never be merged and than this PR should be rebased on top of master to be merged?

Jojo-Schmitz · 2021-12-29T17:50:12Z

Yes, neither this PR here nor #9000 are going to get merged. #10199 might.

yajo · 2021-12-29T17:51:42Z

Thanks! Then maybe this can be closed...

Jojo-Schmitz · 2021-12-29T17:56:49Z

Indeed.

When comparing a Fraction (ticks2measureEnd) to the TDuration of the note being entered (is.duration()), the Fraction is implicitly converted by the TDuration(const Fraction&) ctor. Because of extra logic in TDuration, this can trigger a Q_ASSERT and crash Musescore in case the remaining Fraction cannot be exactly converted into a proper TDuration. In this case, this point is reached when ticks2measureEnd = 5/8 and is.duration() = 1/8. Since what we really want here is to compare the Fraction to the exact note duration (and not the other way around), we can just do an exact comparison between is.duration().fraction() and ticks2measureEnd to achieve the same goal. Integration of PR musescore#9593

Jojo-Schmitz mentioned this pull request Nov 7, 2021

[MU3] Fix GH#7449: Almost everything suitable for 3.6.3 or 3.7.0 #9000

Closed

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Dec 29, 2021

Fix #321716: Realtime note entry 8th crash

4d289d8

Port of musescore#9593

Jojo-Schmitz mentioned this pull request Dec 29, 2021

[MU4] Fix #321716: Realtime note entry 8th crash #10199

Merged

RomanPudashkin closed this Dec 20, 2022

Jojo-Schmitz mentioned this pull request Mar 1, 2023

Fix GH#7449: Almost everything suitable for 3.6.3 or 3.7.0 Jojo-Schmitz/MuseScore#80

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix realtime noteentry 8th crash (fixes #321716) #9593

Fix realtime noteentry 8th crash (fixes #321716) #9593

fxthomas commented Oct 27, 2021 •

edited

Loading

fxthomas commented Oct 27, 2021

Jojo-Schmitz commented Oct 27, 2021 •

edited

Loading

fxthomas commented Oct 27, 2021 •

edited

Loading

Jojo-Schmitz commented Oct 28, 2021 •

edited

Loading

fxthomas commented Nov 6, 2021

Jojo-Schmitz commented Nov 15, 2021

yajo commented Dec 29, 2021

Jojo-Schmitz commented Dec 29, 2021 •

edited

Loading

yajo commented Dec 29, 2021

Jojo-Schmitz commented Dec 29, 2021 •

edited

Loading

yajo commented Dec 29, 2021

Jojo-Schmitz commented Dec 29, 2021

Fix realtime noteentry 8th crash (fixes #321716) #9593

Fix realtime noteentry 8th crash (fixes #321716) #9593

Conversation

fxthomas commented Oct 27, 2021 • edited Loading

fxthomas commented Oct 27, 2021

Jojo-Schmitz commented Oct 27, 2021 • edited Loading

fxthomas commented Oct 27, 2021 • edited Loading

Jojo-Schmitz commented Oct 28, 2021 • edited Loading

fxthomas commented Nov 6, 2021

Jojo-Schmitz commented Nov 15, 2021

yajo commented Dec 29, 2021

Jojo-Schmitz commented Dec 29, 2021 • edited Loading

yajo commented Dec 29, 2021

Jojo-Schmitz commented Dec 29, 2021 • edited Loading

yajo commented Dec 29, 2021

Jojo-Schmitz commented Dec 29, 2021

fxthomas commented Oct 27, 2021 •

edited

Loading

Jojo-Schmitz commented Oct 27, 2021 •

edited

Loading

fxthomas commented Oct 27, 2021 •

edited

Loading

Jojo-Schmitz commented Oct 28, 2021 •

edited

Loading

Jojo-Schmitz commented Dec 29, 2021 •

edited

Loading

Jojo-Schmitz commented Dec 29, 2021 •

edited

Loading