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

[Frank Reininghaus]

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


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 ;-)


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

    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?



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

    better:
    
        } else {


- Frank Reininghaus


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/20121211/d3355253/attachment.htm>