Review Request 107656: Fix crash due to Qthread being restarted and then deleted

[Frank Reininghaus]

> On Jan. 13, 2013, 8:43 a.m., Frank Reininghaus wrote:
> > Thanks, very good analysis! I think that you have really found the root cause of the bug now. The design of the "resuse threads" idea is indeed broken if finished threads invoke their own deleteLater() slot...
> > 
> > Please push to master (the changes are a bit too intrusive for KDE/4.10, I think). Thanks again for spending so much time debugging this very tricky issue!
> 
> Simeon Bird wrote:
>     Thanks very much for your help! 
>     
>     Would it be ok if I committed a short one-line version to fix the bug to the KDE/4.10 branch, and then the rest of the associated cleanups on top of that to master only?
>     
>     The one-line version would look like:
>     
>     diff --git a/dolphin/src/views/versioncontrol/versioncontrolobserver.cpp b/dolphin/src/views/versioncontrol/versioncontrolobserver.cpp
>     index 42e00de..64bc268 100644
>     --- a/dolphin/src/views/versioncontrol/versioncontrolobserver.cpp
>     +++ b/dolphin/src/views/versioncontrol/versioncontrolobserver.cpp
>     @@ -245,7 +245,7 @@ void VersionControlObserver::updateItemStates()
>              connect(m_updateItemStatesThread, SIGNAL(finished()),
>                      m_updateItemStatesThread, SLOT(deleteLater()));
>          }
>     -    if (m_updateItemStatesThread->isRunning()) {
>     +    else {
>              // An update is currently ongoing. Wait until the thread has finished
>              // the update (see slotThreadFinished()).
>              m_pendingItemStatesUpdate = true;
>
> 
> Frank Reininghaus wrote:
>     Sorry for the late response. Yes, that one-line change should indeed fix the root cause of the crash, and I don't see any potential problems with it, so I'd say that it looks safe enough for the KDE/4.10 branch.
> 
> Simeon Bird wrote:
>     Cheers - would you prefer I commit to master and cherrypick, or commit to 4.10 and merge that branch into master?

Commit to KDE/4.10, merge into master.


- Frank


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


On Jan. 12, 2013, 3:58 p.m., Simeon Bird wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107656/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2013, 3:58 p.m.)
> 
> 
> Review request for Dolphin and Frank Reininghaus.
> 
> 
> Description
> -------
> 
> commit c9ff067317155da63393f45d3adf543830fc866b
> Author: Simeon Bird <bladud at gmail.com>
> Date:   Sat Jan 12 10:43:26 2013 -0500
> 
>     A crash occurs if updateItemStates runs between the
>     UpdateItemStatesThread finishing and the finished() signal being
>     delivered.
>     
>     In this case, a new thread was not created, because the old thread still
>     existed. However, pendingItemStatesUpdate was not set, because the thread was
>     not running. Instead, the old thread was restarted.
>     
>     This meant that the finished() signal from the first run could be delivered
>     while the thread was running for a second time, causing the thread to be
>     deleted while still running and thus a crash.
>     
>     Solution: set pendingItemStatesUpdate if the thread is non-null, even if
>     it is not running, knowing that slotThreadFinished has not yet run, and
>     will call updateItemStates itself.
>     
>     BUG: 302264
>     FIXED-IN: 4.10
>     REVIEW: 107656
> 
> commit da96efba82abb8848c1288684babc1883ea8cc8d
> Author: Simeon Bird <bladud at gmail.com>
> Date:   Sat Jan 12 10:37:57 2013 -0500
> 
>     The locking around the plugin access in actions doesn't seem to be
>     necessary, as actions is only called from the main thread.
>     
>     Also it wasn't checked consistently; if the lock could not be taken, the
>     plugin was accessed anyway.
> 
> commit 89711faa7d7718ed35ff77fe5d2df3e502102ecf
> Author: Simeon Bird <bladud at gmail.com>
> Date:   Sat Jan 12 10:37:26 2013 -0500
> 
>     We don't need the mutex guarding m_itemStates in the
>     UpdateItemStatesThread, because m_itemStates is only accessed by the
>     when the thread is done, and set before the thread starts.
>     
>     Also combine the setData function with the constructor.
> 
> 
> 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
> -------
> 
> The crash can be made very easy to reproduce by shortening the verificationTimer in VersionControlObserver to 50msec. 
> With this patch, the crash goes away.
> 
> 
> Thanks,
> 
> Simeon Bird
> 
>

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