Fix crashy dialogs (found by krazy and by hand)
ClosedPublic

Authored by croick on Jun 20 2017, 9:07 PM.

Details

Summary
  • use a wrapper class which simplifies the use of QPointer for the creation of local QDialogs
  • some false positives are marked
  • some dialogs not identified by krazy have the main window as parent, they may still cause a crash, when the program is killed
Test Plan

Before:

  • open "About KDevelop Platform" dialog
  • "killall kdevelop" -> kdevelop closes (as intended), but with crash

After:

  • open same dialog
  • "killall kdevelop" -> kdevelop silently quits

Diff Detail

Repository
R33 KDevPlatform
Branch
crashy
Lint
No Linters Available
Unit
No Unit Test Coverage
croick created this revision.Jun 20 2017, 9:07 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptJun 20 2017, 9:07 PM
croick added inline comments.Jun 20 2017, 9:16 PM
vcs/vcspluginhelper.cpp
464

mh, I guess it should be deleted, right?

croick updated this revision to Diff 15760.Jun 22 2017, 9:30 PM
  • undo change for SessionChooserDialog which has no parent
croick updated this revision to Diff 15766.Jun 22 2017, 10:56 PM
  • Potentially delete VcsCommitDialog after use
croick added inline comments.Jun 22 2017, 10:57 PM
vcs/vcspluginhelper.cpp
464

it might delete itself, but not necessarily

You still can make a wrapper around QPointer prevent call delete after exec() check i.e.

template<class T>
class QPointerWrapper
{
QPoniter<T> ptr;
public:
    QPointerWrapper(T *rawPtr) : ptr(rawPtr) {}
    ~QPointerWrapper() { delete ptr; }
   QPointer<T>& operator->() { return ptr; }
};
// usage
QPointerWrapper<VcsCommitDialog> commitDialog = new VcsCommitDialog(patchSource);
commitDialog->exec();
// without delete commitDialog :)
plugins/appwizard/projectselectionpage.cpp
332

Check should be done on every exec afterwards dialog is used, cause potentially it can be deleted

croick updated this revision to Diff 15806.Jun 23 2017, 9:44 PM
  • use pointer for environment preferences widget to fix crash
  • delete dialog of EnvironmentWidget after access to child
  • use a wrapper for the QPointer of VcsCommitDialog, check for return value of DownloadDialog

You still can make a wrapper around QPointer prevent call delete after exec() check i.e.

template<class T>
class QPointerWrapper
{
QPoniter<T> ptr;
public:
    QPointerWrapper(T *rawPtr) : ptr(rawPtr) {}
    ~QPointerWrapper() { delete ptr; }
   QPointer<T>& operator->() { return ptr; }
};
// usage
QPointerWrapper<VcsCommitDialog> commitDialog = new VcsCommitDialog(patchSource);
commitDialog->exec();
// without delete commitDialog :)

Good point! I used that inside the functions now, since it only occurs twice.

For the rest, I hopefully excluded all parentless dialogs from the patch such that these remain two special cases.

croick added inline comments.Jun 23 2017, 9:52 PM
plugins/appwizard/projectselectionpage.cpp
332

Do you mean like this? DownloadDialog does not delete itself, so I'm wondering what could go wrong, since changedEntries will just be empty.

anthonyfieroni added a comment.EditedJun 24 2017, 9:26 AM

Good point! I used that inside the functions now, since it only occurs twice.

For the rest, I hopefully excluded all parentless dialogs from the patch such that these remain two special cases.

I mean it can be useful when you have more that one deletion i.e.

type *dialog = new dialog;
if (dialog->exec()) {
    delete dialog;
    return;
}
.
.
.
delete dialog;

It can be done without 2 lines of deletes if you use wrapper

wrapper *dialog = new dialog;
if (dialog->exec()) {
    return;
}

I mean it can be useful when you have more that one deletion i.e.

type *dialog = new dialog;
if (dialog->exec()) {
    delete dialog;
    return;
}
.
.
.
delete dialog;

It can be done without 2 lines of deletes if you use wrapper

wrapper *dialog = new dialog;
if (dialog->exec()) {
    return;
}

Right, I was confused between caller and callee...
But then it would actually be nice to have such a wrapper as util(ity), since the problem appears often, but mostly only once per file.

croick updated this revision to Diff 15934.Jun 27 2017, 10:23 PM
  • use a global wrapper for managing the dialog instance
croick edited the summary of this revision. (Show Details)Jun 27 2017, 10:27 PM
croick marked 4 inline comments as done.Jun 27 2017, 10:37 PM

You still can make a wrapper around QPointer prevent call delete after exec() check i.e.

template<class T>
class QPointerWrapper
{
QPoniter<T> ptr;
public:
    QPointerWrapper(T *rawPtr) : ptr(rawPtr) {}
    ~QPointerWrapper() { delete ptr; }
   QPointer<T>& operator->() { return ptr; }
};
// usage
QPointerWrapper<VcsCommitDialog> commitDialog = new VcsCommitDialog(patchSource);
commitDialog->exec();
// without delete commitDialog :)

Your proposal is now in a utility class, hopefully in the right place. The dialog is constructed in the wrapper constructor, so the code only adds few overhead (in terms of characters) compared to the stack version:

DialogWrapper<VcsCommitDialog> commitDialog(patchSource);
commitDialog->exec();
anthonyfieroni added inline comments.Jun 28 2017, 7:21 AM
util/dialogwrapper.h
55 ↗(On Diff #15934)

Exporting template class makes no sense to linker. You can export only complete types, i.e.

#ifdef LIB // your library that export symbols
#    define TEMPLATE(x)    template class x
#else // library clients
#    define TEMPLATE(x)    extern template class x
#endif
TEMPLATE(DialogWrapper<Type>); // somewhere in shared header file

This is not needed here, even this may vary between compilers (mostly MS compilers violate it).

apol accepted this revision.Jun 28 2017, 8:57 AM
apol added a subscriber: apol.
apol added inline comments.
util/dialogwrapper.h
55 ↗(On Diff #15934)

I would rename it to ScopedDialog.

This revision is now accepted and ready to land.Jun 28 2017, 8:57 AM
croick updated this revision to Diff 15974.Jun 28 2017, 11:05 PM
  • do not export utility class and rename it to ScopedDialog
croick marked 2 inline comments as done.Jun 28 2017, 11:07 PM

Thanks for the feedback. I guess I can commit then?
A follow-up could be to replace manual QPointer dialogs by the wrapper.

apol added a comment.Jun 29 2017, 12:00 AM

Yes you can push.
And what do you want to replace?

croick updated this revision to Diff 16070.Jun 30 2017, 9:41 PM

use ScopedDialog for an existing dialog in TemplateClassAssistant which was never deleted

In D6308#120345, @apol wrote:

Yes you can push.
And what do you want to replace?

I was thinking about existing QPointer<QDialog> instances, but actually only found two, one missing a delete, which is now fixed.

This revision was automatically updated to reflect the committed changes.
kossebau added inline comments.
plugins/subversion/svnjobbase.cpp
75 ↗(On Diff #15934)

This one seems fishy, should it not be released in any case?

croick added inline comments.Jul 1 2017, 7:41 PM
plugins/subversion/svnjobbase.cpp
75 ↗(On Diff #15934)

Indeed, I will fix this.

croick marked 2 inline comments as done.Jul 1 2017, 8:08 PM

Missing semaphore release fixed in commit 27fc9e3396cc. Thanks for noticing!