D12371: fix always reproducible crash

dfaure accepted this revision. dfaure added a comment. Patch looks good, but please update the first line of the commit message so that it gives more context (think about the changelog, where the rest of the description won't be visible...) REPOSITORY R241 KIO REVISION DETAIL https://ph

D12371: fix always reproducible crash

jtamate updated this revision to Diff 33872. jtamate marked an inline comment as done. jtamate edited the summary of this revision. jtamate added a comment. Restricted Application added a subscriber: kde-frameworks-devel. The crash after the redirection in fish://127.0.0.1 was due to a remainin

D12371: fix always reproducible crash

dfaure added a comment. "I'm redirected to fish://127.0.0.1/home/jtorres" is exactly what is supposed to happen. If that asserts nowadays, it means there's a regression in the redirect handling. Sounds like this has to be fixed first. QUrl::toString is slowish btw, it has to assemble th

D12371: fix always reproducible crash

jtamate updated this revision to Diff 33242. jtamate edited the summary of this revision. jtamate added a comment. If I use if (!u.path().isEmpty()) or if (!u.path().isEmpty() && !u.path().endsWith('/')), as soon as I enter the url fish://127.0.0.1 (without trailing /), I'm redirected to f

D12371: fix always reproducible crash

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kfilewidget.cpp:1523 > // append '/' if needed: url combo does not add it > // tokenize() expects it because uses KUrl::setFileName() > QUrl u(u

D12371: fix always reproducible crash

jtamate updated this revision to Diff 33211. jtamate edited the summary of this revision. jtamate edited the test plan for this revision. jtamate added a comment. The responsible for changing smb:// into smb:/// was: void KFileWidgetPrivate::_k_enterUrl(const QUrl &url) With this patch, the

D12371: fix always reproducible crash

dfaure added a comment. This is unrelated to concatPaths because concatPaths is about absolute+relative. smb:// + / isn't about adding a relative path. But yes, please find out where smb:/// comes from, and then we can discuss solutions ;) (Check how `kioclient5 ls smb://` and `k

D12371: fix always reproducible crash

anthonyfieroni added a comment. So, it should be Dolphin even empty path + '/' will result in error? And in other review concatPath can make it, what you think? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12371 To: jtamate, dfaure, #frameworks, apol Cc: anthonyfiero

D12371: fix always reproducible crash

dfaure added a comment. After the setHost and the setPath, it'll be smb://host/path, so that one doesn't have any effect (unless both host and path are empty). And this is in the special() command so it doesn't get into the app's KCoreDirLister. REPOSITORY R241 KIO REVISION DETAIL https

D12371: fix always reproducible crash

anthonyfieroni added a comment. @dfaure, off i mean smb:/// it can be seen in smb slave https://phabricator.kde.org/source/kio-extras/browse/master/smb/kio_smb_mount.cpp$66 But why? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12371 To: jtamate, dfaure, #framewor

D12371: fix always reproducible crash

dfaure added a comment. Anthony: do you mean smb:// ? file:/// with 3 slashes is most definitely correct (file://kk would mean a hostname of "kk"). And file://kk doesn't appear anywhere in the output, so surely there's no problem there. The question is which piece of code confuses smb:// and

D12371: fix always reproducible crash

anthonyfieroni added a comment. Can you verify who calls lister with file:///kk -> this is not correct it should be file://kk (2 slashes) then Cache will start to work again. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12371 To: jtamate, dfaure, #frameworks, apol Cc

D12371: fix always reproducible crash

jtamate updated this revision to Diff 33045. jtamate edited the test plan for this revision. jtamate added a comment. Just to know if I'm in the correct path, because I'm getting out of ideas. Use in the hash table, and probably in other parts, always the url with at most one trailing /.

D12371: fix always reproducible crash

jtamate added a comment. > One solution is to call printDebug(), which will output lots of information including the contents of directoryData(). I have some questions: Is a KCoreDirLister able to handle more than one directory? The same KCoreDirLister pointer is holding smb:// and smb

D12371: fix always reproducible crash

dfaure added a comment. It looks like the issue is that forgetDirs() doesn't find smb:// on the line of code that says `DirectoryDataHash::iterator dit = directoryData.find(urlStr);.` So it goes into early return and doesn't do dirData.listersCurrentlyHolding.removeAll(lister); which la

D12371: fix always reproducible crash

jtamate added a comment. More investigation, in bold what causes the problem. moveListersWithoutCachedItemsJob append KDirLister(0x3b510c70) to holders for url= QUrl("file:///virtual/kde5/5kde/build/frameworks/kio") forgetDirs removeAll in directoryData[ "file:///virtual/kde5/5kde/bui

D12371: fix always reproducible crash

dfaure requested changes to this revision. dfaure added a comment. I find this solution horrible. QPointers screams "we designed this wrongly, the ownership rules are unclear, stuff gets deleted randomly, we have to guard against it". So while I don't deny that we might have designed thi

D12371: fix always reproducible crash

jtamate added a comment. In D12371#250366 , @apol wrote: > ^^' this won't work though. If the object gets deleted you will get a null pointer in the list. If the QPointer kdl detects that the guarded pointer is invalid it is removed from

D12371: fix always reproducible crash

apol requested changes to this revision. apol added a comment. This revision now requires changes to proceed. ^^' this won't work though. If the object gets deleted you will get a null pointer in the list. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12371 To: jtamat

D12371: fix always reproducible crash

jtamate updated this revision to Diff 32640. jtamate edited the summary of this revision. jtamate edited the test plan for this revision. jtamate added a comment. Implemented as Aleix suggested, using QPointer. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D123

D12371: fix always reproducible crash

apol added inline comments. INLINE COMMENTS > kdirmodel.cpp:401 > { > +// dirLister is probably onwed by KCoreDirListerCache > if (d->m_dirLister) { Maybe KCoreDirListerCache should be using a QPointer? or checking if it gets deleted? > kdirmodel.h:65 > */ > +// Can't take

D12371: fix always reproducible crash

jtamate created this revision. jtamate added reviewers: dfaure, Frameworks. Restricted Application added a project: Frameworks. jtamate requested review of this revision. REVISION SUMMARY Don't take ownership of KDirLister in KDirModel. When KDirModel is destroyed, it also deleted the dirliste