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

[René J.V. Bertin]

> On Sept. 13, 2014, 1:08 a.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...
> 
> Milian Wolff wrote:
>     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.
> 
> René J.V. Bertin wrote:
>     Some fresh observations on this issue, in the hope they might give one of you a brilliant idea ;)
>     
>     I keep getting unpredictable crashes when closing the patchreview toolview through the "Finish Review" button, but mostly (or even only) when having exported the patch (to file or RB).
>     
>     It appears however that the crash doesn't occur when I first close all the open review windows, and then use the "back" button at the top left of the pinkish main window. That would suggest that whatever happens has to do with those windows : closing them through the window manager is likely to propagate the close event "from the bottom up", with objects being deleted after and because the window has been closed. It also leads me to think that those pending events I mentioned so often above could actually be events that are due to sending the window a close message.

In case anyone is still interested:

a near-identical crash occurs when hitting the "Commit" button in the git->commit... toolview. I evidently didn't try to reproduce that crash (and end up with multiple commits in my working copy), but I did notice that here too it was enough to close all edit windows (Command-Alt-W) before hitting the button.

Which raises the question: is there a way for the toolview plugin to send close messages to the edit windows it opened in the button's callback, potentially wait for those events to have been handled, and then continue with the regular processing? I'm specifically talking about the signal/message they would receive from the window manager, and not closing by deleting the corresponding objects.


- René J.V.


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


On Sept. 12, 2014, 10: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, 10: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/kdevelop-devel/attachments/20140926/4ddbaddf/attachment.html>