From kde-commits Sat Sep 15 00:28:39 2018 From: Anton Anikin Date: Sat, 15 Sep 2018 00:28:39 +0000 To: kde-commits Subject: [kdevelop] plugins/standardoutputview: Fix segfaults in OutputWidget Message-Id: X-MARC-Message: https://marc.info/?l=kde-commits&m=153697133431702 Git commit f856a9a63910a7e9f2fbcbcd63f85100599805ae by Anton Anikin. Committed on 15/09/2018 at 00:26. Pushed by antonanikin into branch 'master'. Fix segfaults in OutputWidget Summary: This patch fixes regressions provided by D14931. Steps to reproduce (my system is neon/bionic with Qt 5.11.1 and KDevelop/ma= ster): 1) Start KDevelop and load some project 2) Start project build 3) Close KDevelop 4) Segfault happens Main problems: 1) Incorrect signal processing. `OutputWidget::updateFilter()` slot is call= ed from parent's destructor when `m_tabwidget`/`m_stackwidget` is deleted s= o we have destroyed `m_views` hash and segfault as a result. 2) Incorrect `QSharedPointer` usage. All handled objects have `QWidget` as = a parent so we have double-free problem and and segfault as a result. BUG: 398615 Test Plan: Tested on different output widgets (`KDevelop::IOutputView::OneV= iew/HistoryView/MultipleView`) with no segfaults. All our `QTreeView`/`QSor= tFilterProxyModel` objects are deleted (no memory leaks). Reviewers: #kdevelop, kossebau Reviewed By: #kdevelop, kossebau Subscribers: kfunk, kossebau, vkorneev, kdevelop-devel Tags: #kdevelop Differential Revision: https://phabricator.kde.org/D15326 M +35 -26 plugins/standardoutputview/outputwidget.cpp M +5 -4 plugins/standardoutputview/outputwidget.h https://commits.kde.org/kdevelop/f856a9a63910a7e9f2fbcbcd63f85100599805ae diff --git a/plugins/standardoutputview/outputwidget.cpp b/plugins/standard= outputview/outputwidget.cpp index 6c7b4e496b..5bae159db5 100644 --- a/plugins/standardoutputview/outputwidget.cpp +++ b/plugins/standardoutputview/outputwidget.cpp @@ -192,6 +192,17 @@ OutputWidget::OutputWidget(QWidget* parent, const Tool= ViewData* tvdata) enableActions(); } = +OutputWidget::~OutputWidget() +{ + // Disconnect our widget to prevent updateFilter() slot calling from p= arent's destructor, + // which leads to segfault since m_views hash will be destroyed before. + if (m_tabwidget) { + m_tabwidget->disconnect(this); + } else if (m_stackwidget) { + m_stackwidget->disconnect(this); + } +} + void OutputWidget::clearModel() { auto view =3D qobject_cast(currentWidget()); @@ -254,7 +265,7 @@ void OutputWidget::removeOutput( int id ) { if( data->outputdata.contains( id ) && m_views.contains( id ) ) { - QTreeView *view =3D m_views.value(id).view.data(); + auto view =3D m_views.value(id).view; if( data->type & KDevelop::IOutputView::MultipleView || data->type= & KDevelop::IOutputView::HistoryView ) { if( data->type & KDevelop::IOutputView::MultipleView ) @@ -272,19 +283,16 @@ void OutputWidget::removeOutput( int id ) m_stackwidget->removeWidget(view); } } - } else { // KDevelop::IOutputView::OneView - /* TODO: this branch of execution has no result because of the= "m_views.remove( id );" - * after the if-else block. Need to find out which behavior ha= s sense. - */ - FilteredView& fview =3D m_views[id]; - fview.view->setModel(nullptr); - fview.view->setItemDelegate(nullptr); - if (fview.proxyModel) { - fview.proxyModel =3D QSharedPointer= (); - fview.filter =3D QString(); - } + } else { + // KDevelop::IOutputView::OneView case + // Do nothig here since our single view will be automatically = removed from layout + // during it's destroy } - m_views.remove(id); + + auto fv =3D m_views.take(id); + // remove our view with proxy model which is view's child (see out= putFilter() method). + delete fv.view; + emit outputRemoved( data->toolViewId, id ); } enableActions(); @@ -339,7 +347,7 @@ QWidget* OutputWidget::currentWidget() const widget =3D m_stackwidget->currentWidget(); } else { - widget =3D m_views.begin().value().view.data(); + widget =3D m_views.begin()->view; } return widget; } @@ -471,7 +479,7 @@ void OutputWidget::activate(const QModelIndex& index) = QTreeView* OutputWidget::createListView(int id) { - auto createHelper =3D [&]() -> QSharedPointer { + auto createHelper =3D [&]() -> QTreeView* { KDevelop::FocusedTreeView* listview =3D new KDevelop::FocusedTreeV= iew(this); listview->setEditTriggers( QAbstractItemView::NoEditTriggers ); listview->setHorizontalScrollBarPolicy(Qt::ScrollBarAlwaysOn); //A= lways enable the scrollbar, so it doesn't flash around @@ -487,10 +495,10 @@ QTreeView* OutputWidget::createListView(int id) connect(listview, &QTreeView::activated, this, &OutputWidget::acti= vate); connect(listview, &QTreeView::clicked, this, &OutputWidget::activa= te); = - return QSharedPointer(listview); + return listview; }; = - QSharedPointer listview; + QTreeView* listview =3D nullptr; if( !m_views.contains(id) ) { bool newView =3D true; @@ -502,18 +510,18 @@ QTreeView* OutputWidget::createListView(int id) = if( data->type & KDevelop::IOutputView::MultipleView ) { - m_tabwidget->addTab(listview.data(), data->outputdata.valu= e(id)->title); + m_tabwidget->addTab(listview, data->outputdata.value(id)->= title); } else { - m_stackwidget->addWidget(listview.data()); - m_stackwidget->setCurrentWidget(listview.data()); + const int index =3D m_stackwidget->addWidget(listview); + m_stackwidget->setCurrentIndex(index); } } else { if( m_views.isEmpty() ) { listview =3D createHelper(); - layout()->addWidget(listview.data()); + layout()->addWidget(listview); } else { listview =3D m_views.begin().value().view; @@ -532,14 +540,14 @@ QTreeView* OutputWidget::createListView(int id) listview =3D m_views.value(id).view; } enableActions(); - return listview.data(); + return listview; } = void OutputWidget::raiseOutput(int id) { if( m_views.contains(id) ) { - auto view =3D m_views.value(id).view.data(); + auto view =3D m_views.value(id).view; if( data->type & KDevelop::IOutputView::MultipleView ) { int idx =3D m_tabwidget->indexOf(view); @@ -646,10 +654,11 @@ void OutputWidget::outputFilter(const QString& filter) auto proxyModel =3D qobject_cast(view->model()= ); if( !proxyModel ) { - proxyModel =3D new QSortFilterProxyModel(view->model()); + // create new proxy model and make view it's parent. This allows u= s destroy view and + // it's model with "one shot" (see removeOutput() method). + fvIt->proxyModel =3D proxyModel =3D new QSortFilterProxyModel(view= ); proxyModel->setDynamicSortFilter(true); proxyModel->setSourceModel(view->model()); - fvIt->proxyModel =3D QSharedPointer(proxyMo= del); view->setModel(proxyModel); } QRegExp regExp(filter, Qt::CaseInsensitive); @@ -676,7 +685,7 @@ void OutputWidget::setTitle(int outputId, const QString= & title) { auto fview =3D m_views.value(outputId, FilteredView{}); if (fview.view && (data->type & KDevelop::IOutputView::MultipleView)) { - int idx =3D m_tabwidget->indexOf(fview.view.data()); + const int idx =3D m_tabwidget->indexOf(fview.view); if (idx >=3D 0) { m_tabwidget->setTabText(idx, title); } diff --git a/plugins/standardoutputview/outputwidget.h b/plugins/standardou= tputview/outputwidget.h index a9db4717d6..fe1f4a9383 100644 --- a/plugins/standardoutputview/outputwidget.h +++ b/plugins/standardoutputview/outputwidget.h @@ -22,9 +22,8 @@ #ifndef KDEVPLATFORM_PLUGIN_OUTPUTWIDGET_H #define KDEVPLATFORM_PLUGIN_OUTPUTWIDGET_H = -#include +#include #include -#include = #include #include @@ -55,6 +54,8 @@ class OutputWidget : public QWidget, public KDevelop::ITo= olViewActionListener = public: OutputWidget(QWidget* parent, const ToolViewData* data); + ~OutputWidget() override; + void removeOutput( int id ); void raiseOutput( int id ); public Q_SLOTS: @@ -103,8 +104,8 @@ private: int currentOutputIndex(); = struct FilteredView { - QSharedPointer view; - QSharedPointer proxyModel; + QTreeView* view =3D nullptr; + QSortFilterProxyModel* proxyModel =3D nullptr; QString filter; }; QHash::iterator findFilteredView(QAbstractItemView = *view);