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

Effect crasher fix #4707

Merged
merged 22 commits into from
Nov 3, 2022
Merged

Effect crasher fix #4707

merged 22 commits into from
Nov 3, 2022

Conversation

daschuer
Copy link
Member

This PR fixes a crasher due to unsafe handling of the effect state lifetime and concurrent access to m_chainStatusForChannelMatrix in EngineEffectChain::deleteStatesForInputChannel()
https://bugs.launchpad.net/mixxx/+bug/1775497

This has been introduced in Mixxx 2.1 via #1468.
Under normal conditions the crash does not happen, but it happens whenever the order of execution of the main and engine thread is disturbed due to an overload condition. A locking that is able to prevent this is missing.
The delete attempt from in the main thread might be delayed and the engine is still accessing the memory. This causes a crash. The other issue is that the m_chainStatusForChannelMatrix is written concurrently from the engine and the master thread which must not happen. It must only change state at the beginning of the new engine callback. #4487

This PR also fixes https://bugs.launchpad.net/mixxx/+bug/1966607 where the effect was not ramped up/down during loading a new track.

To solve both issues, enabling and disabling of an effect is no done in the same way regardless of which control disables the effect. The effect state is initialized on demand, but it is keep until the effect is unloaded under all conditions.
Before it was kept if the effect was disabled via the effect enable button, but it was deleted when the effect was disabled via a routing button.

This allows finally to delete all the brittle code around EngineEffectChain::deleteStatesForInputChannel().

This is big PR and the crasher happens only rarely, I have decided to build this PR on top of 2.4, which allows us to test it well during beta phase. A backport to 2.3 is not simple, because of the recent effect refactoring.

I have tried to build self contained commits, that can be reviewed if the whole PR

@daschuer daschuer added this to the 2.4.0 milestone Jun 7, 2022
@daschuer
Copy link
Member Author

This one merges cleanly again.

@daschuer daschuer added needs review and removed ui labels Jul 19, 2022
@ronso0
Copy link
Member

ronso0 commented Jul 19, 2022

The following tests FAILED:
288 - EngineSyncTest.EjectTrackSyncRemains (SEGFAULT)
363 - HotcueControlTest.SavedLoopUnloadTrackWhileActive (SEGFAULT)
530 - AdjustReplayGainTest.AdjustReplayGainUpdatesPregain (SEGFAULT)
Errors while running CTest

Who is able to review this?

@github-actions github-actions bot added the ui label Jul 22, 2022
@daschuer
Copy link
Member Author

This PR has discovered a missing null check introduced here:
#4771

@fayaaz
Copy link
Contributor

fayaaz commented Oct 10, 2022

Tested this PR vs main on multiple machines: the effects toggling does not cause a crash with this PR, while I have had multiple crashes on main branch toggling FX on or off.
It is however hard to reproduce the segfault reliably, but I will report back if a crash does happen with this branch after a few more hours of playing with it.

Copy link
Member

@ywwg ywwg left a comment

Choose a reason for hiding this comment

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

This looks great, much cleaner! I agree that we can merge to 2.4 and do some testing and fixing. I made a couple of notes.

inputChannelHandleNumber++;
}
m_channelStateMatrix.clear();
//if (kEffectDebugOutput) {
Copy link
Member

Choose a reason for hiding this comment

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

uncomment debug guards

for (const ChannelHandleAndGroup& outputChannel :
std::as_const(m_registeredOutputChannels)) {
outputChannelStates[outputChannel.handle().handle()].reset(
Copy link
Member

Choose a reason for hiding this comment

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

what is handle().handle()? Can we make that more meaningful?


void EngineEffect::deleteStatesForInputChannel(ChannelHandle inputChannel) {
m_pProcessor->deleteStatesForInputChannel(inputChannel);
//TODO: get actual configuration of engine
Copy link
Member

Choose a reason for hiding this comment

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

I think this TODO needs to be addressed?

@daschuer
Copy link
Member Author

Done.

@daschuer daschuer requested a review from ywwg October 28, 2022 18:32
@ywwg
Copy link
Member

ywwg commented Nov 3, 2022

ok to merge

@ywwg ywwg merged commit f96d168 into mixxxdj:main Nov 3, 2022
@daschuer daschuer deleted the effects_refactoring_3 branch November 16, 2022 08:22
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.

4 participants