crash during mix disabling quantize

[ywwg]

Bug Description

Twice now (in many hours of DJing) I have had a crash when disabling quantize on deck 1. It is very, very hard to reproduce.

#0  0x0000561aa8d934cc in void std::__insertion_sort<QList<QSharedPointer<WaveformMark> >::iterator, __gnu_cxx::__ops::_Iter_less_iter>(QList<QSharedPointer<WaveformMark> >::iterator, QList<QSharedPointer<WaveformMark> >::iterator, __gnu_cxx::__ops::_Iter_less_iter) [clone .isra.0] ()
[Current thread is 1 (Thread 0x7f42e99d8380 (LWP 117578))]
(gdb) bt
#0  0x0000561aa8d934cc in void std::__insertion_sort<QList<QSharedPointer<WaveformMark> >::iterator, __gnu_cxx::__ops::_Iter_less_iter>(QList<QSharedPointer<WaveformMark> >::iterator, QList<QSharedPointer<WaveformMark> >::iterator, __gnu_cxx::__ops::_Iter_less_iter) [clone .isra.0] ()
#1  0x0000561aa8d97004 in WOverview::updateCues(QList<CuePointer> const&) ()
#2  0x0000561aa8d9799c in WOverview::onMarkChanged(double) ()
#3  0x00007f42f24f1793 in  () at /lib/x86_64-linux-gnu/libQt5Core.so.5
#4  0x0000561aa8790064 in ControlProxy::valueChanged(double) ()
#5  0x00007f42f24e741e in QObject::event(QEvent*) () at /lib/x86_64-linux-gnu/libQt5Core.so.5
#6  0x00007f42f556c713 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5
#7  0x00007f42f24b9e3a in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt5Core.so.5
#8  0x00007f42f24bcf27 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () at /lib/x86_64-linux-gnu/libQt5Core.so.5
#9  0x00007f42f2513a67 in  () at /lib/x86_64-linux-gnu/libQt5Core.so.5
#10 0x00007f42f145bd3b in g_main_context_dispatch () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#11 0x00007f42f14b1258 in  () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#12 0x00007f42f14593e3 in g_main_context_iteration () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#13 0x00007f42f25130b8 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /lib/x86_64-linux-gnu/libQt5Core.so.5
#14 0x00007f42f24b875b in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /lib/x86_64-linux-gnu/libQt5Core.so.5
#15 0x00007f42f24c0cf4 in QCoreApplication::exec() () at /lib/x86_64-linux-gnu/libQt5Core.so.5
#16 0x0000561aa86098b8 in main ()

so when I disabled quantize it updated the cues... and somewhere in there it got unhappy. This was a release build so no line numbers. Here is the only sort in that function: https://github.com/mixxxdj/mixxx/blob/main/src/widget/woverview.cpp#L455

I am wondering if somehow the marks became invalid? Could updatecues be called from more than one thread?

My only theory is that m_marksToRender is somehow changing during the call -- I don't see how else a simple qlist sort would crash. So for now I'm dropping a mutex in updateCues. (I don't see a simultaneous call to updatecues in the backtrace, there are no other interesting threads).

Version

HEAD

OS

Ubuntu 22.04

[ywwg]

I wrote a controller js hack to spam the quantize button and I reproduced the crash -- I'll see if I can get any more info

[ywwg]

the mark is becoming invalid during the sort operation

fuller backtrace:

#0  0x000055555577cd5c in QSharedPointer<WaveformMark>::data() const (this=0x200000000) at /usr/include/x86_64-linux-gnu/qt5/QtCore/qsharedpointer_impl.h:301
#1  0x000055555577c382 in QSharedPointer<WaveformMark>::operator->() const (this=0x200000000) at /usr/include/x86_64-linux-gnu/qt5/QtCore/qsharedpointer_impl.h:307
#2  0x0000555555aad838 in operator<(QSharedPointer<WaveformMark> const&, QSharedPointer<WaveformMark> const&) (lhs=..., rhs=...)
    at /home/owen/src/github/mixxx/src/waveform/renderers/waveformmark.h:128
#3  0x0000555555df6b2a in __gnu_cxx::__ops::_Val_less_iter::operator()<QSharedPointer<WaveformMark>, QList<QSharedPointer<WaveformMark> >::iterator>(QSharedPointer<WaveformMark>&, QList<QSharedPointer<WaveformMark> >::iterator) const (this=0x7fffffffd3c7, __val=..., __it=...) at /usr/include/c++/11/bits/predefined_ops.h:98
#4  0x0000555555df654c in std::__unguarded_linear_insert<QList<QSharedPointer<WaveformMark> >::iterator, __gnu_cxx::__ops::_Val_less_iter>(QList<QSharedPointer<WaveformMark> >::iterator, __gnu_cxx::__ops::_Val_less_iter) (__last=..., __comp=...) at /usr/include/c++/11/bits/stl_algo.h:1806
#5  0x0000555555df5934 in std::__insertion_sort<QList<QSharedPointer<WaveformMark> >::iterator, __gnu_cxx::__ops::_Iter_less_iter>(QList<QSharedPointer<WaveformMark> >::iterator, QList<QSharedPointer<WaveformMark> >::iterator, __gnu_cxx::__ops::_Iter_less_iter) (__first=..., __last=..., __comp=...) at /usr/include/c++/11/bits/stl_algo.h:1834
#6  0x0000555555df52ea in std::__final_insertion_sort<QList<QSharedPointer<WaveformMark> >::iterator, __gnu_cxx::__ops::_Iter_less_iter>(QList<QSharedPointer<WaveformMark> >::iterator, QList<QSharedPointer<WaveformMark> >::iterator, __gnu_cxx::__ops::_Iter_less_iter) (__first=..., __last=..., __comp=...) at /usr/include/c++/11/bits/stl_algo.h:1871
#7  0x0000555555df4daa in std::__sort<QList<QSharedPointer<WaveformMark> >::iterator, __gnu_cxx::__ops::_Iter_less_iter>(QList<QSharedPointer<WaveformMark> >::iterator, QList<QSharedPointer<WaveformMark> >::iterator, __gnu_cxx::__ops::_Iter_less_iter) (__first=..., __last=..., __comp=...) at /usr/include/c++/11/bits/stl_algo.h:1957
#8  0x0000555555df4891 in std::sort<QList<QSharedPointer<WaveformMark> >::iterator>(QList<QSharedPointer<WaveformMark> >::iterator, QList<QSharedPointer<WaveformMark> >::iterator)
    (__first=..., __last=...) at /usr/include/c++/11/bits/stl_algo.h:4842
#9  0x0000555555ded9c6 in WOverview::updateCues(QList<CuePointer> const&) (this=0x55556e63b4e0, loadedCues=...) at /home/owen/src/github/mixxx/src/widget/woverview.cpp:456

[ywwg]

the operator< is where the crash actually happens: rhs->getSamplePosition()

[ywwg]

discussion on mastodon, it looks like the QSharedPointer is invalid, not the thing it points to: https://furry.engineer/@huxley/110651195121630257

[daschuer]

This looks like a race condition.

When using std::sort the operator> must be reliable in a way that a > b > c and a > c else it will crash. This assumption is not correct here, because the WaveformMark has a CO pointer that can be changed from any thread at any time.

When you change two hotcues from the controller thread it may happen that the second one changes when the first one has triggered a sorting in the main thread.

I can reproduce the crash when returning a random value form the operator> overload.
It looks quite similar to your first back trace. However I am not able to see the dangling pointer access from your second back trace. Probably because of a different implementation of std::sort?

Conclusion, the code must store the positions as value and not as CO pointers.

[uklotzde]

I confused CuePointer with WaveformMarkPointer. For the latter there is an overloaded comparison operator.

This comparison operator should better be removed to prevent such mistakes based on false assumptions. Naively comparing these instances could cause many issues.

[uklotzde]

...otherwise you could have the idea to store it in a std::set ;) The compiler won't stop you. Even without race conditions the basic preconditions are violated.

[ywwg]

fixed in #11744