[Konsole-devel] Review Request 118839: Fix crash on close

[Aurélien Gâteau]

> On June 19, 2014, 5:30 p.m., Eike Hein wrote:
> >
> 
> Kurt Hindenburg wrote:
>     yes go ahead - you are committing to frameworks branch?  Do you think this might help in master/KDE4?
> 
> Alexander Richardson wrote:
>     I doubt this is useful for KDE4 because the actual reason for this crash seems to be is due to a Qt5 behaviour change in QPointer:
>     
>     Note that Qt 5 introduces a slight change in behavior when using QPointer.
>     
>         When using QPointer on a QWidget (or a subclass of QWidget), previously the QPointer would be cleared by the QWidget destructor. Now, the QPointer is cleared by the QObject destructor (since this is when QWeakPointers are cleared). Any QPointers tracking a widget will NOT be cleared before the QWidget destructor destroys the children for the widget being tracked.
>

@Kurt: Konsole master does not suffer from this issue, so I'd rather not fix what aren't broken

@Alexander: Oh I wasn't aware about this change. Good to know, it is indeed likely the reason for the crash.


- Aurélien


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


On June 20, 2014, 2:04 p.m., Aurélien Gâteau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118839/
> -----------------------------------------------------------
> 
> (Updated June 20, 2014, 2:04 p.m.)
> 
> 
> Review request for Konsole.
> 
> 
> Bugs: 331724
>     http://bugs.kde.org/show_bug.cgi?id=331724
> 
> 
> Repository: konsole
> 
> 
> Description
> -------
> 
> Move code responsible for 'forgetting' a view outside of code responding to the
> TerminalDisplay deletion.
> 
> This avoids a loop like this:
> 
> ~MainWindow
> => ~QStackedWidget
> => ~TerminalDisplay
> => QObject::destroyed
> => ViewContainer::viewDestroyed
> => ViewContainer::removeViewWidget
>    - internal cleanup
>    - try to remove TerminalDisplay from QStackedWidget which is being deleted and
> crash
> 
> Instead the code now does:
> 
> ~MainWindow
> => ~QStackedWidget
> => ~TerminalDisplay
> => QObject::destroyed
> => ViewContainer::viewDestroyed
> => ViewContainer::forgetView (does the internal clean up)
> 
> And if one tries to explicitly remove a view, sequence is:
> 
> ViewContainer::removeView
> => ViewContainer::forgetView
> => ViewContainer::removeViewWidget
> 
> The patch also removes ViewManager::focusActiveView() because it causes a crash
> when closing a TerminalDisplay as it tries to put the focus on the deleted
> TerminalDisplay. I initially called it through a queued connection, but realized
> it is actually not needed for focus to be passed to the correct view, so just
> removed it.
> 
> 
> Diffs
> -----
> 
>   src/ViewContainer.h 60d2bd9 
>   src/ViewContainer.cpp 79c24d5 
>   src/ViewManager.h 047b7ee 
>   src/ViewManager.cpp 75473e9 
>   src/ViewSplitter.cpp bfc727e 
> 
> Diff: https://git.reviewboard.kde.org/r/118839/diff/
> 
> 
> Testing
> -------
> 
> Started and closed Konsole, created and deleted tabs, created and deleted split views. No crash.
> 
> 
> Thanks,
> 
> Aurélien Gâteau
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/konsole-devel/attachments/20140620/1ddf715c/attachment-0001.html>