Review Request: Fix crash due to Qthread member variable being deleted before it was finished.

[Frank Reininghaus]

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


Thanks for the new patch and the analysis! I hadn't thought of the possibility that we might be dealing with more than one UpdateItemStatesThread yet.

"When an object gets deleted, its member variables get deleted too":

I'm afraid that this is incorrect. m_updateItemStatesThread is a *pointer* to an UpdateItemStatesThread. Member variables which are pointers are not deleted on destruction (unless they are children of the QObject being destructed, but that is not the case here). This means that the thread object is left alone when the VersionControlObserver is destructed, and the line "m_updateItemStatesThread=0;" in the destructor, which is supposed to fix the crash if I understand correctly, is essentially a no-op if I'm not mistaken.

Could you check if it makes any difference if you comment out that line, or if you add it in the state where you could reproduce the crash rather reliably?

I haven't looked at the other parts of the patch in much detail yet. At first sight, it looks like it might make sense though.

- Frank Reininghaus


On Dec. 24, 2012, 1:13 a.m., Simeon Bird wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107656/
> -----------------------------------------------------------
> 
> (Updated Dec. 24, 2012, 1:13 a.m.)
> 
> 
> Review request for Dolphin and Frank Reininghaus.
> 
> 
> Description
> -------
> 
> Three commits here:
> 
> commit fecc4da9d614b9b3d0924a55e484b34187f0e743
> Author: Simeon Bird <spb41 at ias.edu>
> Date:   Mon Dec 24 01:02:22 2012 +0000
> 
>     When the VersionControlObserver is deleted, its member variable
>     m_updateItemStatesThread is deleted too. If it has not yet finished,
>     this will lead to a crash.
>     
>     BUG: 302264
>     FIXED-IN: 4.10
> 
> commit d35b01694ea8a9e695131d8f59e3e2bfd1863d02
> Author: Simeon Bird <spb41 at ias.edu>
> Date:   Sun Dec 23 23:45:36 2012 +0000
> 
>     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 ae54fd78ea17332a60b601f4916b004fa9572fef
> Author: Simeon Bird <spb41 at ias.edu>
> Date:   Sun Dec 23 23:45:04 2012 +0000
> 
>     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.
> 
> For real! Probably a fix! I'll test this a bit more to be sure though.
> 
> 
> 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
> -------
> 
> Compiles, no crash yet. Will test more though.
> 
> 
> Thanks,
> 
> Simeon Bird
> 
>

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