Crash when clicking on "Remove Channel"

[badosu]

If you click on "remove channel" when right clicking a FX Channel, sometimes it segfaults.

This is a bug of the same class than #1584, i.e. the object that creates the context menu is the same that is deleted before the internal method returns. Sometimes it deletes before, sometimes later, what causes this seemingly random segfault.

See #1678 and #1625 for details on a bug of the same class.

[badosu]

Upon some research I've found this: http://stackoverflow.com/questions/17943486/removing-itself-from-layout-using-context-menu.

However it should work just with subclasses of QObject which is not the case for FxChannel.

[tresf]

However it should work just with subclasses of QObject which is not the case for FxChannel.

But you're not removing it from FxChannel, you're removing it from its QWidget parent, right? Have you tried deleteLater(), there's a good chance it will still work. From what I'm reading, it queues removal on the event queue, which is exactly what is needed.

[badosu]

Well, I tried changing:

// From this
delete m_fxChannels[index];

// To this
m_fxChannels[index]->deleteLater();

But as I said, FxChannel is not a QObject and this does not work.

[tresf]

@badosu sorry that's what I get for speculating on the code rather than reading it.

Can you show me the stack trace that causes this? We may need to mark them for deletion via a signal, but I'm having a hard time finding the stack of events inside the code that causes this.

-Tres

[badosu]

Can you show me the stack trace that causes this?

Sure, just a sec.

We may need to mark them for deletion via a signal, but I'm having a hard time finding the stack of events inside the code that causes this.

As I said, this is a somewhat apparently random segfault, somewhat the processing time of the function + the alignment of the stars make it delete the variable before returning to the event that originated it's deletion. Maybe this can not even happen on some machines.

I suspect this may even be the cause of segfaults @DanWin experienced when he developed the "Remove unused channel" feature.

[badosu]

Is there any point in creating anew issue for this?

In this case at least I was able to point what is the cause of the crash and how to fix it. If you are able to pinpoint how to reproduce the crash and even find what's the deallocation (the most frequent cause) that's causing it we have higher possibilities of fixing it.

The issue with these kind of crashes is that they are utterly difficult to trace, some people experience them, some people not. Some people experience them frequently, some not. It's a developer's hell.

[badosu]

@Spekular Please don't delete!

Every bit of information on crashes is important, I just tried to offer you our perspective.

If your bug report is not a duplicate of one that is already filed this help us a lot tracking how the software is stable.

[Spekular]

@badosu I was just trying to avoid sidetracking this issue (I failed). I posted here as a replacement for messaging you/tres. I'll create a new issue for this crash I suppose, but I don't think our conversation here is necessarily relevant, so by deleting it this issue convo is neater. Will you delete your messages too? If you reallywant to document this I guess here's a summary:
ME: Had a crash I can't reproduce, do I still make an issue?
badosu: See message above message above
ME: Ok, I won't make an issue
badosue: see message above

[DanWin]

Yes, it is indeed the same issue I had when implementing the "remove unused channels" function. It relies on this function and I can reproduce it just by creating a few (20 or more) channels and holding the delete key or using my new function. At some random point deletion of the channel will cause a segfault.
A backtrace can be found here: #1545 (comment)

[tresf]

Ok... I'm starting to see this (again) thanks for the information.

Can we cast it back to QVector element and delete later then?

-Tres

[midi-pascal]

Was this fixed in master branch?
I get a crash each time I click "Remove unused channels" on projects imported from 1.0.3.
Ubuntu 12.04, master branch.
The only workaround I found is to edit the mpp by hand :-)

[badosu]

@midi-pascal This PR fixes it: #1707

However, this solution relies on QObject, a better approach needs to be implemented.

[midi-pascal]

@badosu Thanks for the hint!
I did not check this PR since it is closed.
I will live with the manual editing of the project files for now :)

[badosu]

@midi-pascal I just took a look, and actually that PR contains the new approach that should fix this issue without introducing QObject inheritance, but I couldn't make it work.

[Wallacoloo]

Btw this doesn't appear to fix all issues. The "remove unused channels" feature still results in memory access errors, though many seem to be due to synchronization errors.

Here's one example (valgrind):

==6909== Invalid read of size 4
==6909==    at 0x4D4DE8: Mixer::peakValueLeft(float (*) [2], int) (Mixer.cpp:500)
==6909==    by 0x4B751F: FxChannel::doProcessing() (FxMixer.cpp:178)
==6909==    by 0x4D8822: process (ThreadableJob.h:74)
==6909==    by 0x4D8822: MixerWorkerThread::JobQueue::run() (MixerWorkerThread.cpp:75)
==6909==    by 0x4D898E: MixerWorkerThread::startAndWaitForJobs() (MixerWorkerThread.cpp:148)
==6909==    by 0x4B8FBC: FxMixer::masterMix(float (*) [2]) (FxMixer.cpp:590)
==6909==    by 0x4D756F: Mixer::renderNextBuffer() (Mixer.cpp:434)
==6909==    by 0x4D77E3: Mixer::fifoWriter::run() (Mixer.cpp:950)
==6909==    by 0x60126FE: ??? (in /usr/lib/x86_64-linux-gnu/libQtCore.so.4.8.6)
==6909==    by 0x4E3E6A9: start_thread (pthread_create.c:333)
==6909==    by 0x86C5EEC: clone (clone.S:109)
==6909==  Address 0x32bb0c00 is 0 bytes inside a block of size 2,048 free'd
==6909==    at 0x4C2D5B3: operator delete[](void*) (vg_replace_malloc.c:542)
==6909==    by 0x4B90D5: FxChannel::~FxChannel() (FxMixer.cpp:84)
==6909==    by 0x4B9669: ~FxChannel (FxMixer.cpp:85)
==6909==    by 0x4B9669: FxMixer::deleteChannel(int) (FxMixer.cpp:324)
==6909==    by 0x526DED: FxMixerView::deleteChannel(int) (FxMixerView.cpp:377)
==6909==    by 0x527439: FxMixerView::deleteUnusedChannels() (FxMixerView.cpp:439)
==6909==    by 0x61321EB: QMetaObject::activate(QObject*, QMetaObject const*, int, void**) (in /usr/lib/x86_64-linux-gnu/libQtCore.so.4.8.6)

In this case, the Fx Channel has already had a render job scheduled, but the actual FxChannel gets deleted while the job is still in the queue, resulting in an error once the job is processed.

Then you get another error after all that:

==6909== Invalid write of size 8
==6909==    at 0x4C31E47: memset (vg_replace_strmem.c:1094)
==6909==    by 0x4B8CCE: FxMixer::masterMix(float (*) [2]) (FxMixer.cpp:616)
==6909==    by 0x4D756F: Mixer::renderNextBuffer() (Mixer.cpp:434)
==6909==    by 0x4D77E3: Mixer::fifoWriter::run() (Mixer.cpp:950)
==6909==    by 0x60126FE: ??? (in /usr/lib/x86_64-linux-gnu/libQtCore.so.4.8.6)
==6909==    by 0x4E3E6A9: start_thread (pthread_create.c:333)
==6909==    by 0x86C5EEC: clone (clone.S:109)
==6909==  Address 0x32bb0c00 is 0 bytes inside a block of size 2,048 free'd
==6909==    at 0x4C2D5B3: operator delete[](void*) (vg_replace_malloc.c:542)
==6909==    by 0x4B90D5: FxChannel::~FxChannel() (FxMixer.cpp:84)

This one appears to be because we lack any synchronization mechanism for the actual m_fxChannels array (i.e. we only perform synchronization on the individual FxChannels, but not the array as a whole, so adding and removing channel pointers from this array in one thread causes undefined behavior in another).

[badosu]

@Wallacoloo You're right, that's why I said I was not able to make this work. This PR still has some convo from the QObject::deleteLater approach, sorry for the confusion.

[Fastigium]

I'm going to assign myself to this one, as I just ran into it on my make-music-with-master-bug-hunt. This really should be fixed before releasing. I just hope I'll be able to find an acceptable solution where others have failed 😅

[Fastigium]

PR to fix this available at #2675, testing welcome!

[Umcaruje]

@Fastigium @badosu is this fixed by #2675?

[Fastigium]

@Umcaruje I'm gonna go with "yes", unless you know of anyone who's been able to produce a channel removal crash after that PR was merged 😊

[IvanMaldonado]

Not sure if useful, but I had the exact same Problem but didn't say it because the issue was already there, that PR fixed it for me.

[Umcaruje]

K 👍 Closing.

[badosu]

Congrats @Fastigium!