Bug Fixes (2x addon related. 1x possibly serious filesystem cache related)

[matthuisman]

Fix 1
If you search via Add-ons > Search and there are multiple repos with a same add-on ID,
Kodi will show multiple entries of the same add-on.

Fix 2
Noticed that the database isn't closed before this return statement.
It is returned everywhere else. Potential db is kept open.

Fix 3
This is quite serious. The DirectoryCache becomes out of sync with the real filesystem.
Causing kodi to think directories still exist when they don't.
Easiest way to see the behavior is

1. Install add-on from zip
2. Uninstall add-on
3. Install same add-on from zip again
4. It fails to install

When Kodi tries to install it again, it still thinks the addon path exists.
Error in log is

2020-04-20 01:11:40.423 T:18860   ERROR <general>: XFILE::CFile::Rename - Error renaming file C:\Kodi\addons\inputstream.adaptive
2020-04-20 01:11:40.423 T:18860   ERROR <general>: Failed to move old addon files from 'C:\Kodi\addons\inputstream.adaptive' to 'C:\Kodi\addons\temp\4b6a0f80-44c6-428b-b24d-aa9c7070ca88'

The below folder is removed when uninstalling
"C:\Kodi\addons\inputstream.adaptive\"

The cache layout looks like this (key is the parent directory)

{
   "C:\Kodi\addons": ["inputstream.adaptive"]
}

We need to remove "C:\Kodi\addons" cache key.

The add-on is uninstalled here
https://github.com/xbmc/xbmc/blob/master/xbmc/addons/FilesystemInstaller.cpp#L70
That calls CFile::Rename
https://github.com/xbmc/xbmc/blob/master/xbmc/filesystem/File.cpp#L905
which calls g_directoryCache.ClearFile
https://github.com/xbmc/xbmc/blob/master/xbmc/filesystem/DirectoryCache.cpp#L115

this is where the problem is.
ClearFile assumes its getting a filepath but often it gets a directory path
It calls URIUtils::GetDirectory to get the parent directory of the path.
However, GetDirectory on a Directory returns itself.

The current flow is:

CDirectoryCache::ClearFile: C:\Kodi\addons\inputstream.adaptive\
URIUtils::GetDirectory: C:\Kodi\addons\inputstream.adaptive\
CDirectoryCache::ClearDirectory: C:\Kodi\addons\inputstream.adaptive\
URIUtils::RemoveSlashAtEnd: C:\Kodi\addons\inputstream.adaptive

That is not in the cache so nothing is removed.
We need ClearFile to remove any trailing slash so it works correctly on directories and files.
That is what my last fix does.

Once that line is added, it goes:

CDirectoryCache::ClearFile: C:\Kodi\addons\inputstream.adaptive\
URIUtils::RemoveSlashAtEnd: C:\Kodi\addons\inputstream.adaptive
URIUtils::GetDirectory: C:\Kodi\addons\
CDirectoryCache::ClearDirectory: C:\Kodi\addons\
URIUtils::RemoveSlashAtEnd: C:\Kodi\addons

Filepath are unaffected as the added RemoveSlashAtEnd doesn't affect them.

Who knows where else this has been causing issues.
Any renaming of directories could cause it.
Seems to be just in Matrix

[phunkyfish]

So just some minor stuff.

[phunkyfish]

So the formatting needs to part of the commit it's from. Not a separate one at the end. Is that ok?

[matthuisman]

your really testing my git skills!
Oh man - that was scary.

git rebase --interactive 0e954dea9196f2b890b03277efdca558b8183108
# change desired commit to "edit"
# make the changes
git commit --all --amend --no-edit
git rebase --continue
git reset
git push -f matt HEAD:addon_bug_fix

[phunkyfish]

your really testing my git skills!
Oh man - that was scary.

git rebase --interactive 0e954dea9196f2b890b03277efdca558b8183108
# change desired commit to "edit"
# make the changes
git commit --all --amend --no-edit
git rebase --continue
git reset
git push -f matt HEAD:addon_bug_fix

Practice makes perfect. You could have used fixup's. So let's say the commit you want to change has hash XXXX and it's the 3rd commit in the past. Then once you have the edits handy just do something like this. When we rebase we do the position of the commit we are changing + 1 for the fixup.

git add <the changes>
git commit --fixup XXXX
git rebase -i HEAD~4 --autosquash

[phunkyfish]

Nice, thanks for the fixes.

[phunkyfish]

jenkins build this please

[phunkyfish]

jenkins build this please

[MilhouseVH]

FYI, fix 3 is mentioned in issue #17049 with a fix in #17579 however the current fix appears to be causing additional issues.

I'll include this PR in my nightly test builds (starting with #0419) to see how it fairs, thanks!

[phunkyfish]

I'll include this PR in my nightly test builds (starting with #0419) to see how it fairs, thanks!

Thanks!

[phunkyfish]

@AlwinEsch how do these fixes look to you?

[AlwinEsch]

Your changes are make sense and good to have.

[matthuisman]

Yah. This is my first addition into kodi core in over 10 years. Back then I added ability to insert a list item at a position. Up to then you could only append. Think it was in the addon window XML stuff and a user called donniedarko (or something close) committed it for me. Probably around 2008

[phunkyfish]

Yah. This is my first addition into kodi core in over 10 years. Back then I added ability to insert a list item at a position. Up to then you could only append. Think it was in the addon window XML stuff and a user called donniedarko (or something close) committed it for me. Probably around 2008

First of many I hope 😉 We’ll wait a few days to see if there is any feedback from Milhouse and if it’s all good we can merge it.

[MilhouseVH]

Feedback on personal testing - it's all good so far! This PR appears to have fixed issue #17049

1. Install/Uinstall from Repo: Manually installing the YouTube addon from the Kodi repo, then uninstalling/installing (and repeating this cycle several times) is working flawlessly

2. Automatic update from Repo: I manually edited the version of an installed addon (metadata.album.universal, changing the version in addon.xml from 3.1.2 to 3.1.1) and then after restarting kodi it correctly found that an update was available (version 3.1.2) and it applied the update successfully, so that's working too

3. Install from Zip: Installing an add-on from zip, then uinstalling and re-installing the same addon from zip, is working perfectly

[matthuisman]

That change to the cache couldn't actually cause any issues.
Worst case, it removes something from the cache when it wasn't needed too and an extra lookup is needed. That is much better than not removing something when it needs to.

[phunkyfish]

FYI, fix 3 is mentioned in issue #17049 with a fix in #17579 however the current fix appears to be causing additional issues.

I'll include this PR in my nightly test builds (starting with #0419) to see how it fairs, thanks!

Ok, then I think we consider merging this and closing #17579.

Will do this in a couple days assuming further testing is ok.

[phunkyfish]

Ok, I will merge this later today as there has been no (negative) feedback).