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

[Milian Wolff]

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



plugins/patchreview/patchreview.cpp
<https://git.reviewboard.kde.org/r/120420/#comment50444>

    I don't get this check, why only delete this one? what happens to the other patches? leaking is not an option
    
    also, couldn't this just be a `setPatch(nullptr)` here instead of copying the code again?



plugins/patchreview/patchreview.cpp
<https://git.reviewboard.kde.org/r/120420/#comment50445>

    unrelated



shell/sessioncontroller.cpp
<https://git.reviewboard.kde.org/r/120420/#comment50446>

    under what circumstances would you get here without a valid document? since the arg is not a smart pointer, a queued connection would still see a "non-zero" document pointer, even when the document got destroyed. was this what you try to prevent here? won't work though.



vcs/vcspluginhelper.cpp
<https://git.reviewboard.kde.org/r/120420/#comment50447>

    again, can this really happen? when? why now, and not before?



vcs/vcspluginhelper.cpp
<https://git.reviewboard.kde.org/r/120420/#comment50448>

    also looks wrong. the list should never contain zero items. if it does, something else is wrong



vcs/widgets/vcsdiffpatchsources.cpp
<https://git.reviewboard.kde.org/r/120420/#comment50449>

    remove



vcs/widgets/vcsdiffpatchsources.cpp
<https://git.reviewboard.kde.org/r/120420/#comment50450>

    can you remove the `.data()` here?



vcs/widgets/vcsdiffpatchsources.cpp
<https://git.reviewboard.kde.org/r/120420/#comment50451>

    remove or use `kDebug() << this;`



vcs/widgets/vcsdiffpatchsources.cpp
<https://git.reviewboard.kde.org/r/120420/#comment50452>

    remove



vcs/widgets/vcsdiffwidget.cpp
<https://git.reviewboard.kde.org/r/120420/#comment50453>

    remove and merge the if with the else above:
    
            ...
        } else if (patch) {
            patch->deleteLater();
        }


- Milian Wolff


On Dec. 19, 2014, 5: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, 5: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/20141221/455ab2cf/attachment-0001.html>