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

Removing a SampleTCO while it plays will crash lmms (master and latest rc) #4980

Closed
Reflexe opened this issue May 18, 2019 · 7 comments · Fixed by #4982
Closed

Removing a SampleTCO while it plays will crash lmms (master and latest rc) #4980

Reflexe opened this issue May 18, 2019 · 7 comments · Fixed by #4982
Assignees
Labels

Comments

@Reflexe
Copy link
Member

Reflexe commented May 18, 2019

Steps to reproduce

  • Open lmms
  • load a sampletrack (doubleclick on a sample track and select an audio file)
  • play it
  • remove it while it plays (middle mouse button)
  • see if lmms crashes

Please verify it so I'll work on a solution.

@BaraMGB
Copy link
Contributor

BaraMGB commented May 18, 2019

It crashes only with the middle mouse button. If I use the context menu, it doesn't crash.

@BaraMGB BaraMGB added the bug label May 18, 2019
@PhysSong
Copy link
Member

Could you test the latest stable-1.2? I think it might have been fixed recently.

@Reflexe
Copy link
Member Author

Reflexe commented May 19, 2019

@PhysSong It is probably a race condition since I had to do that a couple of times in order to reproduce it. ‎This is the crash's stacktrace:

1  ??                                                                                                      0x7ffff603aadb 
2  SampleBuffer::play(float ( *) [2], SampleBuffer::handleState *, short, float, SampleBuffer::LoopMode)   0x5555556b3c1f 
3  SamplePlayHandle::play(float ( *) [2])                                                                  0x5555556b72ff 
4  MixerWorkerThread::JobQueue::run()                                                                      0x55555569a5d8 
5  MixerWorkerThread::startAndWaitForJobs()                                                                0x55555569a76f 
6  Mixer::renderNextBuffer()                                                                               0x5555556983aa 
7  Mixer::fifoWriter::run()                                                                                0x555555698914 
8  ??                                                                                                      0x7ffff71a7feb 
9  start_thread                                                                                            0x7ffff7f7ea92 
10 clone                                                                                                   0x7ffff5bfdcd3 

The problem

sharedObject::unref might delete SampleBuffer, but we don't protect it with requestChangesInModel.

@PhysSong
Copy link
Member

I found that TrackContentObjectView::remove may result in deleting TCOs while the mixer is rendering audio. So, I was working on the other approach than #4982:

diff --git a/src/core/Track.cpp b/src/core/Track.cpp
index 7a04ded95..eb8ebb3dc 100644
--- a/src/core/Track.cpp
+++ b/src/core/Track.cpp
@@ -422,7 +422,9 @@ void TrackContentObjectView::remove()
 
        // delete ourself
        close();
-       m_tco->deleteLater();
+       Engine::mixer()->requestChangeInModel();
+       delete m_tco;
+       Engine::mixer()->doneChangeInModel();
 }

#4982 looks fine, and I don't mind if any of them is accepted.

@Reflexe
Copy link
Member Author

Reflexe commented May 19, 2019

I think that your solution might actually be better, but for some reason, lmms still crashes with this solution; I need the research ti fouther.

We also have a problem with this function being called twice in a row; that will simply cause a double-free.

@Reflexe
Copy link
Member Author

Reflexe commented May 27, 2019

OK, for now, I'm going to merge it for stable-1.2. The recording branch has a better fix for that and will target master.

@Reflexe Reflexe self-assigned this May 27, 2019
@Reflexe Reflexe reopened this May 27, 2019
@Reflexe Reflexe removed this from the 1.2.0 milestone May 27, 2019
@BaraMGB
Copy link
Contributor

BaraMGB commented Jun 5, 2019

@Reflexe is there a reason why you reopened this?

@Reflexe Reflexe closed this as completed Jul 26, 2019
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