From 6330bb82bb742a60ab75c96bb6100d64a5fb54de Mon Sep 17 00:00:00 2001
From: Tomaz Canabrava <tcanabrava@kde.org>
Date: Sun, 16 Sep 2018 13:59:37 -0400
Subject: [PATCH] Fixes crashes related to closing tabs with splits

Summary:
If you moved the tabs to splits in a way that you have a different
number of tabs in each split, and started closing it, the count()
of the tabs would be different and we would hit an assert.

The fix is simple: don't separate the logic between tabEmpty and
tabDestroyed, if the tab is empty it will be destroyed but we can
treat everything in the tabEmpty signal, this way we will never hit
a dangling pointer.

Because of that it was possible that a splitView had a invalid
activeWidget for a microsecond, when it's deleting itself, so
instead of asserting if we have no active view, I choose to return
a empty list of properties.

Reviewers: hindenburg, ngraham, sandsmark

Reviewed By: hindenburg

Subscribers: konsole-devel

Tags: #konsole

Differential Revision: https://phabricator.kde.org/D15379
---
 src/ViewContainer.cpp |  4 ++--
 src/ViewManager.cpp   |  4 +++-
 src/ViewSplitter.cpp  | 28 ++++++++--------------------
 src/ViewSplitter.h    |  3 ---
 4 files changed, 13 insertions(+), 26 deletions(-)

diff --git a/src/ViewContainer.cpp b/src/ViewContainer.cpp
index 0eee05055..a3a92ff80 100644
--- a/src/ViewContainer.cpp
+++ b/src/ViewContainer.cpp
@@ -90,6 +90,8 @@ TabbedViewContainer::TabbedViewContainer(ViewManager *connectedViewManager, QWid
     connect(this, &TabbedViewContainer::currentChanged, this, [this](int index) {
         if (index != -1) {
             widget(index)->setFocus();
+        } else {
+            deleteLater();
         }
     });
 
@@ -145,8 +147,6 @@ TabbedViewContainer::~TabbedViewContainer()
         auto view = widget(i);
         disconnect(view, &QWidget::destroyed, this, &Konsole::TabbedViewContainer::viewDestroyed);
     }
-
-    emit destroyed(this);
 }
 
 void TabbedViewContainer::moveTabToWindow(int index, QWidget *window)
diff --git a/src/ViewManager.cpp b/src/ViewManager.cpp
index 8e146a9ce..2347b2195 100644
--- a/src/ViewManager.cpp
+++ b/src/ViewManager.cpp
@@ -877,8 +877,10 @@ QList<ViewProperties *> ViewManager::viewProperties() const
     QList<ViewProperties *> list;
 
     TabbedViewContainer *container = _viewSplitter->activeContainer();
+    if (container == nullptr) {
+        return {};
+    }
 
-    Q_ASSERT(container);
     list.reserve(container->count());
 
     for(int i = 0, end = container->count(); i < end; i++) {
diff --git a/src/ViewSplitter.cpp b/src/ViewSplitter.cpp
index 4d78d2be9..43cbe9231 100644
--- a/src/ViewSplitter.cpp
+++ b/src/ViewSplitter.cpp
@@ -23,6 +23,7 @@
 #include "ViewSplitter.h"
 
 // Qt
+#include <QDebug>
 
 // Konsole
 #include "ViewContainer.h"
@@ -88,13 +89,6 @@ ViewSplitter *ViewSplitter::activeSplitter()
 void ViewSplitter::registerContainer(TabbedViewContainer *container)
 {
     _containers << container;
-    // 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(TabbedViewContainer*)), this,
-            SLOT(containerDestroyed(TabbedViewContainer*)));
     connect(container, SIGNAL(empty(TabbedViewContainer*)), this, SLOT(containerEmpty(TabbedViewContainer*)));
 }
 
@@ -179,10 +173,15 @@ void ViewSplitter::addContainer(TabbedViewContainer *container, Qt::Orientation
     }
 }
 
-void ViewSplitter::containerEmpty(TabbedViewContainer * /*container*/)
+void ViewSplitter::containerEmpty(TabbedViewContainer * myContainer)
 {
+    _containers.removeAll(myContainer);
+    if (count() == 0) {
+        emit empty(this);
+    }
+
     int children = 0;
-    foreach (TabbedViewContainer *container, _containers) {
+    foreach (auto container, _containers) {
         children += container->count();
     }
 
@@ -191,17 +190,6 @@ void ViewSplitter::containerEmpty(TabbedViewContainer * /*container*/)
     }
 }
 
-void ViewSplitter::containerDestroyed(TabbedViewContainer *container)
-{
-    Q_ASSERT(_containers.contains(container));
-
-    _containers.removeAll(container);
-
-    if (count() == 0) {
-        emit empty(this);
-    }
-}
-
 void ViewSplitter::activateNextContainer()
 {
     TabbedViewContainer *active = activeContainer();
diff --git a/src/ViewSplitter.h b/src/ViewSplitter.h
index 7d7e7edb2..70ca0040e 100644
--- a/src/ViewSplitter.h
+++ b/src/ViewSplitter.h
@@ -170,9 +170,6 @@ private:
     void updateSizes();
 
 private Q_SLOTS:
-    // Called to indicate that a child ViewContainer has been deleted
-    void containerDestroyed(TabbedViewContainer *container);
-
     // Called to indicate that a child ViewContainer is empty
     void containerEmpty(TabbedViewContainer *container);