DevHeads.net

Review Request 118858: Fix crashes in KUrlNavigator that are caused by accesses to objects which have been deleted in nested event loops

Review request for kdelibs.

Bugs: 293863
<a href="http://bugs.kde.org/show_bug.cgi?id=293863" title="http://bugs.kde.org/show_bug.cgi?id=293863">http://bugs.kde.org/show_bug.cgi?id=293863</a>

Repository: kdelibs

Description
KUrlNavigator opens menus with exec() in a few places, and accesses member variables or pointers to children after that. This can cause crashes if the object has been deleted inside the nested event loops.

This can be fixed by using QPointers to detect if an object was deleted already, and return immediately in that case.

Diffs
kfile/kurlnavigator.cpp f5dfc81
kfile/kurlnavigatorbutton.cpp 6cb40b1

Diff: <a href="https://git.reviewboard.kde.org/r/118858/diff/" title="https://git.reviewboard.kde.org/r/118858/diff/">https://git.reviewboard.kde.org/r/118858/diff/</a>

Testing
Cannot reproduce the crashes any more. The menus in KUrlNavigator still work fine for me.

Thanks,

Frank Reininghaus

Comments

Re: Review Request 118858: Fix crashes in KUrlNavigator that are

By Frank Reininghaus at 06/22/2014 - 03:58

(Updated June 22, 2014, 7:58 a.m.)

Review request for kdelibs.

Changes
Thanks for your comments, Christoph and Dominik! It's interesting that this kind of crash was already discussed in 2009. I'd like to emphasize though that the problem is not limited to the "quit via D-Bus" use case. None of the reporters of the KUrlNavigatorCrash mention D-Bus in their reports. Lots of other things can happen inside nested event loops: timers can fire, slots can be called via queued connections, etc. Some of these things might delete objects unexpectedly, and cause crashes which are very hard to reproduce.

Trying the "D-Bus quit" method for every dialog and context menu in every application might actually reveal more real bugs that cause rare random crashes.

I did find another nested event loop-related bug in KUrlNavigator: in KUrlNavigator::Private::openContextMenu(): the menu, which is a child of the KUrlNavigator, is created on the stack. This causes a crash if the navigator is closed inside the loop because the QObject destructor tries to delete the child object on the stack.

The fix is quite straightforward: move the menu to the heap, and use the same "delete the menu if the QPointer is not null" idea like in KUrlNavigator::Private::openPathSelectorMenu().

If noone sees a problem with the second version of the patch, I'll commit it in the next few days and forward-port to KF5.

Bugs: 293863
<a href="http://bugs.kde.org/show_bug.cgi?id=293863" title="http://bugs.kde.org/show_bug.cgi?id=293863">http://bugs.kde.org/show_bug.cgi?id=293863</a>

Repository: kdelibs

Description
KUrlNavigator opens menus with exec() in a few places, and accesses member variables or pointers to children after that. This can cause crashes if the object has been deleted inside the nested event loops.

This can be fixed by using QPointers to detect if an object was deleted already, and return immediately in that case.

Diffs (updated)
kfile/kurlnavigator.cpp f5dfc81
kfile/kurlnavigatorbutton.cpp 6cb40b1

Diff: <a href="https://git.reviewboard.kde.org/r/118858/diff/" title="https://git.reviewboard.kde.org/r/118858/diff/">https://git.reviewboard.kde.org/r/118858/diff/</a>

Testing
Cannot reproduce the crashes any more. The menus in KUrlNavigator still work fine for me.

Thanks,

Frank Reininghaus

Re: Review Request 118858: Fix crashes in KUrlNavigator that are

By Commit Hook at 06/29/2014 - 15:09

This review has been submitted with commit f1196e8e9a94993e4d748d283d869c4ad205ff02 by Frank Reininghaus to branch KDE/4.13.

- Commit Hook

On June 22, 2014, 7:58 a.m., Frank Reininghaus wrote:

Re: Review Request 118858: Fix crashes in KUrlNavigator that are

By Frank Reininghaus at 06/29/2014 - 15:09

(Updated June 29, 2014, 7:09 p.m.)

Status
This change has been marked as submitted.

Review request for kdelibs.

Bugs: 293863
<a href="http://bugs.kde.org/show_bug.cgi?id=293863" title="http://bugs.kde.org/show_bug.cgi?id=293863">http://bugs.kde.org/show_bug.cgi?id=293863</a>

Repository: kdelibs

Description
KUrlNavigator opens menus with exec() in a few places, and accesses member variables or pointers to children after that. This can cause crashes if the object has been deleted inside the nested event loops.

This can be fixed by using QPointers to detect if an object was deleted already, and return immediately in that case.

Diffs
kfile/kurlnavigator.cpp f5dfc81
kfile/kurlnavigatorbutton.cpp 6cb40b1

Diff: <a href="https://git.reviewboard.kde.org/r/118858/diff/" title="https://git.reviewboard.kde.org/r/118858/diff/">https://git.reviewboard.kde.org/r/118858/diff/</a>

Testing
Cannot reproduce the crashes any more. The menus in KUrlNavigator still work fine for me.

Thanks,

Frank Reininghaus

Re: Review Request 118858: Fix crashes in KUrlNavigator that are

By Dominik Haumann at 06/21/2014 - 06:34

Just for info: Similar problems exist when an application runs an extra event loop through a QDialog::exec() call and the app is quit through a dbus call.
There once was a blog about it by Frank Osterfeld (read also the "Update" note, unfortunately, the markup is a bit broken):
<a href="http://blogs.kde.org/2009/03/26/how-crash-almost-every-qtkde-application-and-how-fix-it-0" title="http://blogs.kde.org/2009/03/26/how-crash-almost-every-qtkde-application-and-how-fix-it-0">http://blogs.kde.org/2009/03/26/how-crash-almost-every-qtkde-application...</a> (and a followup here: <a href="http://kate-editor.org/2012/04/06/crash-through-d-bus-calls/" title="http://kate-editor.org/2012/04/06/crash-through-d-bus-calls/">http://kate-editor.org/2012/04/06/crash-through-d-bus-calls/</a> )

The solution is also to use a QPointer, on the dialog itself (and maybe additionally on the this QObject).

In other words, your fix is probably more than just a short-term workaround ;)

- Dominik Haumann

On June 21, 2014, 8:26 a.m., Frank Reininghaus wrote:

Re: Review Request 118858: Fix crashes in KUrlNavigator that are

By Christoph Feck at 06/21/2014 - 05:46

Ship it!

A good workaround until we understand how to get rid of nested event loops.

- Christoph Feck

On June 21, 2014, 8:26 a.m., Frank Reininghaus wrote: