<table><tr><td style="">dvratil added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D7285" rel="noreferrer">View Revision</a></tr></table><br /><div><div><div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">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.</pre></div>
<p>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?</p>
<p>Start the sieveeditor, then click Import Sieve Settings. Now run</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">qdbus org.kde.sieveeditor /sieveeditor/MainWindow_1 org.qtproject.Qt.QWidget.close</pre></div>
<p>and observe a double-free corruption crash:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">*** Error in `/data/install/kde/bin/sieveeditor': double free or corruption (out): 0x00007fffffffc810 ***
...</pre></div>
<p>GDB:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">#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
...</pre></div>
<p>Valgrind:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">==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)</pre></div>
<p>Now apply the attached patch and the crash is gone :-)</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>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.</p></blockquote>
<p>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 <tt style="background: #ebebeb; font-size: 13px;">this</tt> wasn't passed as a parent to <tt style="background: #ebebeb; font-size: 13px;">ImportImapSettingsWizard</tt> constructor, then creating <tt style="background: #ebebeb; font-size: 13px;">ImportImapSettingsWizard</tt> 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).</p>
<p>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.</p>
<p>Also, this is actually documented in Qt docs to some extent, although not as explicit as what we are discussing here: <a href="http://doc.qt.io/qt-5/objecttrees.html" class="remarkup-link" target="_blank" rel="noreferrer">http://doc.qt.io/qt-5/objecttrees.html</a></p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>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.</p></blockquote>
<p>(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)</p></div></div><br /><div><strong>REPOSITORY</strong><div><div>R213 PIM Sieve Editor</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D7285" rel="noreferrer">https://phabricator.kde.org/D7285</a></div></div><br /><div><strong>To: </strong>winterz, mlaurent<br /><strong>Cc: </strong>dvratil, dkurz, KDE PIM, dvasin, winterz, vkrause, mlaurent, knauss<br /></div>