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

Fix crashes and deadlocks with previewing preset #3905

Merged
merged 4 commits into from Oct 29, 2017

Conversation

Projects
None yet
3 participants
@PhysSong
Member

PhysSong commented Oct 23, 2017

While investigating #3897, I found another bug.
Steps to reproduce:

  1. Click any preset on sidebar and hold it.
  2. Close LMMS with Alt+F4 or Ctrl+Q(while previewing).
  3. LMMS will crash.

Edit: clarified steps to reproduce.

Here's a part of a backtrace:

Thread 1 (Thread 0x7ffff7ee1900 (LWP 11830)):
#0  0x000000000053c74a in PresetPreviewPlayHandle::isFromTrack(Track const*) const ()
#1  0x0000000000526e8c in Mixer::removePlayHandlesOfTypes(Track*, unsigned char) ()
#2  0x00000000006275a7 in InstrumentTrack::silenceAllNotes(bool) ()
#3  0x000000000062c86d in InstrumentTrack::~InstrumentTrack() ()
#4  0x000000000062ca59 in InstrumentTrack::~InstrumentTrack() ()
#5  0x00000000005c54ba in TrackContainerView::clearAllTracks() ()
#6  0x0000000000552a5c in Song::clearProject() ()
#7  0x00000000004fc74f in LmmsCore::destroy() ()

So I added PlayHandle::TypePresetPreviewHandle in InstrumentTrack::silenceAllNotes(). I also add simple safety check code in PresetPreviewPlayHandle::isFromTrack().

Note: PresetPreviewPlayHandle::play() does nothing since #2586, so the code below is no longer needed. However, leaving it as-is looks fine for me.

if( m_previewPlayHandle != NULL )
{
if( !Engine::mixer()->addPlayHandle(
m_previewPlayHandle ) )
{
m_previewPlayHandle = NULL;
}
}

Edit: I added a commit which fixes #3456, per #3905 (comment).

@PhysSong PhysSong added this to the 1.2.0 milestone Oct 23, 2017

@gi0e5b06

This comment has been minimized.

Show comment
Hide comment
@gi0e5b06

gi0e5b06 Oct 23, 2017

Bug confirmed and proposed fix working. Approved
(and applied to my branch)

gi0e5b06 commented Oct 23, 2017

Bug confirmed and proposed fix working. Approved
(and applied to my branch)

@PhysSong PhysSong removed this from the 1.2.0 milestone Oct 24, 2017

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Oct 24, 2017

Member

I can't reproduce the bug on my machine. Linuxmint 17.3 at fbfcb43

Member

zonkmachine commented Oct 24, 2017

I can't reproduce the bug on my machine. Linuxmint 17.3 at fbfcb43

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Oct 25, 2017

Member

@zonkmachine I clarified the "steps to reproduce" section. I can confirm the issue with the same revision. Could you please test that again?

Member

PhysSong commented Oct 25, 2017

@zonkmachine I clarified the "steps to reproduce" section. I can confirm the issue with the same revision. Could you please test that again?

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Oct 25, 2017

Member

Ah, got it!

Bug verified. Testing the fix but it doesn't appear to shut down completely. Alt + F4.

Program received signal SIGINT, Interrupt.
syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
38	../sysdeps/unix/sysv/linux/x86_64/syscall.S: No such file or directory.
(gdb) bt full
#0  syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
No locals.
#1  0x00007ffff6858dd3 in _q_futex (val2=0, addr2=0x0, timeout=0x0, val=2, op=0, addr=0xef6da0) at thread/qmutex_unix.cpp:99
No locals.
#2  QMutexPrivate::wait (this=this@entry=0xef6da0, timeout=timeout@entry=-1) at thread/qmutex_unix.cpp:113
        ts = {tv_sec = 140737488343808, tv_nsec = 1}
        pts = 0x0
        timer = {t1 = 140737488343808, t2 = 27866800}
#3  0x00007ffff6855275 in QMutex::lockInternal (this=<optimized out>) at thread/qmutex.cpp:450
        maximumSpinTime = <optimized out>
        averageWaitTime = <optimized out>
        actualWaitTime = <optimized out>
        spinTime = 1000326
        d = 0xef6da0
        elapsedTimer = {t1 = 6344, t2 = 617602127}
        maximumSpinTime = <optimized out>
        spinTime = <optimized out>
#4  0x000000000054b43a in PreviewTrackContainer::lockData (this=0xef6d20) at /home/zonkmachine/builds/lmms/lmms/src/core/PresetPreviewPlayHandle.cpp:79
No locals.
#5  0x000000000054ad64 in PresetPreviewPlayHandle::~PresetPreviewPlayHandle (this=0x201c1a0, __in_chrg=<optimized out>)
    at /home/zonkmachine/builds/lmms/lmms/src/core/PresetPreviewPlayHandle.cpp:198
No locals.
#6  0x000000000054adf4 in PresetPreviewPlayHandle::~PresetPreviewPlayHandle (this=0x201c1a0, __in_chrg=<optimized out>)
    at /home/zonkmachine/builds/lmms/lmms/src/core/PresetPreviewPlayHandle.cpp:207
No locals.
Member

zonkmachine commented Oct 25, 2017

Ah, got it!

Bug verified. Testing the fix but it doesn't appear to shut down completely. Alt + F4.

Program received signal SIGINT, Interrupt.
syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
38	../sysdeps/unix/sysv/linux/x86_64/syscall.S: No such file or directory.
(gdb) bt full
#0  syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
No locals.
#1  0x00007ffff6858dd3 in _q_futex (val2=0, addr2=0x0, timeout=0x0, val=2, op=0, addr=0xef6da0) at thread/qmutex_unix.cpp:99
No locals.
#2  QMutexPrivate::wait (this=this@entry=0xef6da0, timeout=timeout@entry=-1) at thread/qmutex_unix.cpp:113
        ts = {tv_sec = 140737488343808, tv_nsec = 1}
        pts = 0x0
        timer = {t1 = 140737488343808, t2 = 27866800}
#3  0x00007ffff6855275 in QMutex::lockInternal (this=<optimized out>) at thread/qmutex.cpp:450
        maximumSpinTime = <optimized out>
        averageWaitTime = <optimized out>
        actualWaitTime = <optimized out>
        spinTime = 1000326
        d = 0xef6da0
        elapsedTimer = {t1 = 6344, t2 = 617602127}
        maximumSpinTime = <optimized out>
        spinTime = <optimized out>
#4  0x000000000054b43a in PreviewTrackContainer::lockData (this=0xef6d20) at /home/zonkmachine/builds/lmms/lmms/src/core/PresetPreviewPlayHandle.cpp:79
No locals.
#5  0x000000000054ad64 in PresetPreviewPlayHandle::~PresetPreviewPlayHandle (this=0x201c1a0, __in_chrg=<optimized out>)
    at /home/zonkmachine/builds/lmms/lmms/src/core/PresetPreviewPlayHandle.cpp:198
No locals.
#6  0x000000000054adf4 in PresetPreviewPlayHandle::~PresetPreviewPlayHandle (this=0x201c1a0, __in_chrg=<optimized out>)
    at /home/zonkmachine/builds/lmms/lmms/src/core/PresetPreviewPlayHandle.cpp:207
No locals.
@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Oct 25, 2017

Member

Okay. There is a releated bug #3456. I'll add a fix for that. It would help fixing new issue.

Member

PhysSong commented Oct 25, 2017

Okay. There is a releated bug #3456. I'll add a fix for that. It would help fixing new issue.

@gi0e5b06

This comment has been minimized.

Show comment
Hide comment
@gi0e5b06

gi0e5b06 Oct 25, 2017

@PhysSong Also double-click in the file browser. The second click happens outside the previewing and so add the controller. I fixed that too.

gi0e5b06 commented Oct 25, 2017

@PhysSong Also double-click in the file browser. The second click happens outside the previewing and so add the controller. I fixed that too.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Oct 25, 2017

Member

The second commit fixes #3456.
Note: lock***()-style functions should be replaced with safer QMutexLocker later anywhere available.

Edit: done by bdf4884.

Member

PhysSong commented Oct 25, 2017

The second commit fixes #3456.
Note: lock***()-style functions should be replaced with safer QMutexLocker later anywhere available.

Edit: done by bdf4884.

@PhysSong PhysSong changed the title from Fix crash when closing while previewing preset to Fix crashes and deadlocks with previewing preset Oct 25, 2017

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Oct 25, 2017

Member

The second commit fixes #3456.

Both bugs and their fixes confirmed.

Member

zonkmachine commented Oct 25, 2017

The second commit fixes #3456.

Both bugs and their fixes confirmed.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Oct 27, 2017

Member

This PR seems to work good. However, I will add some commits to remove potential chances to generate errors because I've changed some lock-related parts. I think using atomic pointers can be a solution.

Member

PhysSong commented Oct 27, 2017

This PR seems to work good. However, I will add some commits to remove potential chances to generate errors because I've changed some lock-related parts. I think using atomic pointers can be a solution.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Oct 28, 2017

Member

Note: lock***()-style functions should be replaced with safer QMutexLocker later anywhere available.

I will add some commits to remove potential chances to generate errors because I've changed some lock-related parts.

Done by bdf4884.

Member

PhysSong commented Oct 28, 2017

Note: lock***()-style functions should be replaced with safer QMutexLocker later anywhere available.

I will add some commits to remove potential chances to generate errors because I've changed some lock-related parts.

Done by bdf4884.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Oct 28, 2017

Member

I've retested this PR and it still works like a charm. I also retested the bug in question and it seem the crash on exit doesn't happen if the project is playing, only when previewing and the song is stopped.

Member

zonkmachine commented Oct 28, 2017

I've retested this PR and it still works like a charm. I also retested the bug in question and it seem the crash on exit doesn't happen if the project is playing, only when previewing and the song is stopped.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Oct 29, 2017

Member

Merge?

Member

PhysSong commented Oct 29, 2017

Merge?

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Oct 29, 2017

Member

Totally...

Member

zonkmachine commented Oct 29, 2017

Totally...

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Oct 29, 2017

Member

Okay. I'll merge it soon. 😄

Member

PhysSong commented Oct 29, 2017

Okay. I'll merge it soon. 😄

@PhysSong PhysSong merged commit 60e9b2f into LMMS:stable-1.2 Oct 29, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@PhysSong PhysSong deleted the PhysSong:previewcrash branch Oct 29, 2017

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