Do not crash when setting new line edit on an editable combo box
ClosedPublic

Authored by mwolff on Dec 4 2017, 10:33 PM.

Details

Summary

The 'Open With' dialog opened by KMail triggers the following crash
for me:

#0  0x0000555568ea4220 in ?? ()
#1  0x00007ffff549bd60 in QObject::connect(QObject const*, char const*, QObject const*, char const*, Qt::ConnectionType) () from /usr/lib/libQt5Core.so.5
#2  0x00007ffff71287f4 in KLineEdit::setCompletionObject (this=0x555568ee8c00, comp=0x555568ea1320, handle=<optimized out>)
   at /home/milian/projects/kf5/src/frameworks/kcompletion/src/klineedit.cpp:1514
#3  0x00007ffff7113d35 in KComboBox::setLineEdit (this=0x555568ea3ba0, edit=0x555568ee8c00) at /home/milian/projects/kf5/src/frameworks/kcompletion/src/kcombobox.cpp:325
#4  0x00007ffff784ef63 in KOpenWithDialogPrivate::init (this=0x7fff70240440,
   _text="<qt>Select the program you want to use to open the file<br/>DebuggingProfilingWindows-2017-WK50.pdf</qt>", _value="")
   at /home/milian/projects/kf5/src/frameworks/kio/src/widgets/kopenwithdialog.cpp:601
#5  0x00007ffff784f93c in KOpenWithDialog::KOpenWithDialog (this=0x7fffffffba50, _urls=QList<QUrl> = {...}, _text=..., _value="", parent=<optimized out>)
   at /home/milian/projects/kf5/src/frameworks/kio/src/widgets/kopenwithdialog.cpp:531
#6  0x00007ffff7862b51 in KRun::displayOpenWithDialog (lst=QList<QUrl> = {...}, window=0x55555599a690, tempFiles=tempFiles@entry=true, suggestedFileName="",
   asn=<QArrayData::shared_null+24> "") at /home/milian/projects/kf5/src/frameworks/kio/src/widgets/krun.cpp:267

This happens when an editable KComboBox is initialized (i.e.
KHistoryComboBox in this case). When we then ask for the completionObject,
this will alias the lineEdit's completion object, which gets destroyed by
the call to QComboBox::setLineEdit, leading to the crash above.

Overall, I think the whole aliasing / delegation here is quite
confusing and I'm unsure as to what the desired behavior is here.
Should we transfer ownership to the new line edit, if it has no
compObj of its own?

The unit test is extended to catch this scenario now. It at least
ensures that nothing crashes and also documents the status quo
of the behavior right now.

Diff Detail

Repository
R284 KCompletion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mwolff created this revision.Dec 4 2017, 10:33 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptDec 4 2017, 10:33 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
mwolff requested review of this revision.Dec 4 2017, 10:33 PM
mwolff added a subscriber: dfaure.Dec 4 2017, 10:33 PM

This makes KMail's Open With feature work again, but I wonder - is anyone capable of running valgrind? Or figure out where the deletion comes from? @dfaure?

mwolff edited the summary of this revision. (Show Details)Dec 4 2017, 10:34 PM
anthonyfieroni added a subscriber: anthonyfieroni.
anthonyfieroni added inline comments.
src/kcombobox.cpp
318

KHistoryComboBox is constructed with line edit in, then setLineEdit removes it with completion object in.

mwolff added inline comments.Dec 5 2017, 1:28 PM
src/kcombobox.cpp
318

can you be more specific, such that I can build a unit test out of this? And are you saying that QComboBox::setLineEdit is deleting the line edit?

mwolff added inline comments.Dec 5 2017, 1:35 PM
src/kcombobox.cpp
318

ah I think I got it now, thanks for the hint!

mwolff updated this revision to Diff 23507.Dec 5 2017, 2:14 PM

write unit test and document what's going on in more detail

mwolff updated this revision to Diff 23508.Dec 5 2017, 2:15 PM
mwolff retitled this revision from Do not crash when completion object gets destroyed underneath us to Do not crash when setting new line edit on an editable combo box.
mwolff edited the summary of this revision. (Show Details)
mwolff removed subscribers: anthonyfieroni, dfaure.

update commit message

mwolff edited the summary of this revision. (Show Details)Dec 5 2017, 2:15 PM
anthonyfieroni accepted this revision.Dec 5 2017, 3:07 PM

Let's David says but i think KOpenWithDialog should be fixed as well, why we need new KLineEdit while KHistoryComboBox do it for us?
https://phabricator.kde.org/source/kio/browse/master/src/widgets/kopenwithdialog.cpp;befcbbd4e36b8f2a948e2baa88a0642e24d55564$601

This revision is now accepted and ready to land.Dec 5 2017, 3:07 PM
dfaure accepted this revision.Dec 5 2017, 9:37 PM
dfaure added a comment.Dec 5 2017, 9:45 PM

@anthonyfieroni commit 0c4f04b074 seems to have added that KLineEdit just to be able to show the clear button. This could just be ported to QLineEdit::setClearButtonEnabled these days.

This revision was automatically updated to reflect the committed changes.