MAR
26
2009

How to crash (almost) every Qt/KDE Application and how to fix it

Want to see how to crash almost every KDE application?

  • In your favorite KDE application, open any modal dialog, e.g. the configuration dialog.
  • Quit the application using D-BUS, like this:


qdbus org.kde.kontact /MainApplication quit

Chances are high that your application just bit the dust and you're greeted by Dr. Konqui.

Why does it happen? Well, let's have a look at how modal dialogs are usually created. Most often applications have some "main widget" with slots reacting to user actions, creating a modal dialog on the stack, like this:


void ParentWidget::slotDoSomething() {
SomeDialog dlg( this ); //the dlg-on-stack-variant
if ( dlg.exec() == QDialog::Accepted ) {
const QString str = dlg.someUserInput();
//do something with with str
}
}

What could possibly go wrong here? Nothing, you might say. Looks safe, right? This code is so trivial and the pattern is used in so many Qt and KDE applications. Wrong! This code is bound to cause a crash in a couple of situations, where a D-BUS call to quit() is only one of them (and a somewhat esoteric one, admittedly). Why?

While dlg.exec() is executed and the dialog is shown, your application returns to the event loop and processes any event that comes along. Which means, that anything might happen before exec() returns. Right. Anything. In the usual case, the user enters something and then presses either OK or Cancel to close the dialog, and exec() returns. He can't to anything else, as the parent widget is disabled anyway, as the dialog is modal. But in most non-trivial applications, there are more things going on: Asynchronous background jobs might complete, e.g. Konqueror might finish loading a website and replace the currently displayed Kate KPart while you're in its print dialog configuring your printout. KMail might start an interval mail check while a (modal) KMessageBox still tells you that it couldn't reach your mail server last time. Or something is triggered by a D-BUS call.

That's fine most of the time, unless in the following case: The background operation/d-bus call triggers the deletion of the dialog's parent, the ParentWidget instance which created SomeDialog dlg in its slotDoSomething(). When the ParentWidget is deleted, dlg is deleted in turn, as it's the child of ParentWidget. Thus when exec() returns, we are in an awkward situation: The execution of the parent widget's slotDoSomething is continued, although "this" is already deleted, and so is dlg, which was created on the stack (!). Access to a member variable of the parent widget will crash the application, and if not, it will crash when leaving slotDoSomething, as it tries to destroy the dlg object a second time when unwinding the stack. Booom.

This happens more often than you think, in particular it's a problem in KParts which can be deleted at any time by their host application, like Konqueror does in the example above. If your code is part of a library, you can even bet that it will be used in such a scenario. So better fix it.

But how? Well, you can avoid the crash by not passing "this" as parent. That's a bad idea though, as not passing a parent to a modal dialog breaks window stacking and has all kinds of bad effects. That's particularly bad on OS X, where the dialog might pop up behind (!) the blocked (!) parent widget, unreachable for the user. Which renders the application useless and isn't much better than a crash, from a user's perspective. Effects on other window systems might be less worse, but annoying nevertheless (like modal dialogs creating other modal dialogs without passing themselves as parent).

Ok, deleting an object created on the stack might be a bad idea. Let's create it on the heap then:


void ParentWidget::slotDoSomething() {
SomeDialog* dlg = new SomeDialog( this ); //the dlg-on-heap-variant
if ( dlg->exec() == QDialog::Accepted ) {
const QString str = dlg->someUserInput();
//do something with with str
}
delete dlg;
}

Now you have to delete the dialog to not leak memory. But if the dialog was already deleted before the return of exec()? Then you delete a now invalid pointer to a already deleted object. Booom, again.

So, is there a solution? Yes, use QPointer:


void ParentWidget::slotDoSomething() {
QPointer dlg = new SomeDialog( this ); //the dlg-on-heap-variant
if ( dlg->exec() == QDialog::Accepted ) {
const QString str = dlg->someUserInput();
//do something with with str
}
delete dlg;
}

If the dialog is deleted, the QPointer is reset to 0, thus dlg is either still valid or was reset to 0 when exec() returns, and "delete dlg" is safe as deleting null pointers does no harm. If was deleted during the exec(), exec() will return QDialog::Rejected. If there is more to be done in the slot after the exec(), especially in the Rejected-Case, you should check for dlg != 0, to find out if dlg and "this" were deleted, to avoid further method calls or member access that would dereference "this".

Conclusion: Use a QPointer when showing modal dialogs via exec(). It avoids crashes caused by deleting the dialog's parent widget, which can be triggered by an event (background job, D-BUS call, ...) before dlg->exec() returns. In general, all event processing, using exec() or otherwise, is unsafe unless you take special measures like done here. This situation occurs more often than one might think at first glance, especially if your parent widget might be created inside a KPart, which can be created and destroyed any time by their host application (e.g., Konqueror).

Update: As Eean points out in the comments, exec() is only one of its kind of methods returning to the event loop behind the scenes. Other synchronous convenience APIs around asynchronous operations like e.g. KJob::exec() or QProcess::waitForStarted/Finished() have the same problem. QDialog::exec() is just the one used all the time. Ideally, such methods would be avoided alltogether. He's right, and the question how to design an application to avoid such issues is worth another article.

And, btw, if you absolutely need to call a method which will process events, there is another cheap trick to check if you're still alive. You can use a QPointer to yourself (works only from within QObjects, obviously) like this:


QPointer that( this );
...call the method which processes events
if ( !that )
return;
...continue

It has the advantage that also works with the static KMessageBox::foo() functions.

Comments

really great blog entry; as we move more and more to multi-process/threading models these things become ever more critical.

btw, "qdbus org.kde.kontact /MainApplication quit" can be more succinctly said as "kquitapp kontact"


By Aaron J. Seigo at Fri, 03/27/2009 - 01:58

Neither of those quits work for non-unique apps like Konsole. You need the pid which you can get from the output of 'qdbus'. For example org.kde.konsole-6520


By Kurt V. Hindenburg at Fri, 03/27/2009 - 04:23

I think the real solution here is to use signal/slots as God and Country intended.

In Amarok we used to have a 'BlockingQuery' mechanism that masked the naturally asynchronous nature of making a SQL call to the function that called it. Which is obviously handy, but this caused all sorts of problems. The scenario you outline above explains how such a crash works. Its easy to imagine more complicated. So in my opinion processEvents(), exec() and their ilk are just bad.

Also modal dialogs are really annoying. They block the systray from working among other things.


By eean at Fri, 03/27/2009 - 02:29

> I think the real solution here is to use signal/slots as God and Country intended.
>[...]
>The scenario you outline above explains how such a crash works. Its easy to imagine more complicated. So > in my opinion processEvents(), exec() and their ilk are just bad.

True. QDialog::exec() is the most prominent one though and used so often (much more often than naked processEvents() calls, where it's more explicit that bad things might happen (TM)). So I concentrated on that to demonstrate the problem. Realistically, I don't see this pattern go away soon. Want to replace every KMessageBox::foo() in KDE's codebase by signal/slot cascades? Have fun. I doubt that it makes the code less error-prone overall though.

Also one could avoid modal dialogs altogether of course (and especially modal dialogs opening other modal dialogs), but that too requires major changes.


By Frank Osterfeld at Fri, 03/27/2009 - 06:39

> Realistically, I don't see this pattern go away soon.
> Want to replace every KMessageBox::foo() in KDE's codebase
> by signal/slot cascades? Have fun. I doubt that it makes
> the code less error-prone overall though.

Still, it might be interesting to create a check in the EBN code checker ("krazy" I think), lest the issue gets forgoten.


By moltonel at Mon, 03/30/2009 - 10:06

Well I wasn't suggesting we scrub code of all QDialog.exec. Hmm maybe I was, but that wasn't really what I was thinking. ;)

Mostly I was saying that while your suggested code works in the more limited case of a KMessageBox-style modal dialog, that in general stuff like processEvents and exec are horrible. And people really shouldn't add modal dialogs anymore, since they are so annoying to the user, which I guess is a separate issue.

One issue is that processEvents docs (last I looked) didn't have a big disclaimer saying this function would likely screw up your APIs and crash your apps.


By eean at Fri, 03/27/2009 - 15:26

The QPointer-trick will probably prevent a possible crash, but creating an object on the heap that you have to delete manually when you are finished is going to cause memory leaks (or, in the above example, unnecessary late deletion of the dialog) because eventually somebody will forget the delete dlg; in some code path.

What we want to use is a combination of QPointer and QSharedPointer for QObjects, i.e. a class that combines the advantages of both classes:
- It resets the internal pointer to 0 if the QObject it points to is deleted. (like QPointer)
- It deletes the internal pointer if it is destroyed. (like QSharedPointer)

This would simplify the example to

void ParentWidget::slotDoSomething() {
QSmartQObjectPointer dlg( new SomeDialog( this ) );
if ( dlg->exec() == QDialog::Accepted ) {
const QString str = dlg->someUserInput();
//do something with with str
}
}


By Ingo Klöcker at Sat, 03/28/2009 - 15:41

Right, a QPointer deleting the object on destruction would the perfect solution. My name for it would be K|QAutoPointer (mix of std::auto_ptr and QPointer). Additionally to the advantages you mention, it would also make exception-safety possible.


By Frank Osterfeld at Sat, 03/28/2009 - 19:50

I've been testing Konsole and can't get it to crash on any of the 5 crashy items.
What is the thought about using deleteLater()? For example on a KDialog*


By Kurt V. Hindenburg at Tue, 06/30/2009 - 14:52

I guess it's the same for QDrag::exec().

Is there a way to call a async exec and connect a signal to a slot? In case the receiver of the signal gets deleted the slot isn't called, so this would seem to be the right way to implement any operation that returns to the event loop. However, I do not see such a signal in QDrag.


By Mathias Panzenböck at Mon, 01/25/2010 - 18:31