Skip to content

Commit

Permalink
dialogs/GUIDialogBusy: stop the CThread properly
Browse files Browse the repository at this point in the history
Class `CBusyWaiter` derives from `CThread`, and its only instance
lives in the stack frame of `CGUIDialogBusy::Wait()`.

Commit cc8364a triggered an ancient
Kodi crash bug based on a misunderstanding how destructors work in
C++, introduced in commit 64427d4
(coincidentally by the same author).

Anyway, that commit cc8364a changed
how cancellation gets triggered, making it more likely.  And if that
cancellation happens, nobody takes care for stopping the CThread
properly; `~CThread()` calls `StopThread()`, but by then, the
`CBusyWaiter` instance has already been morphed back to its base class
`CThread`.  This however triggers the crash in the still-running
thread.

This class morphing while calling destructors is what makes the whole
`~CThread()` implementation wrong from the bottom: calling
`StopThread()` from the base class destructor can never ever work
properly, because it will crash the thread in any case.  And if no
thread were running anymore, the call would be useless.

All uses of `CThread` without additional calls to `StopThread()` are a
crash bug, but this commit fixes only the instance in class
`CBusyWaiter`, by adding another `StopThread()` call to its
destructor.

This is how the crash looks like:

```
Thread 1 (Thread 0x7ff1d37fe700 (LWP 1394)):
#0  __GI___pthread_mutex_lock (mutex=0x66657270747265d3) at ../nptl/pthread_mutex_lock.c:65
plexinc#1  0x0000562afcf3352c in (anonymous namespace)::CRecursiveMutex::lock (this=0x66657270747265d3) at xbmc/threads/platform/RecursiveMutex.h:45
plexinc#2  0x0000562afcf34618 in (anonymous namespace)::CountingLockable<XbmcThreads::CRecursiveMutex>::lock (this=0x66657270747265d3) at xbmc/threads/Lockables.h:63
plexinc#3  0x0000562afcf34466 in (anonymous namespace)::UniqueLock<CCriticalSection>::UniqueLock (this=0x7ff1d37fdb80, lockable=...) at xbmc/threads/Lockables.h:132
plexinc#4  0x0000562afcf3356f in CSingleLock::CSingleLock (this=0x7ff1d37fdb80, cs=...) at xbmc/threads/SingleLock.h:38
plexinc#5  0x0000562afd4ee7c7 in (anonymous namespace)::CEventGroup::Set (this=0x6665727074726563, child=0x7ffeca201060) at xbmc/threads/Event.h:127
plexinc#6  0x0000562afd4ee2db in CEvent::Set (this=0x7ffeca201060) at xbmc/threads/Event.cpp:70
plexinc#7  0x0000562afd4f16ff in CThread::staticThread (data=0x7ffeca200f80) at xbmc/threads/Thread.cpp:133
plexinc#8  0x00007ff20bd955aa in start_thread (arg=0x7ff1d37fe700) at pthread_create.c:463
plexinc#9  0x00007ff20321acbf in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
```

And this is the thread which created the `CBusyWaiter` (at the time of
the crash it had already moved on):

```
Thread 19 (Thread 0x7ff20c191980 (LWP 1353)):
#0  0x00007ff209da1c83 in ?? () from /usr/lib/x86_64-linux-gnu/libsqlite3.so.0
plexinc#1  0x00007ff209d9c3c6 in ?? () from /usr/lib/x86_64-linux-gnu/libsqlite3.so.0
plexinc#2  0x00007ff209dcc0ab in ?? () from /usr/lib/x86_64-linux-gnu/libsqlite3.so.0
plexinc#3  0x00007ff209dd0c9e in ?? () from /usr/lib/x86_64-linux-gnu/libsqlite3.so.0
plexinc#4  0x00007ff209dd11fe in ?? () from /usr/lib/x86_64-linux-gnu/libsqlite3.so.0
plexinc#5  0x00007ff209dd1526 in sqlite3_prepare_v2 () from /usr/lib/x86_64-linux-gnu/libsqlite3.so.0
plexinc#6  0x0000562afd755a56 in (anonymous namespace)::SqliteDataset::query (this=0x562b03898080, query=...) at xbmc/dbwrappers/sqlitedataset.cpp:649
plexinc#7  0x0000562afd33a824 in CVideoDatabase::GetScraperForPath (this=0x562b02530f00, strPath=..., settings=..., foundDirectly=@0x7ffeca201b6f: false) at xbmc/video/VideoDatabase.cpp:7297
plexinc#8  0x0000562afd33b831 in CVideoDatabase::GetContentForPath (this=0x562b02530f00, strPath=...) at xbmc/video/VideoDatabase.cpp:7426
plexinc#9  0x0000562afd27dc6c in CGUIWindowVideoNav::LoadVideoInfo (items=..., database=..., allowReplaceLabels=true) at xbmc/video/windows/GUIWindowVideoNav.cpp:578
plexinc#10 0x0000562afd27db95 in CGUIWindowVideoNav::LoadVideoInfo (this=0x562b02530760, items=...) at xbmc/video/windows/GUIWindowVideoNav.cpp:564
plexinc#11 0x0000562afd27ce81 in CGUIWindowVideoNav::GetDirectory (this=0x562b02530760, strDirectory=..., items=...) at xbmc/video/windows/GUIWindowVideoNav.cpp:545
plexinc#12 0x0000562afd3b4e35 in CGUIMediaWindow::Update (this=0x562b02530760, strDirectory=..., updateFilterPath=true) at xbmc/windows/GUIMediaWindow.cpp:806
plexinc#13 0x0000562afd274a96 in CGUIWindowVideoBase::Update (this=0x562b02530760, strDirectory=..., updateFilterPath=true) at xbmc/video/windows/GUIWindowVideoBase.cpp:1247
plexinc#14 0x0000562afd27ae04 in CGUIWindowVideoNav::Update (this=0x562b02530760, strDirectory=..., updateFilterPath=true) at xbmc/video/windows/GUIWindowVideoNav.cpp:340
plexinc#15 0x0000562afd3b6e91 in CGUIMediaWindow::OnClick (this=0x562b02530760, iItem=2, player=...) at xbmc/windows/GUIMediaWindow.cpp:1072
plexinc#16 0x0000562afd270a66 in CGUIWindowVideoBase::OnClick (this=0x562b02530760, iItem=2, player=...) at xbmc/video/windows/GUIWindowVideoBase.cpp:609
plexinc#17 0x0000562afd283ec0 in CGUIWindowVideoNav::OnClick (this=0x562b02530760, iItem=2, player=...) at xbmc/video/windows/GUIWindowVideoNav.cpp:1205
plexinc#18 0x0000562afd3b7867 in CGUIMediaWindow::OnSelect (this=0x562b02530760, item=2) at xbmc/windows/GUIMediaWindow.cpp:1141
plexinc#19 0x0000562afd270c1b in CGUIWindowVideoBase::OnSelect (this=0x562b02530760, iItem=2) at xbmc/video/windows/GUIWindowVideoBase.cpp:627
plexinc#20 0x0000562afd3b1978 in CGUIMediaWindow::OnMessage (this=0x562b02530760, message=...) at xbmc/windows/GUIMediaWindow.cpp:319
plexinc#21 0x0000562afd26e169 in CGUIWindowVideoBase::OnMessage (this=0x562b02530760, message=...) at xbmc/video/windows/GUIWindowVideoBase.cpp:200
plexinc#22 0x0000562afd27a666 in CGUIWindowVideoNav::OnMessage (this=0x562b02530760, message=...) at xbmc/video/windows/GUIWindowVideoNav.cpp:254
plexinc#23 0x0000562afd61dbba in CGUIControl::SendWindowMessage (this=0x562b03547780, message=...) at xbmc/guilib/GUIControl.cpp:316
plexinc#24 0x0000562afd60ebaa in CGUIBaseContainer::OnClick (this=0x562b03547780, actionID=7) at xbmc/guilib/GUIBaseContainer.cpp:793
plexinc#25 0x0000562afd60cce7 in CGUIBaseContainer::OnAction (this=0x562b03547780, action=...) at xbmc/guilib/GUIBaseContainer.cpp:407
plexinc#26 0x0000562afd64570b in CGUIFixedListContainer::OnAction (this=0x562b03547780, action=...) at xbmc/guilib/GUIFixedListContainer.cpp:81
plexinc#27 0x0000562afd6a69fc in CGUIWindow::OnAction (this=0x562b02530760, action=...) at xbmc/guilib/GUIWindow.cpp:435
plexinc#28 0x0000562afd3b0d43 in CGUIMediaWindow::OnAction (this=0x562b02530760, action=...) at xbmc/windows/GUIMediaWindow.cpp:202
plexinc#29 0x0000562afd26dcb7 in CGUIWindowVideoBase::OnAction (this=0x562b02530760, action=...) at xbmc/video/windows/GUIWindowVideoBase.cpp:111
plexinc#30 0x0000562afd2797a0 in CGUIWindowVideoNav::OnAction (this=0x562b02530760, action=...) at xbmc/video/windows/GUIWindowVideoNav.cpp:105
plexinc#31 0x0000562afd6b3c14 in CGUIWindowManager::HandleAction (this=0x562b01d17c60, action=...) at xbmc/guilib/GUIWindowManager.cpp:1100
plexinc#32 0x0000562afd6b39ce in CGUIWindowManager::OnAction (this=0x562b01d17c60, action=...) at xbmc/guilib/GUIWindowManager.cpp:1050
plexinc#33 0x0000562afd8e1888 in CApplication::OnAction (this=0x562b01a4d5d0, action=...) at xbmc/Application.cpp:1924
xbmc#34 0x0000562afd5b729a in CInputManager::ExecuteInputAction (this=0x562b01bb8680, action=...) at xbmc/input/InputManager.cpp:704
xbmc#35 0x0000562afd5b6c86 in CInputManager::HandleKey (this=0x562b01bb8680, key=...) at xbmc/input/InputManager.cpp:644
xbmc#36 0x0000562afd5b5ed8 in CInputManager::OnKey (this=0x562b01bb8680, key=...) at xbmc/input/InputManager.cpp:483
xbmc#37 0x0000562afd5b5be6 in CInputManager::OnEvent (this=0x562b01bb8680, newEvent=...) at xbmc/input/InputManager.cpp:442
xbmc#38 0x0000562afd8d8aaf in CApplication::HandlePortEvents (this=0x562b01a4d5d0) at xbmc/Application.cpp:341
xbmc#39 0x0000562afd8e4fe6 in CApplication::FrameMove (this=0x562b01a4d5d0, processEvents=true, processGUI=true) at xbmc/Application.cpp:2647
xbmc#40 0x0000562afd9add56 in CXBApplicationEx::Run (this=0x562b01a4d5d0, params=...) at xbmc/XBApplicationEx.cpp:107
xbmc#41 0x0000562afd528974 in XBMC_Run (renderGUI=true, params=...) at xbmc/platform/xbmc.cpp:88
xbmc#42 0x0000562afcef1e72 in main (argc=4, argv=0x7ffeca2096e8) at xbmc/platform/posix/main.cpp:108
```
  • Loading branch information
MaxKellermann committed May 12, 2018
1 parent ce5cecc commit ab4c697
Showing 1 changed file with 5 additions and 0 deletions.
5 changes: 5 additions & 0 deletions xbmc/dialogs/GUIDialogBusy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ class CBusyWaiter : public CThread
explicit CBusyWaiter(IRunnable *runnable) :
CThread(runnable, "waiting"), m_done(new CEvent()), m_runnable(runnable) { }

~CBusyWaiter()
{
StopThread();
}

bool Wait(unsigned int displaytime, bool allowCancel)
{
std::shared_ptr<CEvent> e_done(m_done);
Expand Down

0 comments on commit ab4c697

Please sign in to comment.