Bug 435161

(In reply to Oded Arbel from comment #0)
> Which means that `KInotify::moved` is emitted for both directories and
> files, but `FileWatch::slotFileMoved` expects only files and enforces that
> with an assert.

Lookgin deeper into `FileWatch::slotFileMoved`, it does expect to handle directories in some cases, but for calls that are forwarded to `MetadataMover::moveFileMetadata()` - it doesn't care. `MetadataMover::moveFileMetadata()` itself doesn't seem to care either, but it calls `MetadataMover::updateMetadata()` that will crash on paths that end with '/'.

Maybe the correct approach would be to have `MetadataMover::updateMetadata()` silently ignore paths that end with '/'?

(In reply to Oded Arbel from comment #0)
> Regardless, the
> directory no longer exists - I think it was moved than deleted before baloo
> got a chance to index it, but I don't think that is related to the problem.

This could actually be related to the problem because I have configured `$HOME/.cache` to be excluded from scanning so it shouldn't have been scanned.

`FileWatch::slotFileMoved` consults ``FileIndexerConfig::shouldBeIndexed()` which in turn uses `QFileInfo::isDir()` which - as far as I understand - check if the path is an actual directory and will return `false` for paths that don't exist. I think the case where a directory was moved and then deleted is not handled correctly and will cause weird behaviors that might trigger asserts.

Not sure whether this is related...

It's a glitch I get "every so often". When I've missed capturing it, I've not found a way of reproducing it...

    cd ~/Documents
    mkdir temp
    echo "Hello Penguin" > temp/file1.txt
    balooshow -x temp/file1.txt

gave:

    144da00000fc01 64513 1330592 temp/file1.txt [/home/test/.kde/share/config/kdeglobals]
            Mtime: 1617136526 2021-03-30T22:35:26
            Ctime: 1617136526 2021-03-30T22:35:26
            Cached properties:
                    Line Count: 158

    Internal Info
    Terms: 0 0.025 ... Mplain Mtext T5 T8 X20-158 activebackground activeblend activeforeground backgroundalternate backgroundnormal ... widgetstyle window wm
    File Name Terms: Fkdeglobals
    XAttr Terms:
    lineCount: 158

That is, the details of something else completely (a different file, indexed at a different time). "Balooctl monitor" did not show "file1.txt" being indexed.

After a reboot I get the more expected:

    144da00000fc01 64513 1330592 temp/file1.txt [/home/test/Documents/temp/file1.txt]
            Mtime: 1617270546 2021-04-01T11:49:06
            Ctime: 1617270546 2021-04-01T11:49:06
            Cached properties:
                    Line Count: 1

    Internal Info
    Terms: Mplain Mtext T5 T8 X20-1 hello penguin
    File Name Terms: Ffile1 Ftxt
    XAttr Terms:
    lineCount: 1

I was hoping to be able to snapshot/clone the "confused" system, seemingly not this time though :-(

It may be that if such a "confused" system is referencing a directory rather than a file then you might get such an assert....

    Neon Unstable
    Plasma: 5.21.80
    Frameworks: 5.81.0
    Qt: 5.15.2

Had to trim the list of terms in the previous post as the full list looked too much like spam.

I just hit this same ASSERT() in baloo built from git master, and the pathname was: "/home/bernie/thunderbird.bak/".

Invalid encoding. Ignoring "/home/bernie/emu/amiga/amigahd/Work/Network/Programs/MicroDot/Data/DBX_60/EDJAEDCOBDJEJLABDMJFJNPKGINIEG"
ASSERT: "from[from.size()-1] != QLatin1Char('/')" in file /home/bernie/kde/src/baloo/src/file/metadatamover.cpp, line 80

(gdb) bt
#0  0x00007ffff70cdd22 in raise () from /usr/lib/libc.so.6
#1  0x00007ffff70b7862 in abort () from /usr/lib/libc.so.6
#2  0x00007ffff7680910 in QMessageLogger::fatal(char const*, ...) const () from /usr/lib/libQt5Core.so.5
#3  0x00007ffff767fcf5 in qt_assert(char const*, char const*, int) () from /usr/lib/libQt5Core.so.5
#4  0x00005555555908b2 in Baloo::MetadataMover::updateMetadata (this=0x5555555d9800, tr=0x7fffffffcc00, from="/home/bernie/thunderbird/", to="/home/bernie/thunderbird.bak/") at /home/bernie/kde/src/baloo/src/file/metadatamover.cpp:80
#5  0x00005555555904aa in Baloo::MetadataMover::moveFileMetadata (this=0x5555555d9800, from="/home/bernie/thunderbird/", to="/home/bernie/thunderbird.bak/") at /home/bernie/kde/src/baloo/src/file/metadatamover.cpp:42
#6  0x000055555558a1ed in Baloo::FileWatch::slotFileMoved (this=0x7fffffffd570, urlFrom="/home/bernie/thunderbird/", urlTo="/home/bernie/thunderbird.bak/") at /home/bernie/kde/src/baloo/src/file/filewatch.cpp:77
...

I'm able to trigger a similar assertion a different location by simply creating a new directory in my home:



...
[Detaching after fork from child process 31519]
QIBusPlatformInputContext: invalid portal bus.
QSocketNotifier: Can only be used with threads started with QThread
[Detaching after fork from child process 31538]
QIBusPlatformInputContext: invalid portal bus.
QSocketNotifier: Can only be used with threads started with QThread
ASSERT: "!filePath.endsWith(QLatin1Char('/'))" in file /home/bernie/kde/src/baloo/src/file/newfileindexer.cpp, line 38

Thread 4 "Thread (pooled)" received signal SIGABRT, Aborted.
[Switching to Thread 0x7ffff1680640 (LWP 31481)]
0x00007ffff70cdd22 in raise () from /usr/lib/libc.so.6
(gdb) bt
#0  0x00007ffff70cdd22 in raise () from /usr/lib/libc.so.6
#1  0x00007ffff70b7862 in abort () from /usr/lib/libc.so.6
#2  0x00007ffff7680910 in QMessageLogger::fatal(char const*, ...) const () from /usr/lib/libQt5Core.so.5
#3  0x00007ffff767fcf5 in qt_assert(char const*, char const*, int) () from /usr/lib/libQt5Core.so.5
#4  0x0000555555579e99 in Baloo::NewFileIndexer::run (this=0x555555870bf0)
    at /home/bernie/kde/src/baloo/src/file/newfileindexer.cpp:38
#5  0x00007ffff76c1332 in ?? () from /usr/lib/libQt5Core.so.5
#6  0x00007ffff76be02f in ?? () from /usr/lib/libQt5Core.so.5
#7  0x00007ffff6016259 in start_thread () from /usr/lib/libpthread.so.0
#8  0x00007ffff718f5e3 in clone () from /usr/lib/libc.so.6
(gdb) f 4
#4  0x0000555555579e99 in Baloo::NewFileIndexer::run (this=0x555555870bf0)
    at /home/bernie/kde/src/baloo/src/file/newfileindexer.cpp:38
38              Q_ASSERT(!filePath.endsWith(QLatin1Char('/')));
(gdb) list
33              : BasicIndexingJob::MarkForContentIndexing;
34
35          Transaction tr(m_db, Transaction::ReadWrite);
36
37          for (const QString& filePath : std::as_const(m_files)) {
38              Q_ASSERT(!filePath.endsWith(QLatin1Char('/')));
39
40              QString mimetype;
41              QFileInfo fileInfo(filePath);
42
(gdb) p filePath
$1 = "/home/bernie/foo/"

I don't see how these asserts could have ever worked in the past. I traced back the pathnames, and the '/' is appended to all directories with normalizeTrailingSlash(): https://invent.kde.org/frameworks/baloo/-/blob/master/src/file/kinotify.cpp#L359

So, new directories will always have a trailing slash, why is NewFileIndexer::run() not expecting to see them?
My first hypothesis was that NewFileIndex is only for processing files. But there's a specific check for directories a few lines after the Q_ASSERT that fails: ​https://invent.kde.org/frameworks/baloo/-/blob/master/src/file/newfileindexer.cpp#L47-51

Code to append slashes to directories was added ~1 year ago:
https://invent.kde.org/frameworks/baloo/-/commit/73183acf00a2b7f10b286a1241716cb7e0785f0d

A few months later, we removed a couple of asserts on directory removal and rename:
https://invent.kde.org/frameworks/baloo/-/commit/96284739b4b63110113af0ba7dedfd18c4459063

The second check was missing a '!', and it was added shortly after by Nate:
https://invent.kde.org/frameworks/baloo/-/commit/dc04879f058dd1eab72e4bd4c80bdc1ea3c06c9b

So I think we missed directory creation and a few other cases. It should be safe to keep trailing slashes, because BasicIndexingJob strips them in the constructor:
https://invent.kde.org/frameworks/baloo/-/blob/master/src/file/basicindexingjob.cpp#L26-28

There are other asserts disallowing trailing slashes. These are placed just before code like this:
  QString fileName = filePath.mid(filePath.lastIndexOf(QLatin1Char('/')) + 1);

So I think we can't simply remove the check, as it will set fileName to an empty string. QFileInfo::fileName() is explicitly documented to behave like this:
  "Note that, if this QFileInfo object is given a path ending in a slash, the name of the file is considered empty."

On a tangential note, why didn't the autotests catch this regression?

A possibly relevant merge request was started @ https://invent.kde.org/frameworks/baloo/-/merge_requests/71

Just to be clear: I could 100% reproduce the crash in baloo_file with these directory operations in my home:

  cd ~
  mkdir foo   <- CRASH!
  mv foo bar  <- CRASH!
  chmod -x bar  <- CRASH!

(In reply to Bernie Innocenti from comment #10)
> ... 100% reproduce ...
I remember having a go at building a "with a clean index" test and got nowhere. Chasing up what would happen if an inode was reused but I couldn't pin down anything.

My recollection is that the Asserts were included (and kept) to flush out "bad cases" before they made things worse. Alas, cannot remember/find where I read that.

(In reply to tagwerk19 from comment #11)
> ... but got nowhere ...
I stumbled on this again (Bug 478854)

I can reliably reproduce on neon-unstable; on newly-installed, as vanilla as possible, systems but if I try the same on a neon-testing, I don't get failures.

Baloo::MetadataMover::updateMetadata

got fixed in

commit 27a5e70abe6479dee045f5ed8412c55fc1d87243
Author: Méven Car <meven@kde.org>
Date:   Wed Jan 15 18:35:47 2025 +0100

    MetadataMover::updateMetadata: tolerate path ending with /

    Since KInotify can return path ending with '/', instead of asserting,
    sanitize the input data by chopping the end /.

    SENTRY: BALOO-1H4

in KF6.

The /home/bernie/kde/src/baloo/src/file/newfileindexer.cpp:38

in

commit 33756dd44c4f67a7984fa4c5da61726b46aa6bc6
Author: Méven Car <meven.car@kdemail.net>
Date:   Fri Mar 29 17:52:28 2024 +0000

    Some indexers need non-slash trailed path
    
    Fix regression introduced in 73183acf, prior to this commit all path
    from KInotify did not have a trailing slash, adjust in consequence.
    
    9b61371fdefbd538938f20cdc87eed03d170fa5b adjusted BasicIndexingJob but
    not the other cases.
    
    Add some missing unit test cases, although they don't cover the bug
    cases, they allow to illustrate the crash conditions.
    
    BUG: 478854
    
    Superseeds https://invent.kde.org/frameworks/baloo/-/merge_requests/119
    
    cc @tagwerk19.invent @cullmann

*** Bug 497014 has been marked as a duplicate of this bug. ***