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

[Frank Reininghaus]

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

Ship it!


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!


dolphin/src/views/versioncontrol/updateitemstatesthread.h
<http://git.reviewboard.kde.org/r/107656/#comment19338>

    Whitespace: 'const' should align with 'KVersionControlPlugin'



dolphin/src/views/versioncontrol/versioncontrolobserver.cpp
<http://git.reviewboard.kde.org/r/107656/#comment19339>

    Whitespace: '?' and ':' should be in the same column.


- Frank Reininghaus


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/20130113/ef2016ba/attachment.htm>