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

Crash when previewing sample #5736

Closed
zonkmachine opened this issue Oct 26, 2020 · 16 comments · Fixed by #5764
Closed

Crash when previewing sample #5736

zonkmachine opened this issue Oct 26, 2020 · 16 comments · Fixed by #5764
Labels
Milestone

Comments

@zonkmachine
Copy link
Member

zonkmachine commented Oct 26, 2020

Got a crash when previewing a sample when running lmms in gdb.
-DCMAKE_BUILD_TYPE=Debug -DWANT_DEBUG_FPE=True
lmms version - master, 7808e0b with one unrelated commit on top.

Method to replicate

  • Start master, debug build in gdb.
  • Open demo project loop.mmp loop.mmp.zip
  • Press play in the Song Editor
  • Demo a sample from the side menu
  • Press paus in the Song Editor
  • Demo a sample from the side menu
  • Crash

Logs

Click to expand
Thread 1 "lmms" received signal SIGSEGV, Segmentation fault.
0x00005555557b305f in Mixer::removePlayHandle (this=0x555555c920c0, _ph=0x55555a4d34f0) at /home/zonkmachine/builds/lmms/src/core/Mixer.cpp:705
705		if( _ph->affinityMatters() &&
(gdb) bt
#0  0x00005555557b305f in Mixer::removePlayHandle(PlayHandle*) (this=0x555555c920c0, _ph=0x55555a4d34f0)
    at /home/zonkmachine/builds/lmms/src/core/Mixer.cpp:705
#1  0x000055555586a7f9 in FileBrowserTreeWidget::stopPreview() (this=0x5555598e4650) at /home/zonkmachine/builds/lmms/src/gui/FileBrowser.cpp:607
#2  0x000055555586a13b in FileBrowserTreeWidget::previewFileItem(FileItem*) (this=0x5555598e4650, file=0x555559911170)
    at /home/zonkmachine/builds/lmms/src/gui/FileBrowser.cpp:543
#3  0x000055555586a0c2 in FileBrowserTreeWidget::mousePressEvent(QMouseEvent*) (this=0x5555598e4650, me=0x7fffffffd3e0)
    at /home/zonkmachine/builds/lmms/src/gui/FileBrowser.cpp:532
#4  0x00007ffff7a972d1 in QWidget::event(QEvent*) (this=this@entry=0x5555598e4650, event=event@entry=0x7fffffffd3e0) at kernel/qwidget.cpp:8959
#5  0x00007ffff7b44d52 in QFrame::event(QEvent*) (this=0x5555598e4650, e=0x7fffffffd3e0) at widgets/qframe.cpp:550
#6  0x00007ffff7cc9482 in QAbstractItemView::viewportEvent(QEvent*) (this=this@entry=0x5555598e4650, event=event@entry=0x7fffffffd3e0)
    at itemviews/qabstractitemview.cpp:1750
#7  0x00007ffff7d3762f in QTreeView::viewportEvent(QEvent*) (this=0x5555598e4650, event=0x7fffffffd3e0) at itemviews/qtreeview.cpp:1318
#8  0x00007ffff66e664b in QCoreApplicationPrivate::sendThroughObjectEventFilters(QObject*, QEvent*) (event=, receiver=)
    at kernel/qcoreapplication.cpp:1214
#9  QCoreApplicationPrivate::sendThroughObjectEventFilters(QObject*, QEvent*) (receiver=receiver@entry=0x5555598f3850, event=event@entry=0x7fffffffd3e0)
    at kernel/qcoreapplication.cpp:1203
#10 0x00007ffff7a54a55 in QApplicationPrivate::notify_helper(QObject*, QEvent*)
    (this=this@entry=0x555555b12f70, receiver=receiver@entry=0x5555598f3850, e=e@entry=0x7fffffffd3e0) at kernel/qapplication.cpp:3694
#11 0x00007ffff7a5e343 in QApplication::notify(QObject*, QEvent*) (this=, receiver=0x5555598f3850, e=0x7fffffffd3e0)
    at kernel/qapplication.cpp:3160
#12 0x00007ffff66e693a in QCoreApplication::notifyInternal2(QObject*, QEvent*) (receiver=0x5555598f3850, event=0x7fffffffd3e0)
    at ../../include/QtCore/../../src/corelib/kernel/qobject.h:142
#13 0x00007ffff7a5d457 in QApplicationPrivate::sendMouseEvent(QWidget*, QMouseEvent*, QWidget*, QWidget*, QWidget**, QPointer&, bool, bool)
    (receiver=receiver@entry=0x5555598f3850, event=event@entry=0x7fffffffd3e0, alienWidget=alienWidget@entry=0x5555598f3850, nativeWidget=0x555559501330, buttonDown=buttonDown@entry=0x7ffff7f848d0 , lastMouseReceiver=..., spontaneous=true, onlyDispatchEnterLeave=false)
    at kernel/qapplication.cpp:2646
#14 0x00007ffff7ab335d in QWidgetWindow::handleMouseEvent(QMouseEvent*) (this=0x555559e13700, event=0x7fffffffd860)
    at /usr/include/c++/9/bits/atomic_base.h:413
#15 0x00007ffff7ab61ec in QWidgetWindow::event(QEvent*) (event=0x7fffffffd860, this=0x555559e13700) at kernel/qwidgetwindow.cpp:289
#16 QWidgetWindow::event(QEvent*) (this=0x555559e13700, event=0x7fffffffd860) at kernel/qwidgetwindow.cpp:232
#17 0x00007ffff7a54a66 in QApplicationPrivate::notify_helper(QObject*, QEvent*)
    (this=this@entry=0x555555b12f70, receiver=receiver@entry=0x555559e13700, e=e@entry=0x7fffffffd860) at kernel/qapplication.cpp:3700
#18 0x00007ffff7a5e0f0 in QApplication::notify(QObject*, QEvent*) (this=0x555555b12f50, receiver=0x555559e13700, e=0x7fffffffd860)
    at kernel/qapplication.cpp:3446
#19 0x00007ffff66e693a in QCoreApplication::notifyInternal2(QObject*, QEvent*) (receiver=0x555559e13700, event=0x7fffffffd860)
    at ../../include/QtCore/../../src/corelib/kernel/qobject.h:142
#20 0x00007ffff6acf7d3 in QGuiApplicationPrivate::processMouseEvent(QWindowSystemInterfacePrivate::MouseEvent*) (e=e@entry=0x55555a818520)
    at kernel/qguiapplication.cpp:2107
#21 0x00007ffff6ad110b in QGuiApplicationPrivate::processWindowSystemEvent(QWindowSystemInterfacePrivate::WindowSystemEvent*) (e=e@entry=0x55555a818520)
    at kernel/qguiapplication.cpp:1842
#22 0x00007ffff6aab35b in QWindowSystemInterface::sendWindowSystemEvents(QFlags) (flags=flags@entry=...)
    at kernel/qwindowsysteminterface.cpp:1151
#23 0x00007ffff214032e in xcbSourceDispatch(GSource*, GSourceFunc, gpointer) (source=) at qxcbeventdispatcher.cpp:105
#24 0x00007ffff5455fbd in g_main_context_dispatch () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
#25 0x00007ffff5456240 in  () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
#26 0x00007ffff54562e3 in g_main_context_iteration () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
#27 0x00007ffff673e565 in QEventDispatcherGlib::processEvents(QFlags) (this=0x555555bcd490, flags=...)
    at kernel/qeventdispatcher_glib.cpp:422
#28 0x00007ffff66e54db in QEventLoop::exec(QFlags) (this=this@entry=0x7fffffffdc00, flags=..., flags@entry=...)
    at ../../include/QtCore/../../src/corelib/global/qflags.h:140
#29 0x00007ffff66ed246 in QCoreApplication::exec() () at ../../include/QtCore/../../src/corelib/global/qflags.h:120
#30 0x000055555571bbd4 in main(int, char**) (argc=1, argv=0x7fffffffdf78) at /home/zonkmachine/builds/lmms/src/core/main.cpp:991
(gdb) bt full
#0  0x00005555557b305f in Mixer::removePlayHandle(PlayHandle*) (this=0x555555c920c0, _ph=0x55555a4d34f0)
    at /home/zonkmachine/builds/lmms/src/core/Mixer.cpp:705
#1  0x000055555586a7f9 in FileBrowserTreeWidget::stopPreview() (this=0x5555598e4650) at /home/zonkmachine/builds/lmms/src/gui/FileBrowser.cpp:607
        previewLocker = {val = 93825063077529}
#2  0x000055555586a13b in FileBrowserTreeWidget::previewFileItem(FileItem*) (this=0x5555598e4650, file=0x555559911170)
    at /home/zonkmachine/builds/lmms/src/gui/FileBrowser.cpp:543
        previewLocker = {val = 93825063077529}
        newPPH = 0x5555598f1a30
        fileName = {static null = {}, d = 0x7fffffffcc40}
        ext = {static null = {}, d = 0x555559911170}
#3  0x000055555586a0c2 in FileBrowserTreeWidget::mousePressEvent(QMouseEvent*) (this=0x5555598e4650, me=0x7fffffffd3e0)
    at /home/zonkmachine/builds/lmms/src/gui/FileBrowser.cpp:532
        i = 0x555559911170
        f = 0x555559911170
#4  0x00007ffff7a972d1 in QWidget::event(QEvent*) (this=this@entry=0x5555598e4650, event=event@entry=0x7fffffffd3e0) at kernel/qwidget.cpp:8959
        d = 0x5555598f1a30
#5  0x00007ffff7b44d52 in QFrame::event(QEvent*) (this=0x5555598e4650, e=0x7fffffffd3e0) at widgets/qframe.cpp:550
        result = 
#6  0x00007ffff7cc9482 in QAbstractItemView::viewportEvent(QEvent*) (this=this@entry=0x5555598e4650, event=event@entry=0x7fffffffd3e0)
    at itemviews/qabstractitemview.cpp:1750
        d = 
#7  0x00007ffff7d3762f in QTreeView::viewportEvent(QEvent*) (this=0x5555598e4650, event=0x7fffffffd3e0) at itemviews/qtreeview.cpp:1318
        d = 
#8  0x00007ffff66e664b in QCoreApplicationPrivate::sendThroughObjectEventFilters(QObject*, QEvent*) (event=, receiver=)
    at kernel/qcoreapplication.cpp:1214
        obj = 
        i = 0
#9  QCoreApplicationPrivate::sendThroughObjectEventFilters(QObject*, QEvent*) (receiver=receiver@entry=0x5555598f3850, event=event@entry=0x7fffffffd3e0)
    at kernel/qcoreapplication.cpp:1203
#10 0x00007ffff7a54a55 in QApplicationPrivate::notify_helper(QObject*, QEvent*)
    (this=this@entry=0x555555b12f70, receiver=receiver@entry=0x5555598f3850, e=e@entry=0x7fffffffd3e0) at kernel/qapplication.cpp:3694
        consumed = false
        filtered = false
#11 0x00007ffff7a5e343 in QApplication::notify(QObject*, QEvent*) (this=, receiver=0x5555598f3850, e=0x7fffffffd3e0)
    at kernel/qapplication.cpp:3160
        me = 
              { = { = {_vptr.QEvent = 0x7ffff6f7d200 , static staticMetaObject = {d = {superdata = 0x0, stringdata = 0x7ffff688cde0 , data = 0x7ffff688c820 , static_metacall = 0x0, relatedMetaObjects = 0x0, extradata = 0x0}}, d = 0x0, t = 2, posted = 0, spont = 1, m_accept = 1, reserved = 2876}, modState = {i = 0}, ts = 5706568}, l = {xp = 136, yp = 567}, w = {xp = 172, yp = 728}, s = {xp = 172, yp = 751}, b = Qt::LeftButton, mouseState = {i = 1}, caps = 0, velocity = {v = {0, 0}}}
        w = 0x5555598f3850
        mouse = 0x7fffffffd3e0
        eventAccepted = 
        relpos = {xp = 136, yp = 567}
        pw = {wp = {d = 0x55555a48a690, value = 0x5555598f3850}}
        d = 
        res = false
#12 0x00007ffff66e693a in QCoreApplication::notifyInternal2(QObject*, QEvent*) (receiver=0x5555598f3850, event=0x7fffffffd3e0)
    at ../../include/QtCore/../../src/corelib/kernel/qobject.h:142
        selfRequired = true
        result = false
        cbdata = {0x5555598f3850, 0x7fffffffd3e0, 0x7fffffffd1df}
--Type  for more, q to quit, c to continue without paging--c
        d = 
        threadData = 0x555555b130f0
        scopeLevelCounter = {threadData = 0x555555b130f0}
#13 0x00007ffff7a5d457 in QApplicationPrivate::sendMouseEvent(QWidget*, QMouseEvent*, QWidget*, QWidget*, QWidget**, QPointer&, bool, bool) (receiver=receiver@entry=0x5555598f3850, event=event@entry=0x7fffffffd3e0, alienWidget=alienWidget@entry=0x5555598f3850, nativeWidget=0x555559501330, buttonDown=buttonDown@entry=0x7ffff7f848d0 , lastMouseReceiver=..., spontaneous=true, onlyDispatchEnterLeave=false) at kernel/qapplication.cpp:2646
        graphicsWidget = false
        widgetUnderMouse = 
        wasLeaveAfterRelease = true
        result = true
#14 0x00007ffff7ab335d in QWidgetWindow::handleMouseEvent(QMouseEvent*) (this=0x555559e13700, event=0x7fffffffd860) at /usr/include/c++/9/bits/atomic_base.h:413
        translated = { = { = {_vptr.QEvent = 0x7ffff6f7d200 , static staticMetaObject = {d = {superdata = 0x0, stringdata = 0x7ffff688cde0 , data = 0x7ffff688c820 , static_metacall = 0x0, relatedMetaObjects = 0x0, extradata = 0x0}}, d = 0x0, t = 2, posted = 0, spont = 1, m_accept = 1, reserved = 2870}, modState = {i = 0}, ts = 5706568}, l = {xp = 136, yp = 567}, w = {xp = 172, yp = 728}, s = {xp = 172, yp = 751}, b = Qt::LeftButton, mouseState = {i = 1}, caps = 0, velocity = {v = {0, 0}}}
        contextMenuTrigger = QEvent::MouseButtonPress
        widget = 0x5555598f3850
        mapped = {xp = 136, yp = 567}
        initialPress = 
        receiver = 0x5555598f3850

Edit: method to replicate issue updated

@Spekular
Copy link
Member

#5427 is suspect, I suppose, but I can't say I know why it would cause this to start happening. One thing I find suspicious is that the mixer seems to act as if it owns play handles added via addPlayHandle (it does else delete handle;), but FileBrowser also keeps a reference to the play handle pointer. Maybe FileBrowser ends up passing a deleted play handle into removePlayHandle? I'm not sure how best to fix this though (if it's even the cause of the bug):

  • If FileBrowser doesn't keep the play handle pointer it can't stop the preview when it needs to.
  • If switching to a less raw pointer would help, Mixer would probably need a lot of rewriting(?).
  • If removePlayHandle should handle this, how do you detect that the target of a pointer isn't there anymore?

@zonkmachine
Copy link
Member Author

Yes. It looks like #5427 is the cause of this. I can't make lmms crash like this in the commit prior to this one.

@zonkmachine
Copy link
Member Author

A more precise method to replicate this is.

  • Load demo loop
  • Press play in the Song Editor
  • Demo a sample from the side menu
  • Press paus in the Song Editor
  • Demo a sample from the side menu
  • Crash

@Spekular
Copy link
Member

Marking for 1.3 since it's a regression, but I'd love thoughts on the options above before I try to tackle this. I'm not particularly familiar with pointers.

@Spekular Spekular added this to the 1.3 milestone Oct 29, 2020
@PhysSong
Copy link
Member

I got the following output with Valgrind:

Click to expand
==13564== Invalid read of size 8
==13564==    at 0x580CD0: Mixer::removePlayHandle(PlayHandle*) (Mixer.cpp:705)
==13564==    by 0x5EE9E3: FileBrowserTreeWidget::stopPreview() (FileBrowser.cpp:607)
==13564==    by 0x5F2FA9: FileBrowserTreeWidget::previewFileItem(FileItem*) (FileBrowser.cpp:543)
==13564==    by 0x4A49F9D: QWidget::event(QEvent*) (in /usr/lib64/libQt5Widgets.so.5.15.1)
==13564==    by 0x4AF242D: QFrame::event(QEvent*) (in /usr/lib64/libQt5Widgets.so.5.15.1)
==13564==    by 0x60F4DC2: QCoreApplicationPrivate::sendThroughObjectEventFilters(QObject*, QEvent*) (in /usr/lib64/libQt5Core.so.5.15.1)
==13564==    by 0x4A0A13D: QApplicationPrivate::notify_helper(QObject*, QEvent*) (in /usr/lib64/libQt5Widgets.so.5.15.1)
==13564==    by 0x4A1103A: QApplication::notify(QObject*, QEvent*) (in /usr/lib64/libQt5Widgets.so.5.15.1)
==13564==    by 0x60F5059: QCoreApplication::notifyInternal2(QObject*, QEvent*) (in /usr/lib64/libQt5Core.so.5.15.1)
==13564==    by 0x4A10065: QApplicationPrivate::sendMouseEvent(QWidget*, QMouseEvent*, QWidget*, QWidget*, QWidget**, QPointer<QWidget>&, bool, bool) (in /usr/lib64/libQt5Widgets.so.5.15.1)
==13564==    by 0x4A62000: ??? (in /usr/lib64/libQt5Widgets.so.5.15.1)
==13564==    by 0x4A6550D: ??? (in /usr/lib64/libQt5Widgets.so.5.15.1)
==13564==  Address 0xb9a03f0 is 0 bytes inside a block of size 320 free'd
==13564==    at 0x483B08B: operator delete(void*, unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==13564==    by 0x57FFD3: Mixer::renderNextBuffer() (Mixer.cpp:405)
==13564==    by 0x580C5B: Mixer::fifoWriter::run() (Mixer.cpp:1274)
==13564==    by 0x5F15910: ??? (in /usr/lib64/libQt5Core.so.5.15.1)
==13564==    by 0x4874EB0: start_thread (in /lib64/libpthread-2.32.so)
==13564==    by 0x66D5CCE: clone (in /lib64/libc-2.32.so)
==13564==  Block was alloc'd at
==13564==    at 0x4839DEF: operator new(unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==13564==    by 0x5F33B4: FileBrowserTreeWidget::previewFileItem(FileItem*) (FileBrowser.cpp:559)
==13564==    by 0x4A49F9D: QWidget::event(QEvent*) (in /usr/lib64/libQt5Widgets.so.5.15.1)
==13564==    by 0x4AF242D: QFrame::event(QEvent*) (in /usr/lib64/libQt5Widgets.so.5.15.1)
==13564==    by 0x60F4DC2: QCoreApplicationPrivate::sendThroughObjectEventFilters(QObject*, QEvent*) (in /usr/lib64/libQt5Core.so.5.15.1)
==13564==    by 0x4A0A13D: QApplicationPrivate::notify_helper(QObject*, QEvent*) (in /usr/lib64/libQt5Widgets.so.5.15.1)
==13564==    by 0x4A1103A: QApplication::notify(QObject*, QEvent*) (in /usr/lib64/libQt5Widgets.so.5.15.1)
==13564==    by 0x60F5059: QCoreApplication::notifyInternal2(QObject*, QEvent*) (in /usr/lib64/libQt5Core.so.5.15.1)
==13564==    by 0x4A10065: QApplicationPrivate::sendMouseEvent(QWidget*, QMouseEvent*, QWidget*, QWidget*, QWidget**, QPointer<QWidget>&, bool, bool) (in /usr/lib64/libQt5Widgets.so.5.15.1)
==13564==    by 0x4A62000: ??? (in /usr/lib64/libQt5Widgets.so.5.15.1)
==13564==    by 0x4A6550D: ??? (in /usr/lib64/libQt5Widgets.so.5.15.1)
==13564==    by 0x4A0A14E: QApplicationPrivate::notify_helper(QObject*, QEvent*) (in /usr/lib64/libQt5Widgets.so.5.15.1)

#5736 (comment) seems to make sense according to the output.

@Spekular
Copy link
Member

So, does addPlayHandle need to take a shared pointer instead (+ cascading changes in every class the handle is passed to, seemingly many) or is there a less intrusive solution to this?

@PhysSong
Copy link
Member

I will try reverting #5427 locally and check if the same error existed before.

@PhysSong
Copy link
Member

I could get a similar error, but very rarely. It seems like an underlying issue has been there for a long time.

@PhysSong
Copy link
Member

Found a much more reliable way to trigger the bug:

  1. Play the song
  2. Preview any sample
  3. Stop the song
  4. Try to preview any sample

@PhysSong
Copy link
Member

the mixer seems to act as if it owns play handles added via addPlayHandle

Mixer checks the thread affinity of PlayHandles, but Mixer::clear() and Mixer::removePlayHandlesOfTypes() ignore that. The latter is irrelevant in this context, but the former is called by Song::stop(). See #5427 (review).

@Spekular
Copy link
Member

Spekular commented Nov 2, 2020

Adding && (*it)->affinity() == QThread::currentThread() to the if statement in clearInternal() in Mixer.cpp seems to prevent the crash. Is there a reason this function didn't check affinity before?

Also, I feel like I should add || !(*it)->affinityMatters() to the condition as well, but conditions in other functions are confusing me. For example, removePlayHandle checks if affinity matters AND matches the current thread, supposedly to avoid deleting play handles from other threads. However:

  • If affinity doesn't matter, the play handle won't be removed. But if affinity doesn't matter, shouldn't it be okay to delete the handle? I would expect to check that the thread matches OR affinity doesn't matter
  • Even if this if statement turns out to be false, the play handle will be added to "m_playHandlesToRemove". This is the same thing that causes the crash when clearInternal is called, so it seems that it shouldn't be done to other thread's play handles.

@PhysSong
Copy link
Member

PhysSong commented Nov 3, 2020

That's quite complicated. I think a number of functions in Mixer should be changed in order to prevent thread-related issues like #4009.
BTW, you're safe to set m_previewPlayHandle to null after calling setDoneMayReturnTrue(true), at least for now.

@Spekular
Copy link
Member

Spekular commented Nov 3, 2020

BTW, you're safe to set m_previewPlayHandle to null after calling setDoneMayReturnTrue(true)

Doing so leads to several overlapping sample previews if you preview a new sample while an old one is auto-playing, so I wouldn't consider it a real fix.

What about removing the logic that makes samples keep playing after mouse release? It's not really a fix either, but until I looked at this code I never really understood why samples sometimes stopped their preview and sonetines didn't. To me it's not particularly useful or intuitive, but I suppose it'd be good to get more opinions on this.

Spekular added a commit that referenced this issue Nov 3, 2020
Less than ideal, but should close #5736.
@Spekular
Copy link
Member

Spekular commented Nov 3, 2020

After some discussion in Discord, it's been decided that a new tag should be made once this is fixed.

Discussion summary (hopefully accurate):

Tresf: We should tag a new version ASAP so master builds can go up on the downloads page
Me: Let's wait until the crash I caused is fixed so we don't publish a version that crashes every two seconds

@PhysSong
Copy link
Member

PhysSong commented Nov 4, 2020

Question: LMMS currently removes all PlayHandles when stopping playing the song. Should the sample preview stop when stopping the song or not? Proper fix depends on the choice of behavior.

@Spekular
Copy link
Member

Spekular commented Nov 4, 2020

With the current state of our stop button I don't think it should. If we had a "silence everything" function it should stop previews IMO, but the stop button doesn't do that right now (it allows reverbs, note releases, and similar things to keep going).

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

Successfully merging a pull request may close this issue.

3 participants