[kmcomposerwin] Save configuration on composer window close
ClosedPublic

Authored by anthonyfieroni on Aug 27 2017, 5:13 AM.

Details

Reviewers
dvratil
dfaure
Summary

Now does not crash on every logout with appended backtrace.

Test Plan

Fix crash on logout

Diff Detail

Repository
R206 KMail
Lint
Lint Skipped
Unit
Unit Tests Skipped
anthonyfieroni created this revision.Aug 27 2017, 5:13 AM
Restricted Application added a project: KDE PIM. · View Herald TranscriptAug 27 2017, 5:13 AM
Restricted Application added a subscriber: KDE PIM. · View Herald Transcript
mlaurent requested changes to this revision.Aug 27 2017, 5:32 AM

Hi,
How do you see in backtrace that it's a problem here ?
I can't have a crash here.
This code will never touch during several years.
Which code delete the kmailKernel ?

Regards.

This revision now requires changes to proceed.Aug 27 2017, 5:32 AM

Destructor of KMMainWin is called twice, first i think is because cleanup is called on dangling pointer. Now i see the problem is in closeAllKMailWindows.

anthonyfieroni edited edge metadata.
anthonyfieroni edited the summary of this revision. (Show Details)

How do you see it ?
valgrind ?

anthonyfieroni retitled this revision from [kmkernel] Do not make kmkernel local since it interacts on DBus to [kmkernel] Make world to know that kernel is about to shutting down.

you still forgot to answer at:
"How do you see it ?
valgrind ?"

src/editor/kmcomposerwin.cpp
513

nope as we want to avoid to lose message that we compose if we close by accident kmail.

you still forgot to answer at:
"How do you see it ?
valgrind ?"

gdb bt

anthonyfieroni added inline comments.Aug 28 2017, 7:08 AM
src/editor/kmcomposerwin.cpp
513

So in compose destructor kernel is used, so we cannot live without it.

anthonyfieroni added inline comments.Aug 28 2017, 7:11 AM
src/editor/kmcomposerwin.cpp
513

I'll investigate but compose message should be save and restore if kontact / kamil started again.

As investigate here, kmkernel is not destroyed until last composer is deleted.
And when it's embedded in kontact composer is destroyed when we close kontact.
So I don't know how you can reproduce a case where composer is still open with a kmkernel deleted.

"gdb bt" when I read you backtract without all symbol I don't know how you can be sure that you patch is correct.

anthonyfieroni added a comment.EditedAug 28 2017, 8:06 AM
  1. if closeAllKMailWindows hasn't modified - yes composer isn't alive but on logout it has crash with appended backtrace
  2. if i remove custom deleter from closeAllKMailWindows it crash in kernel because it isn't alive when composer dies
  3. To fix 2 issues above i made this patch

I think it's clear now?

For me it's works correct now. Be sure you are start standalone KMail (compiled with debug symbols) and logout from KLauncher.

krop added a subscriber: krop.Sep 9 2017, 9:11 AM

For me it's works correct now. Be sure you are start standalone KMail (compiled with debug symbols) and logout from KLauncher.

What Laurent wants *is* the backtrace with debug symbol.

Attaching to process 14125
[New LWP 14148]
[New LWP 14170]
[New LWP 14174]
[New LWP 14184]
[New LWP 14254]
[New LWP 14273]
[New LWP 14276]
[New LWP 14277]
[New LWP 14278]
[New LWP 14279]
[New LWP 14280]
[New LWP 14281]
[New LWP 14282]
[New LWP 14283]
[New LWP 14284]
[New LWP 14285]
[New LWP 14286]
[New LWP 14287]
[New LWP 14288]
[New LWP 14289]
[New LWP 14290]
[New LWP 14291]
[New LWP 14292]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/libthread_db.so.1".
0x00006ecf64338a4d in poll () from /lib/libc.so.6
Continuing.
[Thread 0x6ecf31059700 (LWP 14184) exited]
[Thread 0x6ecf30858700 (LWP 14254) exited]

Thread 1 "kmail" received signal SIGSEGV, Segmentation fault.
0x00006ecf65aa6f7f in QWidget::~QWidget() () from /usr/lib/libQt5Widgets.so.5
#0  0x00006ecf65aa6f7f in QWidget::~QWidget() () at /usr/lib/libQt5Widgets.so.5
#1  0x00006ecf65aa7369 in QWidget::~QWidget() () at /usr/lib/libQt5Widgets.so.5
#2  0x00006ecf65148d98 in QObject::event(QEvent*) () at /usr/lib/libQt5Core.so.5
#3  0x00006ecf65aab9e3 in QWidget::event(QEvent*) () at /usr/lib/libQt5Widgets.so.5
#4  0x00006ecf65a6cb8c in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /usr/lib/libQt5Widgets.so.5
#5  0x00006ecf65a73e81 in QApplication::notify(QObject*, QEvent*) () at /usr/lib/libQt5Widgets.so.5
#6  0x00006ecf6511d108 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /usr/lib/libQt5Core.so.5
#7  0x00006ecf6511fd4d in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () at /usr/lib/libQt5Core.so.5
#8  0x00006ecf56373a84 in  () at /usr/lib/libQt5Quick.so.5
#9  0x00006ecf56373ce0 in QQuickRenderControl::~QQuickRenderControl() () at /usr/lib/libQt5Quick.so.5
#10 0x00006ecf4fa3d7e3 in  () at /usr/lib/libQt5QuickWidgets.so.5
#11 0x00006ecf4fa3c293 in  () at /usr/lib/libQt5QuickWidgets.so.5
#12 0x00006ecf4fa3c319 in  () at /usr/lib/libQt5QuickWidgets.so.5
#13 0x00006ecf6514fc27 in QObject::~QObject() () at /usr/lib/libQt5Core.so.5
#14 0x00006ecf65aa71c5 in QWidget::~QWidget() () at /usr/lib/libQt5Widgets.so.5
#15 0x00006ecf5c9de08a in  () at /usr/lib/libQt5WebEngineWidgets.so.5
#16 0x00006ecf56c9a74e in  () at /usr/lib/libQt5WebEngineCore.so.5
#17 0x00006ecf56c9a929 in  () at /usr/lib/libQt5WebEngineCore.so.5
#18 0x00006ecf570dd51f in  () at /usr/lib/libQt5WebEngineCore.so.5
#19 0x00006ecf570d206f in  () at /usr/lib/libQt5WebEngineCore.so.5
#20 0x00006ecf56f2d97e in  () at /usr/lib/libQt5WebEngineCore.so.5
#21 0x00006ecf56f5ad37 in  () at /usr/lib/libQt5WebEngineCore.so.5
#22 0x00006ecf56f5b2c9 in  () at /usr/lib/libQt5WebEngineCore.so.5
#23 0x00006ecf56f6137d in  () at /usr/lib/libQt5WebEngineCore.so.5
#24 0x00006ecf56f30128 in  () at /usr/lib/libQt5WebEngineCore.so.5
#25 0x00006ecf56f2b9fa in  () at /usr/lib/libQt5WebEngineCore.so.5
#26 0x00006ecf571a0336 in  () at /usr/lib/libQt5WebEngineCore.so.5
#27 0x00006ecf571a05f9 in  () at /usr/lib/libQt5WebEngineCore.so.5
#28 0x00006ecf56cb487e in  () at /usr/lib/libQt5WebEngineCore.so.5
#29 0x00006ecf56cb4b2a in QtWebEngineCore::WebContentsAdapter::~WebContentsAdapter() () at /usr/lib/libQt5WebEngineCore.so.5
#30 0x00006ecf5c9d0d06 in  () at /usr/lib/libQt5WebEngineWidgets.so.5
#31 0x00006ecf5c9d0d19 in  () at /usr/lib/libQt5WebEngineWidgets.so.5
#32 0x00006ecf5c9cbbc3 in QWebEnginePage::~QWebEnginePage() () at /usr/lib/libQt5WebEngineWidgets.so.5
#33 0x00006ecf620e45a9 in MessageViewer::MailWebEnginePage::~MailWebEnginePage() () at /usr/lib/libKF5MessageViewer.so.5
#34 0x00006ecf65145d91 in QObjectPrivate::deleteChildren() () at /usr/lib/libQt5Core.so.5
#35 0x00006ecf65aa713b in QWidget::~QWidget() () at /usr/lib/libQt5Widgets.so.5
#36 0x00006ecf620e2569 in MessageViewer::MailWebEngineView::~MailWebEngineView() () at /usr/lib/libKF5MessageViewer.so.5
#37 0x00006ecf6208d5c8 in  () at /usr/lib/libKF5MessageViewer.so.5
#38 0x00006ecf6208d7f9 in  () at /usr/lib/libKF5MessageViewer.so.5
#39 0x00006ecf65145d91 in QObjectPrivate::deleteChildren() () at /usr/lib/libQt5Core.so.5
#40 0x00006ecf65aa713b in QWidget::~QWidget() () at /usr/lib/libQt5Widgets.so.5
#41 0x00006ecf6207f0c9 in MessageViewer::Viewer::~Viewer() () at /usr/lib/libKF5MessageViewer.so.5
#42 0x00006ecf65145d91 in QObjectPrivate::deleteChildren() () at /usr/lib/libQt5Core.so.5
#43 0x00006ecf65aa713b in QWidget::~QWidget() () at /usr/lib/libQt5Widgets.so.5
#44 0x00006ecf695f8c54 in KMReaderWin::~KMReaderWin() (this=0x461c940, __in_chrg=<optimized out>) at /home/toni/kmail/src/kmreaderwin.cpp:235
#45 0x00006ecf695f8c76 in KMReaderWin::~KMReaderWin() (this=0x461c940, __in_chrg=<optimized out>) at /home/toni/kmail/src/kmreaderwin.cpp:237
#46 0x00006ecf65145d91 in QObjectPrivate::deleteChildren() () at /usr/lib/libQt5Core.so.5
#47 0x00006ecf65aa713b in QWidget::~QWidget() () at /usr/lib/libQt5Widgets.so.5
#48 0x00006ecf65bd7c19 in QSplitter::~QSplitter() () at /usr/lib/libQt5Widgets.so.5
#49 0x00006ecf65145d91 in QObjectPrivate::deleteChildren() () at /usr/lib/libQt5Core.so.5
#50 0x00006ecf65aa713b in QWidget::~QWidget() () at /usr/lib/libQt5Widgets.so.5
#51 0x00006ecf65bd7c19 in QSplitter::~QSplitter() () at /usr/lib/libQt5Widgets.so.5
#52 0x00006ecf69640c9d in KMMainWidget::deleteWidgets() (this=0x448d460) at /home/toni/kmail/src/kmmainwidget.cpp:986
#53 0x00006ecf6963e85d in KMMainWidget::destruct() (this=0x448d460) at /home/toni/kmail/src/kmmainwidget.cpp:403
#54 0x00006ecf6963e646 in KMMainWidget::~KMMainWidget() (this=0x448d460, __in_chrg=<optimized out>) at /home/toni/kmail/src/kmmainwidget.cpp:387
#55 0x00006ecf6963e760 in KMMainWidget::~KMMainWidget() (this=0x448d460, __in_chrg=<optimized out>) at /home/toni/kmail/src/kmmainwidget.cpp:388
#56 0x00006ecf65145d91 in QObjectPrivate::deleteChildren() () at /usr/lib/libQt5Core.so.5
#57 0x00006ecf65aa713b in QWidget::~QWidget() () at /usr/lib/libQt5Widgets.so.5
#58 0x00006ecf6867faa9 in KMainWindow::~KMainWindow() () at /usr/lib/libKF5XmlGui.so.5
#59 0x00006ecf695f38cc in KMMainWin::~KMMainWin() (this=0x447e9a0, __in_chrg=<optimized out>, __vtt_parm=<optimized out>) at /home/toni/kmail/src/kmmainwin.cpp:95
#60 0x00006ecf695f3916 in KMMainWin::~KMMainWin() (this=0x447e9a0, __in_chrg=<optimized out>, __vtt_parm=<optimized out>) at /home/toni/kmail/src/kmmainwin.cpp:100
#61 0x00006ecf6961dca7 in qDeleteAll<QList<KMainWindow*>::const_iterator>(QList<KMainWindow*>::const_iterator, QList<KMainWindow*>::const_iterator) (begin=..., end=...) at /usr/include/qt5/QtCore/qalgorithms.h:320
#62 0x00006ecf6961b709 in qDeleteAll<QList<KMainWindow*> >(QList<KMainWindow*> const&) (c=...) at /usr/include/qt5/QtCore/qalgorithms.h:328
#63 0x00006ecf69612fb6 in KMKernel::closeAllKMailWindows() (this=0x77ce3f852c60) at /home/toni/kmail/src/kmkernel.cpp:1102
#64 0x00006ecf696131cc in KMKernel::cleanup() (this=0x77ce3f852c60) at /home/toni/kmail/src/kmkernel.cpp:1117
#65 0x00000000004042f1 in main(int, char**) (argc=1, argv=0x77ce3f852f18) at /home/toni/kmail/src/main.cpp:159

What is not clear now?

dvratil added inline comments.Sep 29 2017, 1:10 PM
src/editor/kmcomposerwin.cpp
515

I'm not sure that calling delete this from the object's own slot is safe, this could crash inside QObject once your return from the slot....

anthonyfieroni retitled this revision from [kmkernel] Make world to know that kernel is about to shutting down to [kmkernel] Save configuration on composer window close.
anthonyfieroni edited the summary of this revision. (Show Details)
anthonyfieroni added inline comments.Oct 2 2017, 8:00 PM
src/editor/kmcomposerwin.cpp
515

I'm using the patch and i don't have such a crash but you have a reason.

anthonyfieroni retitled this revision from [kmkernel] Save configuration on composer window close to [kmcomposerwin] Save configuration on composer window close.Oct 2 2017, 8:01 PM

Tested from Kontact:

  1. Close compose window - works
  2. Close Kontact with open compose window - works
  3. Do not access kernel in AttachmentView' destructor

Can we push for 17.08.2?

mlaurent added inline comments.Oct 6 2017, 9:30 PM
src/editor/attachment/attachmentview.cpp
81

? a pointer for it.
Create a local variable not a pointer.

"Can we push for 17.08.2?" master when patch will be correct.

anthonyfieroni marked an inline comment as done.

ping? Without patch systemd still generate coredump file in /var/lib/systemd/coredumps

Let's push it to master if someone has a problem with it?

mlaurent requested changes to this revision.Nov 5 2017, 7:14 AM
mlaurent added inline comments.
src/editor/attachment/attachmentview.cpp
58

I don't understand why you changed it for your bug ?
Is it fix bug here ?

> this part of patch can't be commit.

This revision now requires changes to proceed.Nov 5 2017, 7:14 AM

All parts of this patch are needed. AttachmentView has parent ComposerWin who dies after kernel so it cannot use it in AttachmentView destructor. I don't see any reason to decline this patch even current implementation crashing every time at logout with corudumps log.

anthonyfieroni added inline comments.
src/editor/attachment/attachmentview.cpp
58

If you don't want this part, you should provide a suggestion what you want.

Rebase to master

dfaure accepted this revision.Nov 13 2017, 9:05 AM

Can you push this to Applications/17.12? Laurent is on vacations, and I'd like to have the fix in that release, with time to test it before the actual release.

Can you close the RR now that the commit is in?

(arc land would have taken care of it)

This revision is now accepted and ready to land.Nov 26 2017, 7:09 PM
bcooksley closed this revision.Nov 26 2017, 7:09 PM

If there is a pending objection by a reviewer, then a review will never be closed by a commit.
Should the reviewer not be available to change their status, please remove them from a review (as I just did above).

Note that even if the precise referencing syntax is not used (as in that commit) in some instances Phabricator can still automatically detect that the commit has been pushed (if the commit hashes are still the same for instance)