Fix segfaults in OutputWidget
ClosedPublic

Authored by antonanikin on Sep 7 2018, 9:19 AM.

Details

Summary

This patch fixes regressions provided by D14931.

Steps to reproduce (my system is neon/bionic with Qt 5.11.1 and KDevelop/master):

  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 called from parent's destructor when m_tabwidget/m_stackwidget is deleted so we have destroyed m_views hash and segfault as a result.
  1. 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::OneView/HistoryView/MultipleView) with no segfaults. All our QTreeView/QSortFilterProxyModel objects are deleted (no memory leaks).

Diff Detail

Repository
R32 KDevelop
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2566
Build 2584: arc lint + arc unit
antonanikin created this revision.Sep 7 2018, 9:19 AM
Restricted Application added a project: KDevelop. · View Herald TranscriptSep 7 2018, 9:19 AM
antonanikin requested review of this revision.Sep 7 2018, 9:19 AM
antonanikin edited the summary of this revision. (Show Details)Sep 7 2018, 9:21 AM
antonanikin edited the test plan for this revision. (Show Details)
antonanikin edited the test plan for this revision. (Show Details)

As an author of D14931 I need to apologize for passing this bug and clear a thing about the 2nd case.
In the first version of the aforementioned patch (https://phabricator.kde.org/D14931?id=40016) I used raw pointers instead of QSharedPointer and was calling helper FilteredView::destroy() method in OutputWidget::removeOutput() in order not to store potentially big view and proxy model objects in memory when we don't need them anymore. This shouldn't cause double deletion because children object should remove itself from object hierarchy when being deleted (as far as I understand). Maybe you can use it too if you want.

  • Return QSharedPointers back
  • Delete views/models on output remove
  • Patch code simplify

As an author of D14931 I need to apologize for passing this bug

Hello, Vyacheslav. It's not a problem :)

and clear a thing about the 2nd case....

Thanks for explanation. Double-free problem is caused not by QSharedPointer as-is, but by it's properties and usage pattern. I'm add comments in new destructor which describes the problem.

In any case thanks for your review.

I have seen related crashes, and think I understand the basic reasoning. Sadly right now I fail to reproduce the crashes though, no idea why, so I am also unsure if I understood the problem :)
So just leaving initial comments, to be corrected during discussion.

plugins/standardoutputview/outputwidget.cpp
204

Isn't m_views destroyed (and thus implicitely cleared) before reaching the ~QWidget and ~QObject destructors where then any QObject-child-tree memory management is done?
So do we really need to explicitly call this here?

209

This here might be more the actual issue, preventing the updateFilter slot being invoked during destruction because tabwidget or stackwidget emitting signals on their subwidgets being removed,

Not sure what is the better approach, only disconnecting from the currentChanged signals or straight deleting the widgets. Personally I tend to only handle the connections, and leave memory management of widgets completly to the Qt internals.

Hi, Friedrich. Simple detaching doesn't remove segfaults. Current version also keep segfaults :(

Steps to reproduce:

  1. Start KDevelop and load some project
  2. Start project build
  3. Type something in the build output view filter edit
  4. Close KDevelop
  5. Segfault happens

It's happens because proxy model destroyed before our destructor by upper-levels (see backtrace later). So I suggest to completely drop QSharedPointer since it's usage here is overkill and produce very fragile code.

We can use plain pointers as in my first revision - it works well without memleaks and segfaults. I'll add code to destroy our model+model on output removing (see first message from @vkorneev) and publish new revision (which will be "old" first variant :) )

Backtrace of proxy model destroy (catched with qick and dirty hack
connect(proxyModel, &QObject::destroyed, this, []() { qDebug() << "proxyModel::destroyed" << 1/0; });):

#0  0x00007fffa22e4a63 in QDebug::maybeSpace() (this=<optimized out>) at /usr/include/x86_64-linux-gnu/qt5/QtCore/qdebug.h:125
#1  0x00007fffa22e4a63 in QDebug::operator<<(char const*) (t=<optimized out>, this=<optimized out>)
    at /usr/include/x86_64-linux-gnu/qt5/QtCore/qdebug.h:153
#2  0x00007fffa22e4a63 in OutputWidget::<lambda()>::operator() (__closure=<optimized out>)
    at /home/htower/develop/kde_qt/kdevelop/plugins/standardoutputview/outputwidget.cpp:679
#3  0x00007fffa22e4a63 in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, OutputWidget::outputFilter(const QString&)::<lambda()> >::call (f=..., arg=<optimized out>) at /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:128
#4  0x00007fffa22e4a63 in QtPrivate::Functor<OutputWidget::outputFilter(const QString&)::<lambda()>, 0>::call<QtPrivate::List<>, void> (f=..., arg=<optimized out>) at /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:238
#5  0x00007fffa22e4a63 in QtPrivate::QFunctorSlotObject<OutputWidget::outputFilter(const QString&)::<lambda()>, 0, QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase *, QObject *, void **, bool *) (which=1, this_=<optimized out>, r=<optimized out>, a=<optimized out>, ret=<optimized out>)
    at /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:421
#6  0x00007ffff4ecbbef in QMetaObject::activate(QObject*, int, int, void**) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#7  0x00007ffff4ecc24f in QObject::destroyed(QObject*) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#8  0x00007ffff4ed2bca in QObject::~QObject() () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#9  0x00007ffff4e6b969 in QSortFilterProxyModel::~QSortFilterProxyModel() () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#10 0x00007ffff4ec96ab in QObjectPrivate::deleteChildren() () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#11 0x00007ffff4ed314b in QObject::~QObject() () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#12 0x00007fffea8e5c79 in KDevelop::OutputModel::~OutputModel() (this=0x555557fe4c60, __in_chrg=<optimized out>)
    at /home/htower/develop/kde_qt/kdevelop/kdevplatform/outputview/outputmodel.h:36
#13 0x00007ffff4ec96ab in QObjectPrivate::deleteChildren() () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#14 0x00007ffff4ed314b in QObject::~QObject() () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#15 0x00007fffa22eb9eb in OutputData::~OutputData() (this=0x55555800b640, __in_chrg=<optimized out>)
    at /home/htower/develop/kde_qt/BUILD/kdevelop/plugins/standardoutputview/kdevstandardoutputview_autogen/EWIEGA46WW/../../../../../../kdevelop/plugins/standardoutputview/toolviewdata.h:38
#16 0x00007fffa22eb9eb in OutputData::~OutputData() (this=0x55555800b640, __in_chrg=<optimized out>)
    at /home/htower/develop/kde_qt/BUILD/kdevelop/plugins/standardoutputview/kdevstandardoutputview_autogen/EWIEGA46WW/../../../../../../kdevelop/plugins/standardoutputview/toolviewdata.h:38
#17 0x00007ffff4ec96ab in QObjectPrivate::deleteChildren() () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#18 0x00007ffff4ed314b in QObject::~QObject() () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#19 0x00007fffa22ea849 in ToolViewData::~ToolViewData() (this=0x555557ff2a90, __in_chrg=<optimized out>)
    at /home/htower/develop/kde_qt/kdevelop/plugins/standardoutputview/toolviewdata.cpp:65
#20 0x00007ffff4ec96ab in QObjectPrivate::deleteChildren() () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#21 0x00007ffff4ed314b in QObject::~QObject() () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#22 0x00007fffa22e1669 in StandardOutputView::~StandardOutputView() (this=0x555556226ad0, __in_chrg=<optimized out>)
    at /home/htower/develop/kde_qt/kdevelop/plugins/standardoutputview/standardoutputview.cpp:94
#23 0x00007ffff7a99c2f in KDevelop::PluginController::unloadPlugin(KDevelop::IPlugin*, KDevelop::PluginController::PluginDeletion) (this=this@entry=0x555555edbf00, plugin=0x555556226ad0, deletion=deletion@entry=KDevelop::PluginController::Now)
    at /home/htower/develop/kde_qt/kdevelop/kdevplatform/shell/plugincontroller.cpp:477
#24 0x00007ffff7a99df7 in KDevelop::PluginController::cleanup() (this=0x555555edbf00)
    at /home/htower/develop/kde_qt/kdevelop/kdevplatform/shell/plugincontroller.cpp:358
#25 0x00007ffff7aaa83c in KDevelop::Core::cleanup() (this=this@entry=0x555555b3a050)
    at /home/htower/develop/kde_qt/kdevelop/kdevplatform/shell/core.cpp:429
#26 0x00007ffff7aaab28 in KDevelop::Core::shutdown() (this=0x555555b3a050) at /home/htower/develop/kde_qt/kdevelop/kdevplatform/shell/core.cpp:385
#27 0x00007ffff7a8940d in KDevelop::MainWindow::~MainWindow() (this=this@entry=0x555555d1b480, __in_chrg=<optimized out>, __vtt_parm=<optimized out>)
    at /home/htower/develop/kde_qt/kdevelop/kdevplatform/shell/mainwindow.cpp:159
#28 0x00007ffff7a89469 in KDevelop::MainWindow::~MainWindow() (this=0x555555d1b480, __in_chrg=<optimized out>, __vtt_parm=<optimized out>)
    at /home/htower/develop/kde_qt/kdevelop/kdevplatform/shell/mainwindow.cpp:163
#29 0x00007ffff4ecc660 in QObject::event(QEvent*) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#30 0x00007ffff5c69213 in QWidget::event(QEvent*) () at /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#31 0x00007ffff5d708d0 in QMainWindow::event(QEvent*) () at /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#32 0x00007ffff0dc5c77 in KMainWindow::event(QEvent*) () at /usr/lib/x86_64-linux-gnu/libKF5XmlGui.so.5
#33 0x00007ffff0e0abb5 in KXmlGuiWindow::event(QEvent*) () at /usr/lib/x86_64-linux-gnu/libKF5XmlGui.so.5
#34 0x00007ffff5c29e8c in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#35 0x00007ffff5c3145f in QApplication::notify(QObject*, QEvent*) () at /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#36 0x00007ffff4e9cab8 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#37 0x00007ffff4e9f5fd in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#38 0x00007ffff4ef7453 in  () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#39 0x00007fffeb20d287 in g_main_context_dispatch () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
#40 0x00007fffeb20d4c0 in  () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
#41 0x00007fffeb20d54c in g_main_context_iteration () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
#42 0x00007ffff4ef6a7f in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#43 0x00007fffcf6c7ed1 in  () at /usr/lib/x86_64-linux-gnu/libQt5XcbQpa.so.5
#44 0x00007ffff4e9adea in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#45 0x00007ffff4ea3fa0 in QCoreApplication::exec() () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#46 0x00005555555601c0 in main(int, char**) (argc=<optimized out>, argv=<optimized out>) at /home/htower/develop/kde_qt/kdevelop/app/main.cpp:849

So I suggest to completely drop QSharedPointer since it's usage here is overkill and produce very fragile code.

My vague feeling would agree from when I had read the code, so looking forward to a new patch, thanks for tackling this.

  • Drop QSharedPointer usage
  • More auto

New version works well - no segfaults, no memory leaks.

kfunk added a subscriber: kfunk.Sep 14 2018, 6:36 AM

Note: Sorry for recommending QSP before. Thanks for working on this, @antonanikin. I'll let Friedrich review this :)

Quick first feedback, will take a closer look in the evening. Positive first impression so far :)

plugins/standardoutputview/outputwidget.cpp
640

Please no drive-by changes to lines not touched otherwise, especially non-whitespace changes. That make the "git blame" annotations confusing.

653–654

Unrelated change?

antonanikin marked 2 inline comments as done.
  • Drop unrelated changes
  • Drop another unrelated change
  • Drop last unrelated changes :)
kossebau accepted this revision.Sep 14 2018, 4:24 PM

Patch looks good to me also on semantic reading. This should remove the crashes we have seen, and make the code less unusual when it comes to QWidget object handling.

As I think the mentioned bug is related and have seen the same backtrace meanwhile again as well, please add a line

BUG: 398615

to the summary of this review request, so arc (if you use it) will pick up that string when amending the patch with the latest text from here, and the KDE server commit push hooks then do the automatic closing of that bug once they process that commit message (just telling in case you do not know yet) :)

This revision is now accepted and ready to land.Sep 14 2018, 4:24 PM
antonanikin edited the summary of this revision. (Show Details)Sep 15 2018, 12:24 AM
This revision was automatically updated to reflect the committed changes.