[KDE/Mac] Review Request 120150: [OS X] prevent another crash after finishing a code difference review

[Milian Wolff]

> On Sept. 12, 2014, 11:08 p.m., Kevin Funk wrote:
> > I've just a closer look at the code. It looks indeed unsafe to delete patch source which are *no* LocalPatchSources, (because others may access dangling pointers then, see 'showVcsDiff' function).
> > 
> > But: Is the `deleteLater` still necessary now? Is checking against `qobject_cast<LocalPatchSource*>` and then just delete'ing not enough?
> 
> René J.V. Bertin wrote:
>     TBH, that's a bit over my head ... what does `qobject_cast<LocalPatchSource*>` do? The code currently checks if it returns true to delete/deleteLater `m_patch`, isn't that the case when `m_patch` IS a LocalPatchSource?
>     
>     Also, if access through a dangling pointer, e.g. by showVcsDiff, were to explain the crashes I've seen, why would it not crash on Linux too? Unless that's a result of differences in event handling, which kind of takes us back to my own hunch...

Yes, it could crash on Linux, too. We've seen many reports about crashes in this code base so it could easily be that there are even more issues hidden in it. Generally though, it might be a race or memory corruption which might or might not crash randomly. Generally, we could try to cleanup the codebase and use smartpointers or direct QObject parent/child relationships to prevent such issues.


- Milian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120150/#review66380
-----------------------------------------------------------


On Sept. 12, 2014, 8:30 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120150/
> -----------------------------------------------------------
> 
> (Updated Sept. 12, 2014, 8:30 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDevelop and Olivier Goffart.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> In https://reviewboard.kde.org/r/120081/ I proposed an (accepted) approach to prevent kdevelop from crashing after closing the patch review ("git/show differences") toolview. I had another of those crashes after heavier-than-usual perusal of the toolview, despite the previous patch. The attached patch replaces all `delete`s of `QObject` derived class instances with `deleteLater()`.
> 
> 
> Diffs
> -----
> 
>   plugins/patchreview/patchreview.cpp 18b63db 
> 
> Diff: https://git.reviewboard.kde.org/r/120150/diff/
> 
> 
> Testing
> -------
> 
> kdevplatform git/kde4-legacy on KDE/MacPorts OS X 10.6.8 . 
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20140913/7b5a6971/attachment.html>