QDialog on stack+exec and dbus quit crash is no more

[Milian Wolff]

On Wednesday 09 July 2014 10:58:50 David Faure wrote:
> On Thursday 12 December 2013 20:01:42 Albert Astals Cid wrote:
> > El Dijous, 12 de desembre de 2013, a les 19:57:25, David Faure va 
escriure:
> > > On Monday 11 November 2013 21:04:35 Jan Kundrát wrote:
> > > > On Monday, 11 November 2013 19:17:22 CEST, Albert Astals Cid wrote:
> > > > > Not sure you're understanding what i say, we have an explicit check
> > > > > about
> > > > > QDialog on stack+exec that says "it will crash if you dbus quit".
> > > > 
> > > > We've chatted about this with Albert on IRC. My understanding of this
> > > > is
> > > > that there are many situations which can trigger destruction of the
> > > > parent
> > > > object; one of them involves triggering the QAction "game_quit" via
> > > > DBUS.
> > > > As such, I consider the EBN check a valid warning, similar to compiler
> > > > warning about other dubious code constructs like no parentheses around
> > > > &&
> > > > and ||, or unused variables -- might be valid in some circumstances,
> > > > but
> > > > should raise an eyebrow during review, and it might make sense to
> > > > strive
> > > > to
> > > > make the code warning-free.
> > > > 
> > > > However, there's another bug, probably in the kdeui code. When I
> > > > trigger
> > > > that QAction, this is what valgrind reports:
> > > > 
> > > > ==355524== Invalid write of size 1
> > > > ==355524==    at 0x5E3A6D7:
> > > > QMenuPrivate::activateCausedStack(QList<QPointer<QWidget> > const&,
> > > > QAction*, QAction::ActionEvent, bool) (qobject_p.h:321)
> > > > ==355524==    by 0x5E412F2: QMenuPrivate::activateAction(QAction*,
> > > > QAction::ActionEvent, bool) (qmenu.cpp:1130)
> > > > ==355524==    by 0x5424A1A: KMenu::mouseReleaseEvent(QMouseEvent*)
> > > > (kmenu.cpp:464)
> > > > ==355524==    by 0x5A000DB: QWidget::event(QEvent*) (qwidget.cpp:8376)
> > > > ==355524==    by 0x5E429DA: QMenu::event(QEvent*) (qmenu.cpp:2481)
> > > > ==355524==    by 0x59A03BB:
> > > > QApplicationPrivate::notify_helper(QObject*,
> > > > QEvent*) (qapplication.cpp:4562)
> > > > ==355524==    by 0x59A6168: QApplication::notify(QObject*, QEvent*)
> > > > (qapplication.cpp:4105)
> > > > ==355524==    by 0x533F265: KApplication::notify(QObject*, QEvent*)
> > > > (kapplication.cpp:311)
> > > > ==355524==    by 0x712AA3B: QCoreApplication::notifyInternal(QObject*,
> > > > QEvent*) (qcoreapplication.cpp:949)
> > > > ==355524==    by 0x59A1461:
> > > > QApplicationPrivate::sendMouseEvent(QWidget*,
> > > > QMouseEvent*, QWidget*, QWidget*, QWidget**, QPointer<QWidget>&, bool)
> > > > (qcoreapplication.h:231)
> > > > ==355524==    by 0x5A31BA0: QETWidget::translateMouseEvent(_XEvent
> > > > const*)
> > > > (qapplication_x11.cpp:4451)
> > > > ==355524==    by 0x5A303B9: QApplication::x11ProcessEvent(_XEvent*)
> > > > (qapplication_x11.cpp:3640)
> > > > ==355524==  Address 0x165b1d21 is 529 bytes inside a block of size 728
> > > > free'd
> > > > ==355524==    at 0x4C2AE0C: operator delete(void*)
> > > > (vg_replace_malloc.c:480)
> > > > ==355524==    by 0x714715F: QObject::~QObject() (qscopedpointer.h:62)
> > > > ==355524==    by 0x59FA0C9: QWidget::~QWidget() (qwidget.cpp:1554)
> > > > ==355524==    by 0x54248CC: KMenu::~KMenu() (kmenu.cpp:168)
> > > > ==355524==    by 0x7140783: QObjectPrivate::deleteChildren()
> > > > (qobject.cpp:1907)
> > > > ==355524==    by 0x59FA02C: QWidget::~QWidget() (qwidget.cpp:1679)
> > > > ==355524==    by 0x5421315: KMainWindow::~KMainWindow()
> > > > (kmainwindow.cpp:467)
> > > > ==355524==    by 0x11A5AE: KMinesMainWindow::~KMinesMainWindow() (in
> > > > /home/jkt/work/prog/kde/kmines/kmines)
> > > > ==355524==    by 0x7147AC7: QObject::event(QEvent*) (qobject.cpp:1175)
> > > > ==355524==    by 0x59FFDFA: QWidget::event(QEvent*) (qwidget.cpp:8846)
> > > > ==355524==    by 0x5E13C8A: QMainWindow::event(QEvent*)
> > > > (qmainwindow.cpp:1478)
> > > > ==355524==    by 0x546E0C7: KXmlGuiWindow::event(QEvent*)
> > > > (kxmlguiwindow.cpp:126)
> > > > 
> > > > I have no idea why the DBUS calls to QAction are handled as mouse
> > > > events,
> > > > but I suspect something fishy is going on here.
> > > 
> > > No, you're deleting the mainwindow while having the mouse over the menu,
> > > so
> > > when Qt finishes the mouse event handling it sends a mouse event to a
> > > deleted widget, probably because all this happens within a nested event
> > > loop, so Qt's main event loop didn't expect the widget to be already
> > > deleted at that point.
> > > 
> > > (I'm confused about the testcase though - this is "calling dbus quit
> > > while
> > > having the mouse over a menu?")
> > 
> > There was no mouse over the menu at all involved. Just dbus invoking the
> > action.
> 
> Ah! Now I remember (yes, 7 months later).
> This crash matches one of the items in my oversized todo list:
> 
> "Qt bug: QMenu crashes when deleted from one of its actions. kde bug 249884,
> RecursionGuard member accessed after deletion, weakpointer guard needed..."
> 
> Millian must have worked around the issue in kdevelop (the BR isn't really
> clear about the fix). I guess using deleteLater().

It's been some time since I last touched anything in that regard. 

But yes, do take a look at the workaround patches I commited to fix

https://bugs.kde.org/show_bug.cgi?id=249884

and maybe also

https://bugs.kde.org/show_bug.cgi?id=311447

Bye
-- 
Milian Wolff
mail at milianw.de
http://milianw.de