Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Boudhayan Gupta]

> On Nov. 19, 2015, 1:36 a.m., Thomas Lübking wrote:
> > Errr... you intend to crash applications depending on whether some file is present??
> > Please fix the actual bug instead of such workaround, got a backtrace?
> 
> Boudhayan Gupta wrote:
>     Err... it **doesn't** cause a crash with the patch, causes a crash without it. This is a perfectly valid fix - this function is **supposed** to return false if the database could not be opened, for any reason whatsoever.
> 
> Thomas Lübking wrote:
>     I didn't say this patch would introduce a crash.
>     I said that "something" crashes for "some" reason and you work around that by testing whether a file is present.
>     Your own commit message clearly says that if - for any reason - ~/.local/share/baloo/index exists (eg. i touch it), things will crash again, what means that the actual problem is not resolved but just shadowed.
> 
> Boudhayan Gupta wrote:
>     Why would you want to touch ~/.local/share/baloo/index, if you disable baloo?
>     
>     Let me explain - balooctl disable works by disabling indexing and removing ~/.local/share/baloo/index. If baloo is disabled, that file is not supposed to exist. A couple of months ago we were even trying to figure out how that file came to exist on systems with Baloo disabled and now I've found out. So this fixes that too.
>     
>     Now the real problem. Before this patch, Baloo::Database::open() would just create an empty database. Fair enough, except that db->open() would return true in places where you clearly have an invalid database. This would still work (i.e., not crash), because invalid in this context is empty. However, once you select a ton of files (the number is very weird  - 158 or something) LMDB would scream at you with this problem - **MDB_READERS_FULL: Environment maxreaders limit reached**
>     
>     There are two ways to solve this - increase the maxreaders limit in LMDB, or return false if the database doesn't exist. The first option is when you want to go all Linus - *we do not break userspace*, but this way is the more correct way of solving this, because again, on systems where Baloo is disabled, the index file isn't supposed to exist at all.
>     
>     I was afraid I was opening up a can of worms by adding yet another condition for db->open() to return false, because I was afraid there were places where a check on db->open()'s return value was missing (and someone would try to do operations on an invalid database). However, I've done all the things that used to trigger crashes before and nothing's crashed yet. With Baloo being both enabled and disabled.
> 
> Thomas Lübking wrote:
>     That sounds as if a ton of jobs is started which all try to access the database?
>     W/o knowing the details on what is going on, there should probably:
>     
>     a) a sane limit per client on how many DB jobs to perform at once (general performance issue)
>     b) a sanity check on the index file ie. testing whether it contains some significant bytes or at least isn't empty.
>     
>     I guess (b) should actually done by lmdb, but it won't hurt to do it on baloo as well
>     
>     Is this also a problem if the file is actually a valid DB, but baloo just disabled?
>     
>     
>     ---
>     General remark: I guess valid databases should not be deleted but at best be renamed with de/activation, no matter what else happens with this patch.

I'll put this down to a LMDB bug (I suspect some race condition) because when the db is active and indexing is enabled, the crash doesn't happen. Therefore I see no sense in increasing the limit. As for b), then I'd have to know the LMDB file format to do it in Baloo (by default an empty database as created by this method is 8KB in size, so there's some sort of data in it).

For your general remark (rename dbs instead of deleting them) - you'd have to ask Vishesh. That's a design decision left to the maintainer. I'm just a drive-by patcher who's somewhat familiar with the codebase because I've fixed quite a few crashes in other apps when Baloo is disabled ;-)


- Boudhayan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126105/#review88544
-----------------------------------------------------------


On Nov. 19, 2015, 1:10 a.m., Boudhayan Gupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> -----------------------------------------------------------
> 
> (Updated Nov. 19, 2015, 1:10 a.m.)
> 
> 
> Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> -------
> 
> Add a check in Baloo::Database::open() to return false if we're opening the database in ReadOnly mode and the database doesn't exist. This fixes a crash in Dolphin when Baloo isn't running.
> 
> This isn't the entire fix - one will also have to remove ~/.local/share/baloo/index to not crash anymore; this patch prevents the file from being recreated everytime Baloo::Database::open() is run, and re-causing the crash.
> 
> 
> Diffs
> -----
> 
>   src/engine/database.cpp 4f0579f 
> 
> Diff: https://git.reviewboard.kde.org/r/126105/diff/
> 
> 
> Testing
> -------
> 
> Builds, runs, doesn't crash anymore, the works.
> 
> 
> Thanks,
> 
> Boudhayan Gupta
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20151119/dece8f90/attachment.html>