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

lp1956144 prevent segmentation fault when using midi.sendShortMsg and platform vnc. #4619

Closed
wants to merge 4,167 commits into from

Conversation

admindotnu
Copy link
Contributor

When starting mixxx from the command line, and using -platform vnc. midi.sendShortMsg is resulting in a segmentation fault. This is because of the used triggerActivity() function to bump the x11 screensaver timer to prevent the sceensaver to popup.

Workaround is to check the platform, and if its vnc, skip the triggerActivity() function.

Holzhaus and others added 30 commits November 17, 2021 13:28
Controller logging categories (with --controllerDebug support)
for compatibility with HidController::poll
Fix library track selection, save model state
WEffectChainPresetSelector: don't set elide padding in c++
CMake: remove obsolete references to files generated by scons
remove autogenerated code from ChannelMixer
ChannelMixer: Use range-based for loops
Fix isDirty() signature and semantics
rename maybeStartOrStopPolling to pollIfAnyControllersOpen
This allows for multiple CMake build directories, for example
build-qt5, build-qt6, build-vcpkg.
gitignore: change /build to /build*
WTrackMenu: add Remove From Disk action
WTrackMenu: Add missing feature to all features enum variant
@Holzhaus
Copy link
Member

Holzhaus commented Jan 14, 2022

Thanks for submitting a patch.

Unfortunately, this will make an expensive string comparison on every single received MIDI message.

If we want to add this workaround it should be added to the Screensaver inhibitor, where this check can be done once during startup, not every time the controller sends a packet (the platform won't change while mixxx is running).

@admindotnu
Copy link
Contributor Author

Thanks for the feedback, understand and agree on what you are saying.

I dont have the right experience to make such patch.

@Holzhaus
Copy link
Member

Could you check if the crash is fixed if you replace lines 177-178 in src/util/screensaver.cpp with:

if (display != nullptr) {
 ​    ​XResetScreenSaver​(display); 
 ​    ​XCloseDisplay​(display);
}

@admindotnu
Copy link
Contributor Author

Just recompiled, looks like your change is working too.

@Holzhaus
Copy link
Member

Just recompiled, looks like your change is working too.

Alright, can you update the PR? You do that with git commit --amend and then git push origin lp1956144 --force.

@Holzhaus
Copy link
Member

Also, please rebase on the 2.3 branch and edit the target branch of this PR:

https://github.com/mixxxdj/mixxx/wiki/Using%20Git#targeting-another-base-branch

@Holzhaus Holzhaus added this to the 2.3.2 milestone Jan 14, 2022
…idi.sendShortMsg is resulting in a segmentation fault. This is because of the used triggerActivity() function to bump the x11 screensaver timer to prevent the sceensaver to popup.

Workaround is to check the platform, and if its vnc, skip the triggerActivity() function.
@Holzhaus Holzhaus changed the base branch from main to 2.3 January 14, 2022 19:38
@admindotnu
Copy link
Contributor Author

I think its right now, same bug also appears in the main branche.

@Holzhaus
Copy link
Member

Also, please rebase on the 2.3 branch and edit the target branch of this PR:

https://github.com/mixxxdj/mixxx/wiki/Using%20Git#targeting-another-base-branch

I think its right now, same bug also appears in the main branche.

You didn't rebase yet, can you please run:

git fetch upstream
git rebase --onto upstream/2.3 upstream/main lp1956144

And then force-push again.

@ronso0
Copy link
Member

ronso0 commented Jan 14, 2022

...and then squash those 3 commits into 1?
And choose a more compact commit message?

Simply run
git rebase -i upstream/2.3

This will give you an editor view similiar to this with an explanation underneath

pick fafcefd5e9 librarycontrol: revise focus control
pick 6352881e4d librarycontrol: use FocusWidget for MoveVertical & GoToItem, revise emitKeyEvent
pick ed172faa47 librarycontrol: consolidate move.. and scroll.. functions

At the top is the first (oldest) commit. In that line, replace pick with r, or reword.
For all commits below you can replace pick with f, or fixup. This will ammend them into the first commit and drop their messages.
Now press the key combo labeled "Exit".
Now you get an editor view again and the chance to reword the first commit's message.
git will do its thing and print "Successfully rebase ..." in the terminal.

@daschuer
Copy link
Member

The rebased branch is here: #4635

@uklotzde uklotzde closed this Jan 18, 2022
@daschuer
Copy link
Member

@admindotnu I like to add your name to the contributor list in the Mixxx about box. Shall I use your full name or just your first name or your GitHub user name?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.