Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MU3] Solve a crash when score contains a vbox which is not on top of a page. #7778

Merged
merged 1 commit into from Mar 23, 2021

Conversation

njvdberg
Copy link
Contributor

@njvdberg njvdberg commented Mar 21, 2021

When a VBox found, a VerticalGapData object is created but if this VBox wasn't the first element on a page, the application crashed because the constructor was looking for a Spacer on a non-existing staff.

PR #7779 is similar for master.

  • 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

@@ -5120,7 +5120,7 @@ VerticalGapData::VerticalGapData(bool first, System *sys, Staff *st, SysStaff *s
_normalisedSpacing = system->y() + (sysStaff ? sysStaff->y() : 0.0) - y;
_maxActualSpacing = system->score()->styleP(Sid::maxStaffSpread);

Spacer* spacer { sys->upSpacer(st->idx(), nextSpacer) };
Spacer* spacer { st ? sys->upSpacer(st->idx(), nextSpacer) : nullptr };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe (sys && st)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, there is always a system, a VBox is also a system. However, a VBox doesn't have any SysStaff's and that caused the crash (if the VBox was somewhere in the middle of the page).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe it'd never happen (sounds like 'famous last words' though), but that check won't harm either

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a test is required, this will be a wrong place since system is already dereferenced several time times before (lines 5540, 5543 and 5544). Also before calling the constructor the system argument is dereferenced already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

system is used before, not sys. Except in the constructor, but who knows what system(sys) does if sys is a nullptr.

but yes, probably not need to check, not there at least

anyway: how to reproduce the issue prior to this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in the constructor and with system(sys) the member system is initialized with the argument sys, so both are the same.

The issue can be reproduced creating a VBox somewhere after the first system.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We, no, I can't reproduce the issue.
And a mix of using system and sys looks wrong to me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You tried to reproduce with 3.6.2 of the 3.x branch? The crash occurs in the 3.x branch (and master) only, not in 3.6.2 .

Strictly speaking, mix of sys and system is not "wrong" but it certainly is confusing and is corrected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I tried to reproduce in 3.6.2, so that crash got to be a later regression.

And yes, 'wrong' is a bit strong, but certainly confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a later regression due to PR #7529

@vpereverzev vpereverzev merged commit 14c94dd into musescore:3.x Mar 23, 2021
@njvdberg njvdberg deleted the spacer-crash-3x branch May 15, 2021 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants