Differential D11044

Don't try to find an icon for an empty url
ClosedPublic
Actions

Authored by jtamate on Mar 5 2018, 9:43 AM.

Details

Reviewers
dfaure
Group Reviewers
Frameworks
Commits
R241:8ba93b82e451: Don't try to find an icon for an empty url
Summary

The message kf5.kio.core: Refilling KProtocolInfoFactory cache in the hope to find ""
was produced when findProtocol was called by KProtocolInfo::icon that was
called with an empty url.

Test Plan

No more Refilling KProtocolInfoFactory cache in the hope to find "" when browsing net or samba shares.

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
jtamate created this revision.Mar 5 2018, 9:43 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 5 2018, 9:43 AM
jtamate requested review of this revision.Mar 5 2018, 9:43 AM
jtamate added a dependency: D11013: Detect incorrect paramenter in findProtocol.
broulik added a subscriber: broulik.Mar 5 2018, 10:05 AM

Shouldn't it check emptiness of the url scheme specifically?

dfaure requested changes to this revision.Mar 5 2018, 10:13 AM

Wait, who's calling KIO::iconNameForUrl() with an empty URL? This should be an assert or an early-return at the top of that method, not nested into one specific if(). Even the very beginning, db.mimeTypeForUrl(url), makes no sense for an empty URL.

This revision now requires changes to proceed.Mar 5 2018, 10:13 AM
jtamate added a comment.Mar 5 2018, 10:40 AM
In D11044#218896, @dfaure wrote:

Wait, who's calling KIO::iconNameForUrl() with an empty URL? This should be an assert or an early-return at the top of that method, not nested into one specific if(). Even the very beginning, db.mimeTypeForUrl(url), makes no sense for an empty URL.

When I just click in network in the save dialog, KFileWidgetPrivate::_k_urlEntered just has the following data:
url= QUrl("remote:/") but filename=locationEditCurrentText() == ""

#11 0x00007f96d01cca5b in KProtocolInfo::icon (_protocol=...) at /home/jtorres/kdesrc/frameworks/kio/src/core/kprotocolinfo.cpp:251
#12 0x00007f96d01c87c0 in KIO::iconNameForUrl (url=...) at /home/jtorres/kdesrc/frameworks/kio/src/core/global.cpp:332
#13 0x00007f96ba0c2cb5 in KFileWidgetPrivate::_k_urlEntered (this=0x7f96b40091a0, url=...) at /home/jtorres/kdesrc/frameworks/kio/src/filewidgets/kfilewidget.cpp:1487
#14 0x00007f96ba0ca25d in KFileWidget::qt_static_metacall (_o=0x210a3a0, _c=QMetaObject::InvokeMetaMethod, _id=9, _a=0x7ffdb629f960) at /home/jtorres/kdesrc/build/frameworks/kio/src/filewidgets/KF5KIOFileWidgets_autogen/include/moc_kfilewidget.cpp:176
#15 0x00007f96cbed302a in QMetaObject::activate(QObject*, int, int, void) () from /usr/lib64/libQt5Core.so.5
#16 0x00007f96ba0a6eef in KDirOperator::urlEntered (this=0x218f400, _t1=...) at /home/jtorres/kdesrc/build/frameworks/kio/src/filewidgets/KF5KIOFileWidgets_autogen/include/moc_kdiroperator.cpp:602
#17 0x00007f96ba09a83d in KDirOperator::setUrl (this=0x218f400, _newurl=..., clearforward=true) at /home/jtorres/kdesrc/frameworks/kio/src/filewidgets/kdiroperator.cpp:1040
#18 0x00007f96ba0c2bea in KFileWidget::setUrl (this=0x210a3a0, url=..., clearforward=true) at /home/jtorres/kdesrc/frameworks/kio/src/filewidgets/kfilewidget.cpp:1469
#19 0x00007f96ba0c2f52 in KFileWidgetPrivate::_k_enterUrl (this=0x7f96b40091a0, url=...) at /home/jtorres/kdesrc/frameworks/kio/src/filewidgets/kfilewidget.cpp:1523
#20 0x00007f96ba0ca280 in KFileWidget::qt_static_metacall (_o=0x210a3a0, _c=QMetaObject::InvokeMetaMethod, _id=10, _a=0x7ffdb629fd10) at /home/jtorres/kdesrc/build/frameworks/kio/src/filewidgets/KF5KIOFileWidgets_autogen/include/moc_kfilewidget.cpp:177
#21 0x00007f96cbed302a in QMetaObject::activate(QObject*, int, int, void) () from /usr/lib64/libQt5Core.so.5
#22 0x00007f96ba0e429f in KFilePlacesView::urlChanged (this=0x22c4f40, _t1=...) at /home/jtorres/kdesrc/build/frameworks/kio/src/filewidgets/KF5KIOFileWidgets_autogen/include/moc_kfileplacesview.cpp:263
#23 0x00007f96ba0e2a3a in KFilePlacesView::Private::setCurrentIndex (this=0x2267fb0, index=...) at /home/jtorres/kdesrc/frameworks/kio/src/filewidgets/kfileplacesview.cpp:1113
#24 0x00007f96ba0e3719 in KFilePlacesView::Private::_k_placeClicked (this=0x2267fb0, index=...) at /home/jtorres/kdesrc/frameworks/kio/src/filewidgets/kfileplacesview.cpp:1330
#25 0x00007f96ba0e3e41 in KFilePlacesView::qt_static_metacall (_o=0x22c4f40, _c=QMetaObject::InvokeMetaMethod, _id=9, _a=0x7ffdb629ff20) at /home/jtorres/kdesrc/build/frameworks/kio/src/filewidgets/KF5KIOFileWidgets_autogen/include/moc_kfileplacesview.cpp:167
#26 0x00007f96cbed302a in QMetaObject::activate(QObject*, int, int, void**) () from /usr/lib64/libQt5Core.so.5
#27 0x00007f96cd0e6cd5 in QAbstractItemView::clicked(QModelIndex const&) () from /usr/lib64/libQt5Widgets.so.5
#28 0x00007f96cd0e9a4b in QAbstractItemView::mouseReleaseEvent(QMouseEvent*) () from /usr/lib64/libQt5Widgets.so.5
#29 0x00007f96cd126cbf in QListView::mouseReleaseEvent(QMouseEvent*) () from /usr/lib64/libQt5Widgets.so.5

jtamate updated this revision to Diff 28690.Mar 5 2018, 10:57 AM

Checking for empty url at the top of the method.

dfaure accepted this revision.Mar 5 2018, 11:57 AM
This revision is now accepted and ready to land.Mar 5 2018, 11:57 AM
Closed by commit R241:8ba93b82e451: Don't try to find an icon for an empty url (authored by jtamate). · Explain WhyMar 5 2018, 6:27 PM
This revision was automatically updated to reflect the committed changes.
dfaure added a comment.Mar 19 2018, 1:16 PM

Kai-Uwe was right, this should actually check for scheme.isEmpty(). Otherwise typing ~ in kfilewidget (which leads to a relative URL without scheme) hits this same assert again. I hate relative URLs... I'll change it.

Revision Contents
Changeset List

PathPackages
Msrc/core/global.cpp (5 lines)

Diff 28739

View Options

src/core/global.cpp

Loading...