search

jamulussoftware / jamulus

Jamulus enables musicians to perform real-time jam sessions over the internet.
https://jamulus.io
Other
997 stars 222 forks source link

Server crash when changing recording directory in UI #1501

Closed hoffie closed 3 years ago

hoffie commented 3 years ago

Describe the bug

Jamulus Server crashes when changing the recording directory in the UI for the second time. This even happens when there is no active recording.

To Reproduce

Expected behavior

As @softins suggested, the recording directory should probably be unchangable when recording is active.

Screenshots

systemd logs this stack trace, although I'm not sure if it is relevant or helpful at all.

Stack trace of thread 38388:
#0  0x00007fc6603ea928 _ZTV12QPaintDevice (libQt5Gui.so.5 + 0x6b7928)
#1  0x000055a8421222c8 n/a (jamulus/Jamulus + 0x752c8)
#2  0x000055a8421e3cb1 n/a (jamulus/Jamulus + 0x136cb1)
#3  0x000055a8421e9f8b n/a (jamulus/Jamulus + 0x13cf8b)
#4  0x000055a8421e9bd1 n/a (jamulus/Jamulus + 0x13cbd1)
#5  0x000055a8421e9562 n/a (jamulus/Jamulus + 0x13c562)
#6  0x00007fc65f903d86 n/a (libQt5Core.so.5 + 0x2ecd86)
#7  0x00007fc66064db3f n/a (libQt5Widgets.so.5 + 0x24cb3f)
#8  0x00007fc66064f063 n/a (libQt5Widgets.so.5 + 0x24e063)
#9  0x00007fc66064f263 _ZN15QAbstractButton17mouseReleaseEventEP11QMouseEvent (libQt5Widgets.so.5 + 0x24e263)
#10 0x00007fc66059cb0e _ZN7QWidget5eventEP6QEvent (libQt5Widgets.so.5 + 0x19bb0e)
#11 0x00007fc66055b752 _ZN19QApplicationPrivate13notify_helperEP7QObjectP6QEvent (libQt5Widgets.so.5 + 0x15a752)
#12 0x00007fc66056287b _ZN12QApplication6notifyEP7QObjectP6QEvent (libQt5Widgets.so.5 + 0x16187b)
#13 0x00007fc65f8cca2a _ZN16QCoreApplication15notifyInternal2EP7QObjectP6QEvent (libQt5Core.so.5 + 0x2b5a2a)
#14 0x00007fc66056187e _ZN19QApplicationPrivate14sendMouseEventEP7QWidgetP11QMouseEventS1_S1_PS1_R8QPointerIS0_Ebb (libQt5Widgets.so.5 + 0x16087e)
#15 0x00007fc6605b5249 n/a (libQt5Widgets.so.5 + 0x1b4249)
#16 0x00007fc6605b863f n/a (libQt5Widgets.so.5 + 0x1b763f)
#17 0x00007fc66055b752 _ZN19QApplicationPrivate13notify_helperEP7QObjectP6QEvent (libQt5Widgets.so.5 + 0x15a752)
#18 0x00007fc65f8cca2a _ZN16QCoreApplication15notifyInternal2EP7QObjectP6QEvent (libQt5Core.so.5 + 0x2b5a2a)
#19 0x00007fc65fe6d594 _ZN22QGuiApplicationPrivate17processMouseEventEPN29QWindowSystemInterfacePrivate10MouseEventE (libQt5Gui.so.5 + 0x13a594)
#20 0x00007fc65fe42bb5 _ZN22QWindowSystemInterface22sendWindowSystemEventsE6QFlagsIN10QEventLoop17ProcessEventsFlagEE (libQt5Gui.so.5 + 0x10fbb5)
#21 0x00007fc65b7f0a81 n/a (libQt5WaylandClient.so.5 + 0xa3a81)
#22 0x00007fc65e19cf9c g_main_context_dispatch (libglib-2.0.so.0 + 0x53f9c)
#23 0x00007fc65e1f0a49 n/a (libglib-2.0.so.0 + 0xa7a49)
#24 0x00007fc65e19a6f1 g_main_context_iteration (libglib-2.0.so.0 + 0x516f1)
#25 0x00007fc65f925691 _ZN20QEventDispatcherGlib13processEventsE6QFlagsIN10QEventLoop17ProcessEventsFlagEE (libQt5Core.so.5 + 0x30e691)
#26 0x00007fc65f8cb3ac _ZN10QEventLoop4execE6QFlagsINS_17ProcessEventsFlagEE (libQt5Core.so.5 + 0x2b43ac)
#27 0x00007fc65f8d3844 _ZN16QCoreApplication4execEv (libQt5Core.so.5 + 0x2bc844)
#28 0x000055a842108a48 n/a (jamulus/Jamulus + 0x5ba48)
#29 0x00007fc65f135b25 __libc_start_main (libc.so.6 + 0x27b25)
#30 0x000055a8420dab4e n/a (jamulus/Jamulus + 0x2db4e)

Operating system

Arch Linux, 5.11.10-arch1-1, Qt 5.15.2

Version of Jamulus

Additional context

3.6.2 does not have this problem.

hoffie commented 3 years ago

I haven't double-checked, but git bisect points towards c8bfa4fcb4a5aa06dcba4ac0c2e69ad8b8a60a7a as the culprit (as @softins already suspected :)).

(I'm not currently working on this further, feel free to follow up; if I have time, I'll comment here)

henkdegroot commented 3 years ago

Interesting issue, I will try to see if I can find the solution for this. I can easy reproduce this on my system. I don't even have to change it twice. When I start the server (previous with 3.6.2 I had selected a folder for recording) and go to the directory setting, changing the folder crashes the application instantly. I recently got debug working in QT, so hope I can find what is causing this with the information already provided.

ann0see commented 3 years ago

If 3.7.0 is affected why do you think it's related to 664fd9e? Wasn't this commit after release 3.7.0?

hoffie commented 3 years ago

@henkdegroot Cool! I'm assigning this issue to you unless you state otherwise. Please keep us posted here. :)

If 3.7.0 is affected why do you think it's related to 664fd9e? Wasn't this commit after release 3.7.0?

I only wanted to state that 664fd9e is also affected. I didn't intend to say that it is the cause. I suspect c8bfa4f to be the cause. Reverting it will most likely fix the crash, but will also probably re-introduce the memory leaks (I still haven't verified that though).

henkdegroot commented 3 years ago

c8bfa4f has indeed introduced this. When I comment out the 

        delete pJamRecorder;

at line 97, the crash no longer occurs.

Also believe my issue (with an already populated directory in the settings) and the issue you described (changing for the second time) is the same, provided you started with a "empty" directory.

Thinking it might be the recorder got never fully initialized (as no recording was performed yet). Will look into this further, but just wanted to share this.

henkdegroot commented 3 years ago

@softins I am trying to find the best way to resolve this issue and would like to get your input, as it seems my knowledge is failing... You have been fixing some memory leaks in the recorder area and part of that code is to implement a "delete" on the pJamRecorder. This delete seems to crash the server application in some circumstances. I wonder if the delete is actually performed twice?

What I found in jamcontroller.cpp is:

    emit EndRecorderThread();
    pthJamRecorder->wait();
    delete pthJamRecorder;
    pthJamRecorder = nullptr;

--- some more code lines removed here ---

        delete pJamRecorder;
        pJamRecorder = nullptr;

Just listed the part of interest here.

Also found the following connect in the file:

    QObject::connect ( pthJamRecorder, &QThread::finished,
        pJamRecorder, &QObject::deleteLater );

My thought is that this connect to the pthJamRecorder thread finished is already triggering the delete of the pJamRecorder. For that reason I believe the "manual" coding of the delete pJamRecorder (and setting it to nullptr) is not needed. Obviously I do not want to introduce a memory leak....and have limited knowledge in that area.

When the "delete pJamRecorder" is removed from the code, the server app no longer crashes. When the "connect" is removed (and the delete is kept in place), the server app no longer crashes.

Hope you have some time to share your thoughts/opinion on this.

softins commented 3 years ago

@henkdegroot my time is currently limited due to family circumstances, but I did spend a couple of hours looking at this yesterday, and found similar to what you have. I need to spend some more time to fully understand the interactions between the objects. I'll reply more later.

henkdegroot commented 3 years ago

@softins thank you for the reply. Will wait for you later....I will try to find more myself as well. So far I did noticed that (while I have debug running) I see the pJamRecorder thread has values set up to the moment the pthJamRecorder gets deleted. When it gets deleted I see some values turn into "not accessible". Not always that same items are showing this, but most of the time the recordBaseDir is showing this. Sometimes I have even seen this on the pJamRecorder item itself (not accessible) as well. Hope this helps.

hoffie commented 3 years ago

@henkdegroot Thanks for your analysis!

For that reason I believe the "manual" coding of the delete pJamRecorder (and setting it to nullptr) is not needed. Obviously I do not want to introduce a memory leak....and have limited knowledge in that area.

When the "delete pJamRecorder" is removed from the code, the server app no longer crashes. When the "connect" is removed (and the delete is kept in place), the server app no longer crashes.

I can confirm and I agree. Side note: I had tried replacing the delete with a pJamRecorder->deleteLater(), which, according to the docs, is safe to be called multiple times. However, Jamulus still crashed.

I believe it is safe and correct to remove the delete pJamRecorder call. Reasoning:

@henkdegroot Can you open a PR to do just that? I think @softins and @pljones should have the final call here. Having the PR ready would speed things up a bit so that we can include it into the upcoming release.

pljones commented 3 years ago

OK, having checked the identified problem - and as this is effectively reverting to what I originally wrote ;) - I can say I'm happy.

henkdegroot commented 3 years ago

@henkdegroot Can you open a PR to do just that? I think @softins and @pljones should have the final call here. Having the PR ready would speed things up a bit so that we can include it into the upcoming release.

Thanks for checking. Will do the PR shortly.