Review Request 120420: [OS X] a more complete approach to prevent the crash after finishing a code-difference review or git/commit

Mon Dec 22 00:15:48 UTC 2014

> On Dec. 21, 2014, 10:41 p.m., Milian Wolff wrote:
> > vcs/vcspluginhelper.cpp, line 206
> > <https://git.reviewboard.kde.org/r/120420/diff/4/?file=333845#file333845line206>
> >
> >     again, can this really happen? when? why now, and not before?
> 
> René J.V. Bertin wrote:
>     same thing, though in this case it's almost impossible to say whether the crashes I saw were related or not to the issue at hand.
>     When and why, I have no idea. As I said, I haven't been able to debug the crashes post-mortem, and it's not an option to run the IDE with full debug and zero optimisation for something that happens only occasionally and without a well-defined trigger.
> 
> Milian Wolff wrote:
>     you should really try this. I've been running KDevelop in debug mode since before the first 4.0 alpha release. Yes, its slower than using a RelWithDebInfo, but no it's not such a big deal that it would stop you from using it.
>     
>     And even then, RelWithDebInfo should give you decent backtraces and quite some performance. So again, I wonder how you work? Just looking at code and blindly adding checks is not going to work out, really. You'll need backtraces which proof why a change is required.

I do run with RelWithDebInfo. Backtraces aren't always enough though, when the optimisation has suppressed key variables.
The other thing I fear with this particular kind of issue is that it is a kind of "heisenbug", one that disappears (or moves) when you build without optimisation.
Anyway I'll be removing these changes from the RR, as I said they shouldn't have been included.

- René J.V.

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

On Dec. 19, 2014, 6:52 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120420/
> -----------------------------------------------------------
> 
> (Updated Dec. 19, 2014, 6:52 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDevelop.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> This RR replaces https://git.reviewboard.kde.org/r/120150/, itself a follow-up to RR https://reviewboard.kde.org/r/120081/ . In that RR I proposed an (accepted & submitted ) approach to prevent kdevelop from crashing after closing the patch review ("git/show differences") toolview.
> 
> This turned out to be insufficient, the most likely culprit being a nested event loop. In my most recent bug hunting I noticed that `closeReview` is called through Qt's mouse-eventh handling function when the "Finish Review" button (or "Commit", when doing a git->commit) is pressed. The crash I have been experiencing occurs in that function (or at least not far down the call stack from that function).
> I had already noticed that the crash never occurred when I closed all document views first (the patch file and all modified files), e.g. by using the "Close All" menu action.
> 
> So:
> - `closeReview` closes all open documents (minus the patchfile) via `qDeleteAll(m_highlighters)` or by calling `m_highlighters.erase(iterator)` repeatedly. Documents are thus closed by deleting the "content object" that represents them, letting that fact filter back to the UI
> - closing documents via the UI takes the opposite route, and is more in line with a modern GUI framework (at least OS X/Cocoa) where everything happens because of an event in the user interface. You receive an event (message) when the user closes a document, the framework handles most UI-related stuff for you, and if you have to dispose of document-related non-gui data you can do that for example just before the widget actually closes (`windowWillClose`, `menuWillClose`, `applicationWillClose` etc. messages in Cocoa).
> 
> This led to the current patch, which cycles through and closes the `Sublime::View`s associated with the window area, and then lets Qt send and process all events that are pending and/or result from closing those windows. Only then does it proceed the regular course of action (calling `removeHighLighting()` which ought to be no longer necessary).
> 
> The only change for the user is that the pinkish patch review plugin background (with the "back to code" button) is visible briefly before the toolview is closed.
> 
> I understand that this crash (or a closely related one) does not only occur on OS X but also on Linux, so I did not put the new code in a conditional block.
> 
> 
> Diffs
> -----
> 
>   plugins/grepview/grepviewplugin.cpp 7e3d933 
>   plugins/patchreview/patchreview.cpp 18b63db 
>   shell/sessioncontroller.cpp ae4e355 
>   vcs/vcspluginhelper.cpp 26ab57c 
>   vcs/widgets/vcsdiffpatchsources.cpp c3e2dae 
>   vcs/widgets/vcsdiffwidget.cpp 54787b9 
> 
> Diff: https://git.reviewboard.kde.org/r/120420/diff/
> 
> 
> Testing
> -------
> 
> kdevplatform git/kde4-legacy on KDE/MacPorts OS X 10.6.8 with kdelibs 4.14.1
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20141222/b8131df3/attachment.html>