Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JSON]Fix Player.GetItem during track change seg fault #14454

Merged
merged 1 commit into from Sep 25, 2018

Conversation

@DaveTBlake
Copy link
Member

commented Sep 19, 2018

Experiencing infrequent seg fault crashes when JSON Player.GetItem requests were made during music playback. See https://forum.kodi.tv/showthread.php?tid=298461&pid=2768289#pid2768289 for crash log summary.

The cause was found by code inspection: the JSON thread was using a const ref string pointer which could be made invalid by CApplication::m_itemCurrentFile being simultaneously reset in the main thread, as happens at track change. Crucially timmed JSON requests could result in invalid calls to CUtil::ValidatePath and a crash.

The code was already testing that the string passed by const ref was not empty, but did not allow for it subsequently vanishing during the rest of the process. While m_itemCurrentFile is protected by being a shared pointer, the path string itself was not.

Tested on RPi2 (thanks for build @MilhouseVH ), sending repeated Player.GetItem requests in rapid succession using a script while Kodi was playing a playlist of short songs (hence change track happening frequently). This produced a seg fault in LibreElec v8.90.004 (v18beta1) and v8.90.005 (v18beta2) builds fairly consistently within 4 hours of playback (yes that hard to reproduce the exact unfortunate timing in real use), but never managed to get it to fault with debug logging. The patched build did not crash despite repeated tests of longer duration.

Obviously the code change was conceptually necessary, but I think it reasonable to concude that it has also fixed the seg fault too.

Patch bump to JSON schema version

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Sep 19, 2018

this is not an api change

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Sep 19, 2018

It is a fix that effects the way an API method works hence the patch bump. It means that API consumers know something has changed internally, in this case a seg fault is avoided.
This was how @Montellese suggested that the schema versioning is used, see the JSON documentation.

EDIT: or is it the Git label you question? In this case I use "JSON-RPC API Change" label to mean something impacting the JSON API processing not necessarily a breaking change to the API itself. Again helpful in searches

@@ -163,12 +163,12 @@ JSONRPC_STATUS CPlayerOperations::GetItem(const std::string &method, ITransportL
if (fileItem->GetLabel().empty())
fileItem->SetLabel(originalLabel);
}
fileItem->SetPath(g_application.CurrentFileItem().GetPath());
fileItem->SetPath(fileItem->GetPath());

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Sep 19, 2018

Author Member

@FernetMenta this line is what looks most odd to me now. I want to rethink it before merge

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 19, 2018

Member

why does it look odd to you?

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 19, 2018

Member

right :) basically it is the same code as before but now it is more obvious that this line was nonsense.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 19, 2018

Member

I was wrong, it is not the same behaviour as before but I did not get the sense of what it was intended to do

          if (!CAudioLibrary::FillFileItem(g_application.CurrentFile(), fileItem, parameterObject))
          {
            const MUSIC_INFO::CMusicInfoTag *currentMusicTag = g_infoManager.GetCurrentSongTag();
            if (currentMusicTag != NULL)
              fileItem = CFileItemPtr(new CFileItem(*currentMusicTag));
            fileItem->SetPath(g_application.CurrentFileItem().GetPath());
          }

it changes file item to represent video tag, then set original path. all this strange logic is misplaced in what is supposed to be an API.

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Sep 20, 2018

Author Member

I looked back through Git to try and understand why @Montellese added it. I believe he had a coherent design in mind even if it is not obvious now to us what that was. The idea was to use the item data in CGUIInfoManager current video or music tag as a fallback if looking it up for the app current item path was unsuccessfull, without changing the path. See 73fd7b2. It was subsequently amended to allow for a null tag pointers etc., but that was the introduction back in 2012.

At that time the app current item was not the definative data, CGUIInfoManager was taken as a valid source. I also think that addons can edit the GUI item data, and since the API is trying to provide consumers with whatever data the GUI has (be that from db or elsewhere), then the processing here looks less strange.

With the changes in v18 to CApplication::m_itemCurrentFile do we still need to look for fallback item data in the GUI item? I'm not sure. The fallback comes into effect when playing non-library items, can GUI current item have more details about the item than the app current item? I think it might, so at this point I think we are best keeping this fallback, but preserving the initial path without accessing CApplication::m_itemCurrentFile a second time in case it has already changed on the main thread.

As for being misplaced, well yes it would it be better if some other common something was providing both GUI, Python API and JSON API with consistent full item data, but fixing a seg fault in beta I don't think we will get there today.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 21, 2018

Member

CApplication::m_itemCurrentFile is single source of truth. Changing db entry or guiInfo must only be done by Capp.
Hence any manipulation of fileItem here is wrong and should go away. This is true regardless of the item is in library or not.
Accessing guiInfo is even worse than accessing db because json-rpc is supposed to work when kodi runs headless.

In the current state the addition code does not really harm because, provided all goes right, it updates fimeItem with its own values.
Please place a todo comment that all this code should be dropped in v19.

Please also drop the api bump because this is not an api change.

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Sep 22, 2018

Author Member

CApplication::m_itemCurrentFile is single source of truth.

Yes @FernetMenta I know that is what is wanted, but it has not been achieved in v18. The code here does not manipulate anything, it simply continues to accommodate the fact that CApplication::m_itemCurrentFile does not always have all the item details that GUI item does.

Please place a todo comment that all this code should be dropped in v19.

I have added @todo, but it is not just about dropping this code that would simply cause a regression. More changes need to be made elsewhere to achieve CApplication::m_itemCurrentFile as the single source of truth.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Sep 19, 2018

It means that API consumers know something has changed internally, in this case a seg fault is avoided

Applying this rule you need to bump API with every single change. The PR fixes an internal bug which is not at the API level. Hence no API change. From a client perspective absolutely nothing changed.

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Sep 19, 2018

It is clearly documented (unusual for Kodi I know) that the patch number is "Bumped on any changes to the internal implementation but not to the API definition".

This PR directly changes the way Player. GetItem is implemented, the patch bump is approriate

@DaveTBlake DaveTBlake force-pushed the DaveTBlake:JSONPlayerGetItemFix branch from a7c6d8f to d31a82f Sep 20, 2018

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2018

@FernetMenta could you look at this current version of code, I just realised we were possibly discussing the outdated version.

An example of when the fallback to GUI item data is used is when playback of a non-library music file has been started via JSON. In that case CApplication::m_itemCurrentFile does not have the data scanned from tags, but the GUI item does and is showing it, and Player.GetItem returns this data to the API.

Depending on the GUI item in this way is obviously undesirable, but it is not something new and not something we can safely change during beta. I think we agree?

Another example is addons like Shoutcast2 that play internet streams where the content i.e. song title, can change without the player caring. In v16 the GUI and Player.GetItem both have updated titles, but I notice in does not get updated in v17 or 18. I don't know enough about the addon to know the cause of this breakage, but I can see the requirement for addons to be able to change the details of the current item, and for the API to accurately return it.

Rewritting Kodi to have a proper API used by GUI, Python and JSON would be great, meanwhile I think this PR is the way to maintain existing functionality while avoiding a seg fault.

fileItem->SetFromMusicInfoTag(*currentMusicTag);
if (fileItem->GetLabel().empty())
fileItem->SetLabel(originalLabel);
fileItem->SetPath(originalPath); // Ensure path unchanged

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 22, 2018

Member

this is wrong. path of an fileitem must never be changed because path is id.

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Sep 22, 2018

Author Member

fileitem must never be changed

Exactly. Without this line SetFromMusicInfoTag could have changed the path, this ensures the original path (that from the app current item) is retained.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 22, 2018

Member

SetFromMusicInfoTag must not change the path. It it does, please correct.

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Sep 22, 2018

Author Member

Path can be set by SetFromMusicInfoTag here

m_strPath = music.GetURL();

But no I am not willing to change something as fundamental SetFromMusicInfoTag at this stage in the dev cycle

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Sep 22, 2018

Author Member

To be precise it does not change the path, just sets it from url if it is empty. Is setting path from URL harmful? Is path 100% guarenteed to be non-empty? Have you tested all the possible ways that playback can be started?

fileItem->SetFromVideoInfoTag(*currentVideoTag);
if (fileItem->GetLabel().empty())
fileItem->SetLabel(originalLabel);
fileItem->SetPath(originalPath); // Ensure path unchanged

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 22, 2018

Member

wrong. see comment below.

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Sep 22, 2018

Author Member

See comment above - SetVideoInfoTag possibly changes path

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 22, 2018

Member

SetVideoInfoTag possibly changes path

please point me to the code where it does

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Sep 22, 2018

Author Member

SetVideoInfoTag sets path

m_strPath = video.m_strFileNameAndPath;

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 24, 2018

Member

check out this comment:

* @todo Ideally this (and SetPath) would not be available outside of construction

changing path after construction is not desired. Instead of calling SetVideoInfoTag, clients can construct a new object.

VideoInfoTag is stored in db with path as primary key. If path of VideoInfoTag does not equal originalPath things really go wrong.

If Tag obtained from gui is different to the one already got from CApp, things go wrong too. If you ever intend to improve the API by eliminating unneeded/wrong code, I suggest to place some comments and log warnings.

Apart from this, the changes here do not change behaviour (just fixes segfault). I was referring to code/design.

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Sep 24, 2018

Author Member

Understood. I will extend code comments

@@ -1 +1 @@
JSONRPC_VERSION 9.6.0
JSONRPC_VERSION 9.6.1

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 22, 2018

Member

still wrong

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Sep 22, 2018

Author Member

No patch bump is correct - see definition of patch

"patch": { "type": "integer", "minimum": 0, "required": true, "description": "Bumped on any changes to the internal implementation but not to the API definition" }

This PR changes xbmc/interfaces/json-rpc/PlayerOperations.cpp the internal implementation of the API. Schema version will then bump to 10.0.0 for v18RC

If the team wishes to change the definition of the schema version number for v19 then raise a PR on schema definition etc., meanwhile I am applying the currently correct and documented strategy expected by JSON consumers.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Sep 22, 2018

your recent changes made it worse.

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2018

your recent changes made it worse.

@FernetMenta would you please read the replies against the code, that explain my resaoning. This seg fault fix is both necessary and sufficient.

@DaveTBlake DaveTBlake force-pushed the DaveTBlake:JSONPlayerGetItemFix branch from d31a82f to 1660121 Sep 24, 2018

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2018

Apart from this, the changes here do not change behaviour (just fixes segfault). I was referring to code/design.

That is my aim with this PR - fix seg fault and keep behaviour.

Yes I understand the future design aims: that path must not change (and with this code it won't), that the app item has the definite data, and GUI item is not used as a source of anything. At very least this requires the way playback of non-library items from JSON is implemented to change, and a close examination of the other diverse ways playback can be requested, best of all would be for GUI, JSON and Python to all use the same API. This is beyond the scope of this PR and changes for beta.

The discussion here will be informative for anyone looking to work on this for v19 (easily found via Git blame). I will add example JSON calls that invoke the fallback to GUI part of the code to aid that, putting JSON in a code comment is too much!

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2018

To see the fallback to GUI code in use play a non-library music file via JSON

[{"jsonrpc": "2.0", "id": 0, "method": "Playlist.Clear", "params": {"playlistid": 0}},
{"jsonrpc":"2.0","id":0,"method":"Playlist.Add","params":{"playlistid":0,
"item":{"file":"C:/MusicTest/Dragon/01 - This Is Berk.flac"}}},
{"jsonrpc":"2.0","id":0,"method":"Player.Open","params":{"item":{"playlistid":0,"position":0}}}]

Then once playback has started request item details

{"jsonrpc": "2.0", "id": 1, "method": "Player.GetItem", 
"params": { "playerid":0, 
"properties": ["album", "albumartist", "artist", "displayartist", "episode", "art", "file", "genre", 
"plot","rating","season","showtitle","studio","imdbnumber","tagline","title","track","year","streamdetails",
"originaltitle","playcount","runtime","duration","cast","writer","director","userrating","firstaired"]}}

In this case the GUI item has more item data than the app current item does, but depending on the GUI like this is undesirable. It is important that the item is not in the library. There may be other occurances of this situation e.g. if an addon modifies item data during playback. Such could happen with playback of internet radio - stream path does not change, but song title or programme needs to update.

Note this PR has not changed this behaviour, just fixed the seg fault.

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Sep 25, 2018

jenkins build this please

@DaveTBlake DaveTBlake merged commit 5ff20e6 into xbmc:master Sep 25, 2018

1 check passed

default You're awesome. Have a cookie
Details

@DaveTBlake DaveTBlake deleted the DaveTBlake:JSONPlayerGetItemFix branch Sep 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.