[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-commits
Subject:    [konsole/frameworks] src: Fix crash on close
From:       Aurélien Gâteau <agateau () kde ! org>
Date:       2014-06-20 12:04:47
Message-ID: E1WxxYl-0000d0-4b () scm ! kde ! org
[Download RAW message or body]

Git commit dd1b2b4df04f13bb2a9f3bcef106dc3604c5fc1a by Aurélien Gâteau.
Committed on 19/06/2014 at 14:24.
Pushed by gateau into branch 'frameworks'.

Fix crash on close

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.

BUG: 331724
REVIEW: 118839

M  +15   -29   src/ViewContainer.cpp
M  +3    -0    src/ViewContainer.h
M  +0    -20   src/ViewManager.cpp
M  +0    -1    src/ViewManager.h
M  +7    -2    src/ViewSplitter.cpp

http://commits.kde.org/konsole/dd1b2b4df04f13bb2a9f3bcef106dc3604c5fc1a

diff --git a/src/ViewContainer.cpp b/src/ViewContainer.cpp
index 79c24d5..d425e0e 100644
--- a/src/ViewContainer.cpp
+++ b/src/ViewContainer.cpp
@@ -153,37 +153,25 @@ void ViewContainer::addView(QWidget* view , ViewProperties* \
item, int index)  void ViewContainer::viewDestroyed(QObject* object)
 {
     QWidget* widget = static_cast<QWidget*>(object);
+    forgetView(widget);
+}
 
-    _views.removeAll(widget);
-    _navigation.remove(widget);
-
-    // FIXME This can result in ViewContainerSubClass::removeViewWidget() being
-    // called after the widget's parent has been deleted or partially deleted
-    // in the ViewContainerSubClass instance's destructor.
-    //
-    // Currently deleteLater() is used to remove child widgets in the subclass
-    // constructors to get around the problem, but this is a hack and needs
-    // to be fixed.
-    removeViewWidget(widget);
+void ViewContainer::forgetView(QWidget* view)
+{
+    _views.removeAll(view);
+    _navigation.remove(view);
 
-    emit viewRemoved(widget);
+    emit viewRemoved(view);
 
     if (_views.count() == 0)
         emit empty(this);
 }
+
 void ViewContainer::removeView(QWidget* view)
 {
-    _views.removeAll(view);
-    _navigation.remove(view);
-
     disconnect(view, &QWidget::destroyed, this, \
                &Konsole::ViewContainer::viewDestroyed);
-
     removeViewWidget(view);
-
-    emit viewRemoved(view);
-
-    if (_views.count() == 0)
-        emit empty(this);
+    forgetView(view);
 }
 
 const QList<QWidget*> ViewContainer::views() const
@@ -264,6 +252,7 @@ TabbedViewContainer::TabbedViewContainer(NavigationPosition \
position, ViewManage  {
     _containerWidget = new QWidget;
     _stackWidget = new QStackedWidget();
+    connect(_stackWidget.data(), &QStackedWidget::widgetRemoved, this, \
&TabbedViewContainer::widgetRemoved);  
     // The tab bar
     _tabBar = new ViewContainerTabBar(_containerWidget, this);
@@ -649,15 +638,18 @@ void TabbedViewContainer::addViewWidget(QWidget* view , int \
index)  if (navigationVisibility() == ShowNavigationAsNeeded)
         dynamicTabBarVisibility();
 }
+
 void TabbedViewContainer::removeViewWidget(QWidget* view)
 {
     if (!_stackWidget)
         return;
-    const int index = _stackWidget->indexOf(view);
+    _stackWidget->removeWidget(view);
+}
 
+void TabbedViewContainer::widgetRemoved(int index)
+{
     Q_ASSERT(index != -1);
 
-    _stackWidget->removeWidget(view);
     _tabBar->removeTab(index);
 
     if (navigationVisibility() == ShowNavigationAsNeeded)
@@ -754,11 +746,5 @@ void StackedViewContainer::removeViewWidget(QWidget* view)
 {
     if (!_stackWidget)
         return;
-    const int index = _stackWidget->indexOf(view);
-
-    Q_ASSERT(index != -1);
-    Q_UNUSED(index);
-
     _stackWidget->removeWidget(view);
 }
-
diff --git a/src/ViewContainer.h b/src/ViewContainer.h
index 60d2bd9..bd6b5cc 100644
--- a/src/ViewContainer.h
+++ b/src/ViewContainer.h
@@ -342,6 +342,8 @@ private slots:
     void searchBarDestroyed();
 
 private:
+    void forgetView(QWidget* view);
+
     NavigationVisibility _navigationVisibility;
     NavigationPosition _navigationPosition;
     QList<QWidget*> _views;
@@ -417,6 +419,7 @@ private:
     void setTabActivity(int index, bool activity);
     void renameTab(int index);
     void updateVisibilityOfQuickButtons();
+    void widgetRemoved(int index);
 
     ViewContainerTabBar* _tabBar;
     QPointer<QStackedWidget> _stackWidget;
diff --git a/src/ViewManager.cpp b/src/ViewManager.cpp
index 75473e9..359aa06 100644
--- a/src/ViewManager.cpp
+++ b/src/ViewManager.cpp
@@ -386,23 +386,6 @@ void ViewManager::sessionFinished()
         emit unplugController(_pluggedController);
 }
 
-void ViewManager::focusActiveView()
-{
-    // give the active view in a container the focus.  this ensures
-    // that controller associated with that view is activated and the \
                session-specific
-    // menu items are replaced with the ones for the newly focused view
-
-    // see the viewFocused() method
-
-    ViewContainer* container = _viewSplitter->activeContainer();
-    if (container) {
-        QWidget* activeView = container->activeView();
-        if (activeView) {
-            activeView->setFocus(Qt::MouseFocusReason);
-        }
-    }
-}
-
 void ViewManager::viewActivated(QWidget* view)
 {
     Q_ASSERT(view != 0);
@@ -761,14 +744,11 @@ void ViewManager::viewDestroyed(QWidget* view)
     Session* session = _sessionMap[ display ];
     _sessionMap.remove(display);
     if (session) {
-        display->deleteLater();
-
         if (session->views().count() == 0)
             session->close();
     }
     //we only update the focus if the splitter is still alive
     if (_viewSplitter) {
-        focusActiveView();
         updateDetachViewState();
     }
     // The below causes the menus  to be messed up
diff --git a/src/ViewManager.h b/src/ViewManager.h
index 047b7ee..ee86471 100644
--- a/src/ViewManager.h
+++ b/src/ViewManager.h
@@ -346,7 +346,6 @@ private:
     static const ColorScheme* colorSchemeForProfile(const Profile::Ptr profile);
 
     void setupActions();
-    void focusActiveView();
 
     // takes a view from a view container owned by a different manager and places it \
in  // newContainer owned by this manager
diff --git a/src/ViewSplitter.cpp b/src/ViewSplitter.cpp
index bfc727e..d558ca3 100644
--- a/src/ViewSplitter.cpp
+++ b/src/ViewSplitter.cpp
@@ -85,8 +85,13 @@ ViewSplitter* ViewSplitter::activeSplitter()
 void ViewSplitter::registerContainer(ViewContainer* container)
 {
     _containers << container;
-    connect(container , \
static_cast<void(ViewContainer::*)(ViewContainer*)>(&Konsole::ViewContainer::destroyed) \
                , this , &Konsole::ViewSplitter::containerDestroyed);
-    connect(container , &Konsole::ViewContainer::empty , this , \
&Konsole::ViewSplitter::containerEmpty); +    // Connecting to container::destroyed() \
using the new-style connection +    // syntax causes a crash at exit. I don't know \
why. Using the old-style +    // syntax works.
+    //connect(container , \
static_cast<void(ViewContainer::*)(ViewContainer*)>(&Konsole::ViewContainer::destroyed) \
, this , &Konsole::ViewSplitter::containerDestroyed); +    //connect(container , \
&Konsole::ViewContainer::empty , this , &Konsole::ViewSplitter::containerEmpty); +    \
connect(container , SIGNAL(destroyed(ViewContainer*)) , this , \
SLOT(containerDestroyed(ViewContainer*))); +    connect(container , \
SIGNAL(empty(ViewContainer*)) , this , SLOT(containerEmpty(ViewContainer*)));  }
 
 void ViewSplitter::unregisterContainer(ViewContainer* container)


[prev in list] [next in list] [prev in thread] [next in thread]