Review Request: Fix crash due to deleteLater being sent to the wrong object sometimes

[Simeon Bird]

> On Dec. 11, 2012, 7:23 a.m., Frank Reininghaus wrote:
> > Thanks for the patch!
> > 
> > > If updateItemStates was called from slotThreadFinished,
> > > m_updateItemStatesThread would be different when slotThreadFinished was
> > > over. deleteLater would be called on the *new* m_updateItemStatesThread,
> > > instead of the just-finished one, causing it to be deleted before
> > > it had finished running, and a crash.
> > 
> > Are you sure? VersionControlObserver::updateItemStates() connects the finished() signal and the deleteLater() slot of the *same* thread, so I don't see how a thread can be deleted before it's finished.
> > 
> > About the "} else if(!m_updateItemStatesThread){" changes: could you elaborate on why this could fix a crash? Moreover, could it be a problem that your new implementation may return an empty list 'actions' if lockPlugin() fails?
> > 
> > Sorry if my questions are stupid! I have little experience with the version control stuff ;-)

I'm not sure, but I have a 2-sigma confidence :) 

This is how I figured it out: 

I was getting this crash occasionally, so I tried removing the unlock/relock pair. 
This was slower, of course, so then I tried making the lock a QReadWriteLock. 
Rather to my surprise, I then got the crash all the time! And in the logs there was the message:

QThread: Destroyed while thread is still running

Which means that a delete event has to be passed to the QThread *before* the finished() signal is emitted. 

Here is what I think happens: 

updateItemStates() connects m_updateItemStatesThread's finished() to slotThreadFinished() and m_updateItemStatesThread->deleteLater() (in that order). So when the event is fired, Qt calls slotThreadFinished(), and then once it has finished, calls m_updateItemStatesThread->deleteLater().

BUT m_updateItemStatesThread is changed in slotThreadFinished, and thus does not point to the same object that emitted finished()!

Most of the time we have m_updateItemStatesThread=0 (which just leaks), but sometimes we call updateItemStates() from slotThreadFinished(), which assigns m_updateItemStatesThread to a new QThread, which then gets deleted too early.


As to the } else if(!m_updateItemStatesThread){ bit:

The code tries to get the plugin lock if m_updateItemStatesThread != 0. But getting the plugin lock may fail (because m_updateItemStatesThread is holding onto it too long). If it does fail to get the lock, however, 
the code used the plugin anyway, which is not safe, because it may be being changed by m_updateItemStatesThread::setData at the time. 
I think returning an empty action list is fine, because it will just mean that some file will not have the "git add" menu items.
I think in practice that the plugin lock can always be obtained though, so it is a bit academic.

You want I should put all this in the commit messages? :)


> On Dec. 11, 2012, 7:23 a.m., Frank Reininghaus wrote:
> > dolphin/src/views/versioncontrol/updateitemstatesthread.cpp, line 76
> > <http://git.reviewboard.kde.org/r/107656/diff/1/?file=98047#file98047line76>
> >
> >     The Qt docs don't say anything about 'upgrading' read locks to write locks, so I'm wondering if it would actually be valid behaviour for QReadWriteMutex to freeze forever here?

hm, it doesn't appear to in the current implementation, but it's true that the docs are a bit ambiguous. They explicitly state that *lockForRead* will block if the lock is already locked for writing, but they say nothing about lockForWrite. Since we are under the plugin mutex anyway here, we can safely unlock() the itemMutex before locking for write, so how about we do that, just to be safe?


- Simeon


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107656/#review23301
-----------------------------------------------------------


On Dec. 10, 2012, 4:27 a.m., Simeon Bird wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107656/
> -----------------------------------------------------------
> 
> (Updated Dec. 10, 2012, 4:27 a.m.)
> 
> 
> Review request for Dolphin and Frank Reininghaus.
> 
> 
> Description
> -------
> 
> Three commits here:
> 
> commit bd89a3ebe4f546e2032676c4b445befd9606e877
> 
>     Fix a potential crash if the plugin lock could not be taken
>     in VersionControlObserver.
> 
> (the "} else if(!m_updateItemStatesThread){" changes: I didn't see this, and I think it is unlikely, but best to be safe.)
> 
> 
> commit b1473eb1247e9168805ac92964cab11a10d6a0e2
> 
>     Make the UpdateItemStatesThread use a QReadWriteLock for protection
>     instead of a QMutex, now the previous crash is fixed and we can.
>     
>     Makes listings a bit faster.
> 
> commit 8f17e50800a7769521e496ab324d608ec0e36df4
> 
>     VersionControlObserver:
>     
>     If updateItemStates was called from slotThreadFinished,
>     m_updateItemStatesThread would be different when slotThreadFinished was
>     over. deleteLater would be called on the *new* m_updateItemStatesThread,
>     instead of the just-finished one, causing it to be deleted before
>     it had finished running, and a crash.
>     
>     Instead call deleteLater from slotThreadFinished directly.
>     
>     BUG: 302264
>     FIXED-IN: 4.10
> 
> This should really fix bug 302264 this time. 
> 
> 
> This addresses bug 302264.
>     http://bugs.kde.org/show_bug.cgi?id=302264
> 
> 
> Diffs
> -----
> 
>   dolphin/src/views/versioncontrol/updateitemstatesthread.h f0f91d7 
>   dolphin/src/views/versioncontrol/updateitemstatesthread.cpp e07d72c 
>   dolphin/src/views/versioncontrol/versioncontrolobserver.cpp 42e00de 
> 
> Diff: http://git.reviewboard.kde.org/r/107656/diff/
> 
> 
> Testing
> -------
> 
> Bug is fixed! 
> 
> It was very easy to reproduce (for me) once the window for it was made wider by using QReadWriteLock in UpdateItemStatesThread, and is no longer there.
> 
> 
> Thanks,
> 
> Simeon Bird
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20121212/98a8bb83/attachment.htm>