don't try and set bpm on invalid m_pBeats pointers #1893
Conversation
src/engine/bpmcontrol.cpp
Outdated
| @@ -219,6 +219,9 @@ void BpmControl::slotTapFilter(double averageLength, int numSamples) { | |||
| if (numSamples < 4) | |||
| return; | |||
|
|
|||
| if (!m_pBeats) | |||
There was a problem hiding this comment.
Nice catch! Could you grab a reference to the shared pointer first so that it can't be deleted between this line and the setBpm call below?
auto pBeats = m_pBeats;
if (!pBeats) {
return;
}
...
pBeats->setBpm(averageBpm);There was a problem hiding this comment.
I can do that. Other usages in this file don't do that though. If that's necessary, there might be some race conditions.
|
This is a pretty serious bug! Thanks for sending a fix. Could you rebase this onto the 2.1 branch? I'd like to include it in our next stable release (2.1.6). |
If no track is loaded and the user presses the bpm_tap button, mixxx will crash. https://bugs.launchpad.net/mixxx/+bug/1801844
|
Rebased, I also updated the merge target to branch 2.1, not sure if that's necessary or not. |
|
The race condition between checking for Null and using the pointer seems to be really an issue. It effects m_pBPM and m_pTrack as well. We need to check from which thread both is set and then fix at least functins called from other threads. I am afraid other engine controls suffer the same issue. This may explain the crash recently reported in the forums wen ejecting a track. |
|
I have reported a follow-up bug https://bugs.launchpad.net/bugs/1801874 |
|
LGTM |
|
LGTM as well. Could you please sign the Mixxx contributor agreement? This gives us permission to distribute your change with Mixxx. |
|
AFAIK I have already signed it. |
Thanks |
I think it might be worth renaming methods based on their execution context to be easier to read. Or just deleting the engine and starting over with a design that doesn't have multiple threads touching shared state ;). |
If you're volunteering to do the work... :) |
|
What would be involved in a rewrite like this? How feasible is doing something like that? |
I am! |
A metric crap-tonne of work, in my estimation ;). |
|
snapshotting controlobject state at the start of every audio callback?? 🙇♂️ |
If no track is loaded and the user presses the bpm_tap button, mixxx will crash. I didn't test this, or write a unit test for it. Scons has suddenly started complaining my compiler does not support c++11, but it looks like this is the cause of the crash.
https://bugs.launchpad.net/mixxx/+bug/1801844