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

[PVR] Fix crashes on pvr manager deinit/reinit - part 2 #11225

Merged
merged 3 commits into from Dec 25, 2016

Conversation

@ksooo
Copy link
Member

commented Dec 20, 2016

This is a followup to #11143

Now, that first chunk of fixes is in master/Krypton, next (and hopefully the last) wave of followup issues arrives. First wave of fixes was definitely okay, but formerly, we did not get that far... ;-)

Crashes with stacks very similar to the following were reported on the forum (http://forum.kodi.tv/showthread.php?tid=298461&pid=2481973#pid2481973) and by @peak3d:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x006c98f8 in CDatabase::InitSettings (this=this@entry=0x5c12c500, dbSettings=...) at /home/neil/projects/LibreELEC.tv/build.LibreELEC-RPi2.arm-8.0-devel-debug/kodi-17.0-beta7-7581c4a/xbmc/dbwrappers/Database.cpp:404
[Current thread is 1 (Thread 0x56e233a0 (LWP 831))]

Thread 52 (Thread 0x66f023a0 (LWP 815)):
...
#29 0x007d0d28 in PVR::CPVRRecordings::CPVRRecordings (this=0x721141e8) at /home/neil/projects/LibreELEC.tv/build.LibreELEC-RPi2.arm-8.0-devel-debug/kodi-17.0-beta7-7581c4a/xbmc/pvr/recordings/PVRRecordings.cpp:51
=> PVR::CPVRManager::ResetProperties called PVR::CPVRManager::Clear() which deleted m_database and has set it to nullptr!
#30 0x007fb938 in PVR::CPVRManager::ResetProperties (this=this@entry=0x2ec7f50) at /home/neil/projects/LibreELEC.tv/build.LibreELEC-RPi2.arm-8.0-devel-debug/kodi-17.0-beta7-7581c4a/xbmc/pvr/PVRManager.cpp:281
#31 0x007fbb18 in PVR::CPVRManager::Start (this=0x2ec7f50) at /home/neil/projects/LibreELEC.tv/build.LibreELEC-RPi2.arm-8.0-devel-debug/kodi-17.0-beta7-7581c4a/xbmc/pvr/PVRManager.cpp:322
#32 0x007f0cf0 in PVR::CPVRClients::ConnectionStateChange (this=, client=0x721bd4d0, strConnectionString=..., newState=PVR_CONNECTION_STATE_CONNECTED, strMessage=...) at /home/neil/projects/LibreELEC.tv/build.LibreELEC-RPi2.arm-8.0-devel-debug/kodi-17.0-beta7-7581c4a/xbmc/pvr/addons/PVRClients.cpp:1543
#33 0x007fa1fc in PVR::CPVRClientConnectionJob::DoWork (this=0x5c2cd480) at /home/neil/projects/LibreELEC.tv/build.LibreELEC-RPi2.arm-8.0-devel-debug/kodi-17.0-beta7-7581c4a/xbmc/pvr/PVRManager.cpp:1884
#34 0x005b6ffc in CJobWorker::Process (this=0x4798500) at /home/neil/projects/LibreELEC.tv/build.LibreELEC-RPi2.arm-8.0-devel-debug/kodi-17.0-beta7-7581c4a/xbmc/utils/JobManager.cpp:69
#35 0x005f65c4 in CThread::Action (this=0x4798500) at /home/neil/projects/LibreELEC.tv/build.LibreELEC-RPi2.arm-8.0-devel-debug/kodi-17.0-beta7-7581c4a/xbmc/threads/Thread.cpp:221
#36 0x005f6ca8 in CThread::staticThread (data=0x4798500) at /home/neil/projects/LibreELEC.tv/build.LibreELEC-RPi2.arm-8.0-devel-debug/kodi-17.0-beta7-7581c4a/xbmc/threads/Thread.cpp:131
#37 0x76f0ff40 in start_thread (arg=0x66f023a0) at pthread_create.c:335
#38 0x7532be18 in ?? () at ../sysdeps/unix/sysv/linux/arm/clone.S:86 from /usr/lib/libc.so.6
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

Thread 1 (Thread 0x56e233a0 (LWP 831)):
#0  0x006c98f8 in CDatabase::InitSettings (this=this@entry=0x5c12c500, dbSettings=...) at /home/neil/projects/LibreELEC.tv/build.LibreELEC-RPi2.arm-8.0-devel-debug/kodi-17.0-beta7-7581c4a/xbmc/dbwrappers/Database.cpp:404
=> m_database access is not protected by pvr mgr lock => ptr value may get null or invalid at any time (like happened in this case because thread 52 deleted m_database concurrently).
#1  0x006cab64 in CDatabase::Open (this=0x5c12c500, settings=...) at /home/neil/projects/LibreELEC.tv/build.LibreELEC-RPi2.arm-8.0-devel-debug/kodi-17.0-beta7-7581c4a/xbmc/dbwrappers/Database.cpp:370
#2  0x007fbc94 in PVR::CPVRManager::Process (this=0x2ec7f50) at /home/neil/projects/LibreELEC.tv/build.LibreELEC-RPi2.arm-8.0-devel-debug/kodi-17.0-beta7-7581c4a/xbmc/pvr/PVRManager.cpp:429
#3  0x005f65c4 in CThread::Action (this=0x2ec7f58) at /home/neil/projects/LibreELEC.tv/build.LibreELEC-RPi2.arm-8.0-devel-debug/kodi-17.0-beta7-7581c4a/xbmc/threads/Thread.cpp:221
#4  0x005f6ca8 in CThread::staticThread (data=0x2ec7f58) at /home/neil/projects/LibreELEC.tv/build.LibreELEC-RPi2.arm-8.0-devel-debug/kodi-17.0-beta7-7581c4a/xbmc/threads/Thread.cpp:131
#5  0x76f0ff40 in start_thread (arg=0x56e233a0) at pthread_create.c:335
#6  0x7532be18 in ?? () at ../sysdeps/unix/sysv/linux/arm/clone.S:86 from /usr/lib/libc.so.6
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

@Jalle19 mind doing the code review.
@MilhouseVH could you please include this into your build?

@ksooo ksooo added this to the L 18.0-alpha1 milestone Dec 20, 2016
@ksooo ksooo requested a review from Jalle19 Dec 20, 2016
Copy link
Member

left a comment

Ideally requests for GUI info shouldn't have to go through the PVR manager, but I guess that's for another day.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Dec 20, 2016

grabbing the pvrManager lock for GUI info may harm and can lead to video stuttering

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Dec 20, 2016

do not backport!

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2016

grabbing the pvrManager lock for GUI info may harm and can lead to video stuttering

Okay, I will drop the guiinfo commit, then, but stay with the db access changes, including backport.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Dec 20, 2016

ok

@ksooo ksooo force-pushed the ksooo:pvr-fix-database-concurrency branch from 37f7d79 to f0f88fc Dec 20, 2016
@ksooo

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2016

dropped the guiinfo commit

@ksooo ksooo changed the title [PVR] Fix crashes on pvr manager deinit/reinit - part 2 & 3 [PVR] Fix crashes on pvr manager deinit/reinit - part 2 Dec 20, 2016
@Jalle19

This comment has been minimized.

Copy link
Member

commented Dec 20, 2016

I was about to ping @FernetMenta but forgot about it.

@janbar

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2016

@ksooo, I tested again with this fix , which is little different than you send me before, and the issue still here:

Thread 1 (Thread 0x7fe80dffb700 (LWP 22475)):
#0  __GI___pthread_mutex_lock (mutex=0x350) at ../nptl/pthread_mutex_lock.c:67
#1  0x00000000011f7d1f in PVR::CPVRManager::QueueJob(CJob*) ()
#2  0x00007fe85c2e15a2 in CHelper_libXBMC_pvr::TriggerRecordingUpdate() () from /home/jlb/.kodi/addons/pvr.mythtv/pvr.mythtv.so.4.1
2.7
#3  0x00007fe85c2d2c1a in PVRClientMythTV::RunHouseKeeping() () from /home/jlb/.kodi/addons/pvr.mythtv/pvr.mythtv.so.4.12.7
#4  0x00007fe85c2d1253 in PVRClientMythTV::HandleBackendMessage(Myth::shared_ptr<Myth::EventMessage>) () from /home/jlb/.kodi/addon
s/pvr.mythtv/pvr.mythtv.so.4.12.7

In your latest commit you moved the block to the begining of clear(). Maybe it could help.

@janbar

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2016

I retried with your original fix , but still crash now. Seems it was a chance it works as I guessed. I can open my PR #11231 which fix the issue.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Dec 21, 2016

I can open my PR #11231 which fix the issue.

Please, no. Although it fixes the issue it is not the right approach (violates architecture and introduces bad side effects, as i wrote earlier).

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2016

@FernetMenta root cause for the issue @janbar reported (see stack above) is that on kodi exit the PVR manager singleton gets destructed to early and therefore addon callbacks land in the woods.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2016

The second commit I added to this PR actually will not fix the kodi exit issue @janbar reported, but is still a needed fix for other cases, btw.

@janbar

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2016

Please find my bt at http://pastebin.com/6JBpPwEs
Don't care of commit revision, I applied this fix with a git apply.

@janbar

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2016

@ksooo , seems my PR still valid to fix my issue. Please let me know to re-open it.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Dec 21, 2016

We may need to move m_PVRManager.reset() after m_addonMgr.reset(), in CServiceManager::Deinit

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Dec 21, 2016

I will look into it

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2016

@ksooo , seems my PR still valid to fix my issue. Please let me know to re-open it.

No, it's not. Although it fixes the issue it is not the right approach (violates architecture and introduces bad side effects, as i wrote at least two times). Let's wait what @FernetMenta finds out.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2016

We may need to move m_PVRManager.reset() after m_addonMgr.reset(), in CServiceManager::Deinit

@FernetMenta This feels correct.

@ksooo ksooo force-pushed the ksooo:pvr-fix-database-concurrency branch from fe25670 to 761999e Dec 21, 2016
@ksooo

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2016

@janbar you can give it a go by applying this PR to your test system, if you like.

@ksooo ksooo force-pushed the ksooo:pvr-fix-database-concurrency branch from 761999e to 29c2243 Dec 21, 2016
@janbar

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2016

Thanks, I try it

@janbar

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2016

Unfortunately that doesn't help. See bt http://pastebin.com/XSD5V261

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Dec 22, 2016

@janbar thanks for testing

@FernetMenta seems that it is to late to release the addons in pvr manager's dtor as the callback gets called in a state where the manager is already "to much" destructed. Thus, releasing addons must be done earlier, before dtor body gets called, but not in Clear() and not in Stop() as we must release addons only on "final" shutdown of pvr mgr, not if it gets reinited due to addon update, async connect etc.

Give me some time. I guess I know how to fix properly and will come up with something, maybe later today.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Dec 22, 2016

my fault, for members with non-trivial destructors lifetime ends when execution of destructor begins. I will rework the patch.

@ksooo ksooo force-pushed the ksooo:pvr-fix-database-concurrency branch from 29c2243 to 489f3b3 Dec 22, 2016
@ksooo

This comment has been minimized.

Copy link
Member Author

commented Dec 22, 2016

@janbar new commit, next try? Should work now.

@janbar

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2016

.. compiling

@janbar

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2016

It has been a long time since I had seen a normal stop. Thanks @ksooo, @FernetMenta

17:55:34.982 T:140493969926528   DEBUG: AddOnLog: MythTV PVR Client: (CPPMyth)Close: done
17:55:34.982 T:140493969926528   DEBUG: AddOnLog: MythTV PVR Client: (CPPMyth)Stop: event handler thread (0x7fc6e400ca50)
17:55:35.111 T:140491769231104   DEBUG: AddOnLog: MythTV PVR Client: (CPPMyth)AnnounceStatus: (0x7fc6e400ca50) STOPPED
17:55:35.111 T:140491794409216   DEBUG: AddOnLog: MythTV PVR Client: RunHouseKeeping
17:55:35.111 T:140491769231104   DEBUG: AddOnLog: MythTV PVR Client: (CPPMyth)SendCommand: DONE
17:55:35.111 T:140491769231104   DEBUG: AddOnLog: MythTV PVR Client: (CPPMyth)Close: done
17:55:35.111 T:140493969926528   DEBUG: AddOnLog: MythTV PVR Client: (CPPMyth)Stop: event handler thread (0x7fc6e400ca50) stopped
17:55:35.112 T:140493969926528   DEBUG: AddOnLog: MythTV PVR Client: (CPPMyth)Stop: subscription thread (0x7fc6e4001f30:1)
17:55:35.112 T:140493969926528   DEBUG: AddOnLog: MythTV PVR Client: (CPPMyth)Stop: subscription thread (0x7fc6e4001f30:1) stopped
17:55:35.112 T:140493969926528   DEBUG: AddOnLog: MythTV PVR Client: (CPPMyth)Stop: subscription thread (0x7fc6e4001f30:2)
17:55:35.112 T:140493969926528   DEBUG: AddOnLog: MythTV PVR Client: (CPPMyth)Stop: subscription thread (0x7fc6e4001f30:2) stopped
17:55:35.112 T:140493969926528   DEBUG: AddOnLog: MythTV PVR Client: (CPPMyth)SendCommand: DONE
17:55:35.112 T:140493969926528   DEBUG: AddOnLog: MythTV PVR Client: (CPPMyth)Close: done
17:55:35.113 T:140493969926528   DEBUG: SECTION:UnloadDll(/home/jlb/.kodi/addons/pvr.mythtv/pvr.mythtv.so.5.0.8)
17:55:35.113 T:140493969926528    INFO: ADDON: Dll Destroyed - MythTV PVR Client
17:55:35.113 T:140493969926528   DEBUG: CSettingsManager: requested setting (epg.daystodisplay) was not found.
17:55:35.114 T:140493969926528   DEBUG: PVRManager - destroyed
17:55:35.114 T:140493969926528   DEBUG: ActiveAE DSP - destroyed
17:55:35.115 T:140493454214912   DEBUG: Thread Announce 140493454214912 terminating
17:55:35.115 T:140493969926528  NOTICE: application stopped...
17:55:35.118 T:140493969926528   DEBUG: SECTION:UnloadDll(libcurl.so.4)
17:55:35.120 T:140493969926528   DEBUG: LogindUPowerSyscall - delay lock released```
@janbar

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2016

I would like to test the Krypton's backport too when it will be ready to pull. Again thanks for you work.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Dec 22, 2016

I would like to test the Krypton's backport

Sure, I will ping you when it's ready. Want to first hear from the milhouse build testers that it is working without regressions. This may take some more days.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Dec 22, 2016

jenkins build this please

@ksooo ksooo force-pushed the ksooo:pvr-fix-database-concurrency branch from 489f3b3 to df58319 Dec 24, 2016
@ksooo

This comment has been minimized.

Copy link
Member Author

commented Dec 24, 2016

No negative feedback from milhouse build testers... If nobody objects i will merge this to master tomorrow and open the backport PR, then.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Dec 25, 2016

jenkins build this please

@ksooo ksooo merged commit 6da8f44 into xbmc:master Dec 25, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins4kodi You did a great job. Have a cookie.
Details
@ksooo ksooo deleted the ksooo:pvr-fix-database-concurrency branch Dec 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.