this is to prevent a crash as described in https://blogs.kde.org/node/3919

yep but here we didn"t use data from wizard so I don't see where it can crash

In D7285#135693, @mlaurent wrote:

yep but here we didn"t use data from wizard so I don't see where it can crash

Without QPointer, the destructor of the dialog is always called at the end of slotImportImapSettings(). Problem is, the dialog might not exist any more. This is also discussed in D7291.

We will not add a QPointer for each element which create on stack and convert on pointer.
it's just a wizard when we don't any data from it.
Otherwise give me a test case which provides that it will crash.
Regards.

Open the editor window, then open the import wizard. Now close the editor window (through any means) and then close/accept the import wizard and it will crash. When you close the SieveEditorMainWindow it is deleted, which in turn deletes the ImportImapSettingWizard because we passed the SieveEditorMainWindow as its parent. This won't, however, quit the wizard. When you close the wizard and the code returns from slotImportImapSettings(), the w variable will go out-of-scope, causing ImportImapSettingWizard's destructor to be called again -> booom :-)

IOW anything that uses nested QEventLoops shouldn't be created on the stack and should be guarded by QPointer.

I spoke about a real test case not a hypothetical testcase.
It's a wizard modal to apps => we can"t close it.
"qdbus org.kde.sieveeditor /MainApplication org.qtproject.Qt.QCoreApplication.quit" close apps + wizard without crash.

So if it's forbidden to use a element which has a QEventLoops on stack we need to have a policy for it as when I look at kdepim source code we use it in several place.
I never see in Qt doc that it's forbidden.

I spoke about a real test case not a hypothetical testcase.
It's a wizard modal to apps => we can"t close it.
"qdbus org.kde.sieveeditor /MainApplication org.qtproject.Qt.QCoreApplication.quit" close apps + wizard without crash.

Quitting the QApplication does not produce a crash in this particular case because of how things are ordered. But how about just destroying the parent window?

Start the sieveeditor, then click Import Sieve Settings. Now run

qdbus org.kde.sieveeditor /sieveeditor/MainWindow_1 org.qtproject.Qt.QWidget.close

and observe a double-free corruption crash:

*** Error in `/data/install/kde/bin/sieveeditor': double free or corruption (out): 0x00007fffffffc810 ***
...

GDB:

#0  0x00007fe26c4fc66b in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51                                                                                                                                     
#1  0x00007fe26c4fe470 in __GI_abort () at abort.c:89    
#2  0x00007fe26c5428b1 in __libc_message (do_abort=do_abort@entry=2, fmt=fmt@entry=0x7fe26c65f100 "*** Error in `%s': %s: 0x%s ***\n") at ../sysdeps/posix/libc_fatal.c:175                                                         
#3  0x00007fe26c54d759 in malloc_printerr (ar_ptr=<optimized out>, ptr=<optimized out>, str=0x7fe26c65f4f8 "double free or corruption (out)", action=<optimized out>) at malloc.c:5077                                              
#4  0x00007fe26c54d759 in _int_free (av=<optimized out>, p=<optimized out>, have_lock=0) at malloc.c:3873         
#5  0x00007fe26c5530be in __GI___libc_free (mem=<optimized out>) at malloc.c:2947                                 
#6  0x00007fe26fba5c50 in ImportImapSettingWizard::~ImportImapSettingWizard() (this=0x7fffffffc5b0, __in_chrg=<optimized out>) at /data/KDE/src/kde/pim/pim-sieve-editor/src/importwizard/importimapsettingwizard.cpp:69            
#7  0x00007fe26d3fc94b in QObjectPrivate::deleteChildren() () at /lib64/libQt5Core.so.5                                 
#8  0x00007fe26e1c72ac in QWidget::~QWidget() () at /lib64/libQt5Widgets.so.5                                           
#9  0x00007fe26f8a81cb in KMainWindow::~KMainWindow() (this=0x9f5de0, __in_chrg=<optimized out>) at /data/KDE/src/frameworks/kxmlgui/src/kmainwindow.cpp:395                                                                        
#10 0x00007fe26f8ebb54 in KXmlGuiWindow::~KXmlGuiWindow() (this=0x9f5de0, __vtt_parm=0x7fe26fdd4200 <VTT for SieveEditorMainWindow+8>, __in_chrg=<optimized out>) at /data/KDE/src/frameworks/kxmlgui/src/kxmlguiwindow.cpp:111     
#11 0x00007fe26fb7d823 in SieveEditorMainWindow::~SieveEditorMainWindow() (this=0x9f5de0, __in_chrg=<optimized out>, __vtt_parm=<optimized out>) at /data/KDE/src/kde/pim/pim-sieve-editor/src/sieveeditormainwindow.cpp:74         
#12 0x00007fe26fb7d874 in SieveEditorMainWindow::~SieveEditorMainWindow() (this=0x9f5de0, __in_chrg=<optimized out>, __vtt_parm=<optimized out>) at /data/KDE/src/kde/pim/pim-sieve-editor/src/sieveeditormainwindow.cpp:82
...

Valgrind:

==32250== Invalid free() / delete / delete[] / realloc()
==32250==    at 0x4C301E8: operator delete(void*) (vg_replace_malloc.c:576)
==32250==    by 0x5097C4F: ImportImapSettingWizard::~ImportImapSettingWizard() (importimapsettingwizard.cpp:69)
==32250==    by 0x7A9694A: QObjectPrivate::deleteChildren() (in /usr/lib64/libQt5Core.so.5.9.1)
==32250==    by 0x694D2AB: QWidget::~QWidget() (in /usr/lib64/libQt5Widgets.so.5.9.1)
==32250==    by 0x53521CA: KMainWindow::~KMainWindow() (kmainwindow.cpp:395)
==32250==    by 0x5395B53: KXmlGuiWindow::~KXmlGuiWindow() (kxmlguiwindow.cpp:111)
==32250==    by 0x506F822: SieveEditorMainWindow::~SieveEditorMainWindow() (sieveeditormainwindow.cpp:74)
==32250==    by 0x506F873: SieveEditorMainWindow::~SieveEditorMainWindow() (sieveeditormainwindow.cpp:82)
==32250==    by 0x7A99587: QObject::event(QEvent*) (in /usr/lib64/libQt5Core.so.5.9.1)
==32250==    by 0x69519E2: QWidget::event(QEvent*) (in /usr/lib64/libQt5Widgets.so.5.9.1)
...
==32250==  Address 0x1ffeffe570 is on thread 1's stack
==32250==  in frame #24, created by SieveEditorMainWindow::slotImportImapSettings() (sieveeditormainwindow.cpp:265)

Now apply the attached patch and the crash is gone :-)

So if it's forbidden to use a element which has a QEventLoops on stack we need to have a policy for it as when I look at kdepim source code we use it in several place. I never see in Qt doc that it's forbidden.

You are right, I over-generalized my statement. There are certainly valid cases where you can create an object with nested event loop on the stack: for example, it's OK to create a QApplication on the stack in main(). It's also OK to do so for any QObject-derived class with a nested event loop that does not have an explicit parent - in the code in this review, if this wasn't passed as a parent to ImportImapSettingsWizard constructor, then creating ImportImapSettingsWizard on the stack would be OK. It's also OK for a non-widget QObject-derived class with nested event loop where you can guarantee that the parent object will not be deleted before the child (we do this on some places in Akonadi).

Therefore there's no real policy, as it's hard to precisely list all the circumstances when you should use QPointer and when you are fine with just a stack allocation. The blog post linked by Alan describes a thing to watch out for and be mindful of when writing code, especially with nested modal dialogs.

Also, this is actually documented in Qt docs to some extent, although not as explicit as what we are discussing here: http://doc.qt.io/qt-5/objecttrees.html

The parent's destructor is called first because it was created last. It then calls the destructor of its child, quit, which is incorrect because quit is a local variable. When quit subsequently goes out of scope, its destructor is called again, this time correctly, but the damage has already been done.

(this does not apply exactly to the problem at hand, but describes the same kind of a problem: double deletion due to the object being deleted first by its parent, and then when the variable on the stack goes out-of-scope)

PS: I'm not saying we should now go and rigorously change this everywhere - it's a too rare a case. I, however, see no reason to not accept a valid crash fix, even if for just a potential or unlikely crash. Users are capable of triggering the weirdest kinds of impossible crashes, after all :-) Even less of a reason if the fix is as trivial as this one...