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

[René J.V. Bertin]

> On Oct. 9, 2014, 2:19 p.m., Milian Wolff wrote:
> > plugins/patchreview/patchreview.cpp, line 348
> > <https://git.reviewboard.kde.org/r/120420/diff/3/?file=316725#file316725line348>
> >
> >     nested eventloops are very evil, are you sure this helps and does not introduce new issues?

Yes, they are, but in this case the events should concern only the windows being shown and of course they are already posted so we'd be getting them anyway. The theory is that it's better to make sure we deal with them before going on and destroy the context for which they're intended.
I haven't seen new issues (but sadly I can't affirm that this patch removes all chances of crashing when closing the plugin).


- René J.V.


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


On Oct. 6, 2014, 12:03 a.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120420/
> -----------------------------------------------------------
> 
> (Updated Oct. 6, 2014, 12:03 a.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/patchreview/patchreview.cpp 18b63db 
> 
> 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/20141009/999777ec/attachment.html>