- 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
Details
- Reviewers
apol - Group Reviewers
KDevelop - Commits
- R33:7cda7c95d5bf: Fix crashy dialogs (found by krazy and by hand)
R32:7cda7c95d5bf: Fix crashy dialogs (found by krazy and by hand)
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
vcs/vcspluginhelper.cpp | ||
---|---|---|
464 | mh, I guess it should be deleted, right? |
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 | ||
---|---|---|
335 | Check should be done on every exec afterwards dialog is used, cause potentially it can be deleted |
- 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
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.
plugins/appwizard/projectselectionpage.cpp | ||
---|---|---|
335 | Do you mean like this? DownloadDialog does not delete itself, so I'm wondering what could go wrong, since changedEntries will just be empty. |
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.
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();
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). |
util/dialogwrapper.h | ||
---|---|---|
55 ↗ | (On Diff #15934) | I would rename it to ScopedDialog. |
Thanks for the feedback. I guess I can commit then?
A follow-up could be to replace manual QPointer dialogs by the wrapper.
use ScopedDialog for an existing dialog in TemplateClassAssistant which was never deleted
I was thinking about existing QPointer<QDialog> instances, but actually only found two, one missing a delete, which is now fixed.
plugins/subversion/svnjobbase.cpp | ||
---|---|---|
75 | This one seems fishy, should it not be released in any case? |
plugins/subversion/svnjobbase.cpp | ||
---|---|---|
75 | Indeed, I will fix this. |