Fix crashes on deleting an FX channel #2675

Fastigium · 2016-03-14T13:28:30Z

This PR fixes three crashes that would happen when deleting an FX channel. Now that #2669 has been merged, using LMMS with this PR should be pretty much random-crash-free. At least I can't produce any 😋. So go on, go wild, do everything crazy you can come up with concerning the FX mixer, and post what you did plus the backtrace if you still manage to make it crash

Fastigium · 2016-03-14T15:39:27Z

DeRobyJ reported an issue with this PR, namely that changing the FX channel on an instrument doesn't actually change the channel it sends to. Looking into that now.

Fastigium · 2016-03-14T16:22:40Z

Right, rebased to take care of that. More testing welcome!

zonkmachine · 2016-03-14T17:47:14Z

Test.
1 - My heaviest track that used to crash frequently up until your merge from earlier today and has been playing now continuously for two hours.
2 - Add 250 channels.
3 - Connect three of the channels to separate LFO's sweeping up and down fast.
4 - 'Remove unused channels'

No crash. Job well done!

Umcaruje · 2016-03-14T21:46:40Z

@Fastigium I have a carla-related error:

[108/232] Building CXX object plugins/carlabase/CMakeFiles/carlabase.dir/carla.cpp.o
FAILED: /usr/bin/c++   -DPLUGIN_NAME=carlabase -DQT_CORE_LIB -DQT_GUI_LIB -DQT_WIDGETS_LIB -DQT_XML_LIB -Dcarlabase_EXPORTS -fno-exceptions -Wall -Werror=unused-function -Wno-sign-compare -Wno-strict-overflow -Wno-array-bounds  -fPIC -DPIC -g -DLMMS_DEBUG -fPIC -I/usr/include/qt5 -I/usr/include/qt5/QtCore -I/usr/lib/x86_64-linux-gnu/qt5/mkspecs/linux-g++-64 -I/usr/include/qt5/QtGui -I/usr/include/qt5/QtWidgets -I/usr/include/qt5/QtXml -I/usr/include/carla -I/usr/include/carla/includes -Iplugins/carlabase -I. -I../include -I../src/gui -MMD -MT plugins/carlabase/CMakeFiles/carlabase.dir/carla.cpp.o -MF "plugins/carlabase/CMakeFiles/carlabase.dir/carla.cpp.o.d" -o plugins/carlabase/CMakeFiles/carlabase.dir/carla.cpp.o -c ../plugins/carlabase/carla.cpp
In file included from ../plugins/carlabase/carla.cpp:33:0:
../include/InstrumentPlayHandle.h: In member function ‘virtual void InstrumentPlayHandle::play(sample_t (*)[2])’:
../include/InstrumentPlayHandle.h:59:54: error: range-based ‘for’ loops are not allowed in C++98 mode
    for( const NotePlayHandle * constNotePlayHandle : nphv )
                                                      ^

Though that's more related to #2669.

tresf · 2016-03-14T22:58:56Z

../include/InstrumentPlayHandle.h

We'll probably have to add -std=c++0x to carlabase as well, since it links against InstrumentPlayhandle.h

Umcaruje · 2016-03-14T23:19:29Z

I can confirm this fixes #2667. I'll do even more extensive crash testing tomorrow.

Fastigium · 2016-03-15T08:56:37Z

@zonkmachine @Umcaruje Yay for the crashlessness (that's a word now 😁)!

I have a carla-related error

Woops, I'll look into that. @tresf I guess Travis doesn't build the carla plugin at the moment?

Fastigium · 2016-03-15T09:51:10Z

@Umcaruje PR that should make carla compile again: #2677

tresf · 2016-03-15T13:45:45Z

@tresf I guess Travis doesn't build the carla plugin at the moment?

No, we epic failed in our attempts... #2643

Umcaruje · 2016-03-15T22:56:59Z

@Fastigium I did a stress test, managed to crash it again:

I did a pretty serious bash with a LFO moving channels extremely fast and moving channels and removing channels. I needed to rebase this branch to carlacompile though. I'll do a fresh build and continue the stress testing tommorow.

ASSERT: "isDetached()" in file /usr/include/qt5/QtCore/qvector.h, line 325

Program received signal SIGABRT, Aborted.
[Switching to Thread 0x7fff937fe700 (LWP 10336)]
0x00007ffff3fcdcc9 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
56  ../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt full
#0  0x00007ffff3fcdcc9 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
        resultvar = 0
        pid = 10324
        selftid = 10336
#1  0x00007ffff3fd10d8 in __GI_abort () at abort.c:89
        save_stage = 2
        act = {__sigaction_handler = {sa_handler = 0x7fff937fe9c0, sa_sigaction = 0x7fff937fe9c0}, sa_mask = {__val = {
              140735668020992, 140735668018368, 140737351947607, 140733193388037, 0, 140735668018064, 140737286626600, 256, 
              140735668018368, 19219728, 140737351976213, 1, 140737287563405, 0, 0, 140735407980576}}, 
          sa_flags = -1820334336, sa_restorer = 0x712f65726f437451}
        sigs = {__val = {32, 0 <repeats 15 times>}}
#2  0x00007ffff759b655 in QMessageLogger::fatal(char const*, ...) const () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
No symbol table info available.
#3  0x00007ffff7598454 in qt_assert(char const*, char const*, int) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
No symbol table info available.
#4  0x000000000052082e in QVector<FxChannel*>::detach (this=0xfd2c48) at /usr/include/qt5/QtCore/qvector.h:325
No locals.
#5  0x000000000051f8f2 in QVector<FxChannel*>::data (this=0xfd2c48) at /usr/include/qt5/QtCore/qvector.h:128
No locals.
#6  0x000000000051f2aa in QVector<FxChannel*>::operator[] (this=0xfd2c48, i=71) at /usr/include/qt5/QtCore/qvector.h:370
No locals.
#7  0x000000000051df83 in FxMixer::masterMix (this=0xfd2c00, _buf=0x11b7580) at ../src/core/FxMixer.cpp:618
        i = 71
        fpp = 256
        volBuf = 0x0
        v = 1
#8  0x000000000053917c in Mixer::renderNextBuffer (this=0x1183640) at ../src/core/Mixer.cpp:441
        playModeSupportsMetronome = true
        it_rem = {i = 0x7fff840030d0}
        fxMixer = 0xfd2c00
        last_metro_pos = {<MidiTime> = {m_ticks = -1, static s_ticksPerTact = 192}, m_timeLine = 0x0, m_currentFrame = 0}
        song = 0x123f8c0
        currentPlayMode = Song::Mode_PlaySong
        p = {<MidiTime> = {m_ticks = 1897, static s_ticksPerTact = 192}, m_timeLine = 0x197e070, 
          m_currentFrame = 188,273438}
#9  0x000000000053a789 in Mixer::fifoWriter::run (this=0x11ee120) at ../src/core/Mixer.cpp:986
---Type <return> to continue, or q <return> to quit---
        buffer = 0x7fff84007860
        b = 0x11b7580
        frames = 256
#10 0x00007ffff75a9233 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
No symbol table info available.
#11 0x00007ffff7bc4182 in start_thread (arg=0x7fff937fe700) at pthread_create.c:312
        __res = <optimized out>
        pd = 0x7fff937fe700
        now = <optimized out>
        unwind_buf = {cancel_jmp_buf = {{jmp_buf = {140735668020992, 956274728558233560, 1, 0, 140735668021696, 
                140735668020992, -956319809574434856, -956256583975729192}, mask_was_saved = 0}}, priv = {pad = {0x0, 0x0, 
              0x0, 0x0}, data = {prev = 0x0, cleanup = 0x0, canceltype = 0}}}
        not_first_call = <optimized out>
        pagesize_m1 = <optimized out>
        sp = <optimized out>
        freesize = <optimized out>
        __PRETTY_FUNCTION__ = "start_thread"
#12 0x00007ffff409147d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
No locals.

My build may be stale though.

Fastigium · 2016-03-16T07:38:42Z

@Umcaruje Thanks for taking the time for such a stress test! carlacompile shouldn't influence the crashing or the line numbers, so I don't think your build is stale. The backtrace you posted implies that there is still a copy constructor being used somewhere. I have some hunting to do 🐶!

Fastigium · 2016-03-16T08:10:42Z

Well, who'da thunk! The copy constructor is still used on FxMixer::m_fxChannels when right-clicking the FX channel number on an instrument! So the fast LFOs are fine, but the right-click-menu needed to connect them caused the crash 😆. I'll kick out the copy constructor and post a PR 👢

Fastigium · 2016-03-16T09:03:36Z

@Umcaruje The PR turned out to be so trivial that I went ahead and merged it after getting all greens from Travis. If you have time for another stress test, please update to the latest master and pull this PR on top of it; it'll get you both carlacompile and copyconstructorcleanup.

In general, there seem to be many uses of the copy constructor left in the code base. I think I may be able to automate the search for them, but I'm not sure if it's worth it. Getting rid of all of them is bound to be a pain in the backside; I/we would need to check for each individual use if the copy constructor isn't there for a reason.

If users keep running into more isDetached() assert crashes, then a total sweep of copy constructor use could be made. If not, a partial sweep for performance-critical code only would prevent deep copies and speed things up somewhat. It's something to think about.

eagles051387 · 2016-03-16T17:39:10Z

If you do need to do a wide sweep of the copy constructor might want to
keep in mind git grep. some google fu would be needed as I dont know off
the top of my head what command you would issue to have it search the
entire code base.

Jonathan Aquilina

On Wed, Mar 16, 2016 at 10:03 AM, Fastigium [email protected]
wrote:

@Umcaruje https://github.com/Umcaruje The PR turned out to be so
trivial that I went ahead and merged it after getting all greens from
Travis. If you have time for another stress test, please update to the
latest master and pull this PR on top of it; it'll get you both
carlacompile and copyconstructorcleanup.

In general, there seem to be many uses of the copy constructor left in the
code base. I think I may be able to automate the search for them, but I'm
not sure if it's worth it. Getting rid of all of them is bound to be a pain
in the backside; I/we would need to check for each individual use if the
copy constructor isn't there for a reason.

If users keep running into more isDetached() assert crashes, then a total
sweep of copy constructor use could be made. If not, a partial sweep for
performance-critical code only would prevent deep copies and speed things
up somewhat. It's something to think about.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#2675 (comment)

Fastigium · 2016-03-16T18:38:32Z

@eagles051387 Thanks for the pointer, I hadn't heard of git grep yet 😊. If I understand its manual correctly it is very similar to normal grep, but git-aware. The problem in this case is that the copy constructor is not used explicitly, but is hidden in other language constructs. For example, foreach (int i, intList) uses the copy constructor QList<int>::QList(QList<int> &v), but that is not actually visible in the code 😠. This function also uses a copy constructor:

inline QVector<FxChannel *> fxChannels() const
{
    return m_fxChannels;
}

Again, you don't see it, but under the hood, it uses it. Foreach was easy to grep for, so that has been done, but grepping for functions like the example above is less practical. Plus, there are probably more code constructs that copy secretly.

My plan for a sweep is therefore based on a different observation: if an object method is never referenced in the code, it is optimized out. After I got rid of the copy constructor that caused Umcaruje's latest crash, that copy constructor disappeared from the gdb breakpoint tab completion. So I plan to reverse that mechanism and somehow find out on which code lines the copy constructors that are still in the compiled code are invoked. It's not trivial, but it should be doable 😅, and it should eliminate the copy constructor entirely

eagles051387 · 2016-03-17T08:43:46Z

Im sure there is a way you can git grep for that particular string that you
mentioned which is part of the copy constructor regardless of where its
found in the code. At least with this you will know what files have it
otherwise you will spend hours if not days goign through one file at a time.

Jonathan Aquilina

On Wed, Mar 16, 2016 at 7:38 PM, Fastigium [email protected] wrote:

@eagles051387 https://github.com/eagles051387 Thanks for the pointer, I
hadn't heard of git grep yet [image: 😊]. If I understand its manual
correctly it is very similar to normal grep, but git-aware. The problem
in this case is that the copy constructor is not used explicitly, but is
hidden in other language constructs. For example, foreach (int i, intList)
uses the copy constructor QList::QList(QList &v), but that is
not actually visible in the code [image: 😠]. This function also
uses a copy constructor:

inline QVector<FxChannel *> fxChannels() const
{
return m_fxChannels;
}

Again, you don't see it, but under the hood, it uses it. Foreach was easy
to grep for, so that has been done
#2669, but grepping for functions like
the example above is less practical. Plus, there are probably more code
constructs that copy secretly.

My plan for a sweep is therefore based on a different observation: if an
object method is never referenced in the code, it is optimized out. After I
got rid of the copy constructor that caused Umcaruje's latest crash, that
copy constructor disappeared from the gdb breakpoint tab completion. So I
plan to reverse that mechanism and somehow find out on which code lines the
copy constructors that are still in the compiled code are invoked. It's not
trivial, but it should be doable [image: 😅], and it should
eliminate the copy constructor entirely

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#2675 (comment)

Fastigium · 2016-03-17T12:59:28Z

@eagles051387 Thanks for trying to help, but I think I'll manage. This goes beyond what git grep is intended for. Besides, the way I propose does not involve going through files one at a time.

eagles051387 · 2016-03-17T13:02:04Z

Not a problem. Sorry if I potentially derailed the discussion of the
Original Post of the bug report :(

Jonathan Aquilina

On Thu, Mar 17, 2016 at 1:59 PM, Fastigium [email protected] wrote:

@eagles051387 https://github.com/eagles051387 Thanks for trying to
help, but I think I'll manage. This goes beyond what git grep is intended
for. Besides, the way I propose does not involve going through files one at
a time.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#2675 (comment)

Fastigium · 2016-03-17T13:05:33Z

@eagles051387 No problem 😉

Anyway, to get back on-topic: this PR seems ready to merge to me with one exception. It now introduces a mixer lock in GUI code. That does not help the efforts to separate GUI and core. I'll see if the lock can be moved to core without reintroducing channel removal crashes.

Lock the mixer before performing a channel delete to prevent any race conditions causing a crash. Also, update the audioport FX channel when an InstrumentTrack's FX channel is changed to prevent the audioport mixing to a nonexistent channel.

…crash In Qt, it is not safe to delete a QObject inside a signal emitted by that QObject. This happened with FxLine when removing an FX channel using the context menu. This commit changes that by using deleteLater() instead of delete on the FxLine. It also hides the FxLine to prevent a ghost of it being drawn when deleting the last non-master FX channel.

Fastigium · 2016-03-17T18:25:52Z

Rebased to move the mixer lock from gui/FxMixerView.cpp to core/FxMixer.cpp. Seems just as crash-resistant here. Can anyone look over the code and give 👍/👎? If 👍, I say we merge.

Umcaruje · 2016-03-17T18:39:09Z

I'll do another stress test once I get back home.

Fastigium · 2016-03-17T18:54:11Z

@Umcaruje Woah, you have time for that besides the mountain of BoL3 submissions? Respect 😁! And good luck with that mountain btw!

Umcaruje · 2016-03-17T21:14:29Z

Ok, I did an extencive stress test:

I had 4 LFO's going at high speeds x 100 with different phases, hooked up to the FX channel selectors of 4 instruments in a random project file:

Then I added around 80ish channels, and started moving them around, removing them, sending a channel to 10 other and then removing it, I also removed unused channels. I repeated the process a couple of times. It makes an interesting lightshow in the FX mixer:

All this lasted around 5-10 mins. During all that time I haven't experienced a single crash or a hang. LMMS didn't even slow down and I was using SDL as my sound driver on Linux.

So I have to thank you @Fastigium for the wonderful work you've done. I'm going to close #2192 cuz that seems to be fixed by your merged commits.

tresf · 2016-03-17T21:55:28Z

So I have to thank you @Fastigium for the wonderful work you've done. I'm going to close #2192 cuz that seems to be fixed by your merged commits.

Agreed, kudos @Fastigium. Sounds like we're getting ready for an RC2. 👍

Umcaruje · 2016-03-17T21:59:55Z

Sounds like we're getting ready for an RC2. 👍

And its gonna be really stable 👍

Fastigium · 2016-03-18T08:30:46Z

Awesome! Thanks for the testing and encouragement 😊! I'll leave this PR open a little while longer in case someone has remarks about the code.

musikBear · 2016-03-18T11:11:10Z

If a binary is made, i will test win32

Umcaruje · 2016-03-18T11:21:07Z

If a binary is made, i will test win32

You can make it yourself, its not rocket science. All you need is a VM and some good will:
https://github.com/LMMS/lmms/wiki/Compiling-lmms-(Windows)

Just instead of cloning the main repo, clone Fastigium's repo.

Fastigium · 2016-03-26T08:26:35Z

It's been more than a week without comments, so I'm merging this 😊. @tresf I think I'd prefer to complete the work on the export crashes before RC2 is released. I'm considering a slightly larger overhaul of the audio interfaces code, and if that happens it'd be nice to have it in an RC before it ends up in a stable release.

Umcaruje · 2016-03-26T08:55:26Z

👍

tresf · 2016-03-26T14:04:07Z

@Fastigium I read the explanation in the other thread and it makes perfect sense thanks. 👍

This was referenced Mar 14, 2016

Crash when clicking on "Remove Channel" #1679

Closed

Qt5: crash when removing a channel that sends to another channel #2667

Closed

Fastigium force-pushed the deletechannelfix branch from c972a04 to 1e7c74e Compare March 14, 2016 16:21

Fastigium added 2 commits March 17, 2016 19:14

Fix two crashes when deleting FX channels

32b7e04

Lock the mixer before performing a channel delete to prevent any race conditions causing a crash. Also, update the audioport FX channel when an InstrumentTrack's FX channel is changed to prevent the audioport mixing to a nonexistent channel.

Fastigium force-pushed the deletechannelfix branch from 1e7c74e to 82055a9 Compare March 17, 2016 18:15

Fastigium mentioned this pull request Mar 25, 2016

Render the mixer levels in a dB FS scale #2672

Merged

Fastigium merged commit d1739ce into LMMS:master Mar 26, 2016

Fastigium mentioned this pull request Mar 27, 2016

Crackling sound in master branch #2708

Closed

tresf mentioned this pull request May 23, 2016

Lmms 1.2.0-RC1 64bit crashing and export problems #2792

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crashes on deleting an FX channel #2675

Fix crashes on deleting an FX channel #2675

Fastigium commented Mar 14, 2016

Fastigium commented Mar 14, 2016

Fastigium commented Mar 14, 2016

zonkmachine commented Mar 14, 2016

Umcaruje commented Mar 14, 2016

tresf commented Mar 14, 2016

Umcaruje commented Mar 14, 2016

Fastigium commented Mar 15, 2016

Fastigium commented Mar 15, 2016

tresf commented Mar 15, 2016

Umcaruje commented Mar 15, 2016

Fastigium commented Mar 16, 2016

Fastigium commented Mar 16, 2016

Fastigium commented Mar 16, 2016

eagles051387 commented Mar 16, 2016

Fastigium commented Mar 16, 2016

eagles051387 commented Mar 17, 2016

Fastigium commented Mar 17, 2016

eagles051387 commented Mar 17, 2016

Fastigium commented Mar 17, 2016

Fastigium commented Mar 17, 2016

Umcaruje commented Mar 17, 2016 via email

Fastigium commented Mar 17, 2016

Umcaruje commented Mar 17, 2016

tresf commented Mar 17, 2016

Umcaruje commented Mar 17, 2016

Fastigium commented Mar 18, 2016

musikBear commented Mar 18, 2016

Umcaruje commented Mar 18, 2016

Fastigium commented Mar 26, 2016

Umcaruje commented Mar 26, 2016

tresf commented Mar 26, 2016

Fix crashes on deleting an FX channel #2675

Fix crashes on deleting an FX channel #2675

Conversation

Fastigium commented Mar 14, 2016

Fastigium commented Mar 14, 2016

Fastigium commented Mar 14, 2016

zonkmachine commented Mar 14, 2016

Umcaruje commented Mar 14, 2016

tresf commented Mar 14, 2016

Umcaruje commented Mar 14, 2016

Fastigium commented Mar 15, 2016

Fastigium commented Mar 15, 2016

tresf commented Mar 15, 2016

Umcaruje commented Mar 15, 2016

Fastigium commented Mar 16, 2016

Fastigium commented Mar 16, 2016

Fastigium commented Mar 16, 2016

eagles051387 commented Mar 16, 2016

Fastigium commented Mar 16, 2016

eagles051387 commented Mar 17, 2016

Fastigium commented Mar 17, 2016

eagles051387 commented Mar 17, 2016

Fastigium commented Mar 17, 2016

Fastigium commented Mar 17, 2016

Umcaruje commented Mar 17, 2016 via email

Fastigium commented Mar 17, 2016

Umcaruje commented Mar 17, 2016

tresf commented Mar 17, 2016

Umcaruje commented Mar 17, 2016

Fastigium commented Mar 18, 2016

musikBear commented Mar 18, 2016

Umcaruje commented Mar 18, 2016

Fastigium commented Mar 26, 2016

Umcaruje commented Mar 26, 2016

tresf commented Mar 26, 2016