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

[imdb] fix crash on empty videoInfoTag #12373

Merged
merged 1 commit into from Jul 29, 2017

Conversation

@lobermann
Copy link
Member

commented Jun 26, 2017

Scraping with an invalid api key caused a crash after trying to scrape a few times.

Description

When cleaning my database to re-scrape everything, I still had the old tmdb addon running, as there was an issue with the update. This caused kodi to crash after trying to scan for content a few times. As I catched the crash in the debugger I added some checks, as it seems in that case the videoInfoTag is not assigned. This should just prevent the crash when it tries to access the info tag.

Motivation and Context

This prevents the crash I catched in the debugger.

How Has This Been Tested?

I re-tested scraping a few times with the invalid key and it was not crashing any more. Noticed no issues.

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed
@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Jun 26, 2017

backport perhaps?

@@ -306,6 +306,11 @@ void CGUIDialogVideoInfo::SetMovie(const CFileItem *item)

// setup cast list
ClearCastList();

// When the scraper throws an error, the video tag can be null here
if (item->GetVideoInfoTag() == nullptr)

This comment has been minimized.

Copy link
@garbear

garbear Jun 26, 2017

Member

if (!item->HasVideoInfoTag())

This comment has been minimized.

Copy link
@lobermann

lobermann Jun 26, 2017

Author Member

nice, missed that one. Updating it now. Thanks.

@lobermann lobermann force-pushed the lobermann:imdb_scrape_crash branch from 5b0f125 to 1445537 Jun 26, 2017

@mkortstiege

This comment has been minimized.

Copy link
Member

commented Jun 27, 2017

Looks good to me. Backport won't hurt.

@mkortstiege

This comment has been minimized.

Copy link
Member

commented Jun 27, 2017

jenkins build this please

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Jun 27, 2017

jenkins build this please

@arnova

This comment has been minimized.

Copy link
Member

commented Jun 28, 2017

Don't we want the video dialog to appear after failure so the user can refresh like I did here?: #12385

@mkortstiege

This comment has been minimized.

Copy link
Member

commented Jun 28, 2017

Don't we want the video dialog to appear after failure so the user can refresh like I did here?: #12385

Unsure, but since this most likely only happens when there's a massive failure (eg. due to invalid api key) i don't think raising the info dialog would make sense here.

@arnova

This comment has been minimized.

Copy link
Member

commented Jun 28, 2017

@mkortstiege : I can't think of all the possible scenarious when this occurs. I'm just wondering whether still raising it could help as a temporary workaround for certain failures. Not sure though...

@mkortstiege

This comment has been minimized.

Copy link
Member

commented Jun 28, 2017

@arnova If we're going to re-open the info dialog it might look like we're done refreshing. I am unable to test it myself right now, but from a brief look there's no toast or error dialog involved.

@arnova

This comment has been minimized.

Copy link
Member

commented Jun 28, 2017

@mkortstiege : There are 2 error dialogs raised (see my comment in #12385), they are triggered by the RefreshItemModal-call in the refresh-loop.

@lobermann

This comment has been minimized.

Copy link
Member Author

commented Jun 30, 2017

I think asking for a reload does not make sense in this case, as it will fail again and then we still will not have a videoinfotag. But we could do a simple OK-Dialog/Notification saying there was an error scraping, just to inform the user.

@arnova

This comment has been minimized.

Copy link
Member

commented Jul 1, 2017

@lobermann : If you add an additional dialog you might as well move it down like I suggested as that will also give you an error dialog (followed by an empty info screen) which does the job just as well IMO. @mkortstiege : Your opinion please.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Jul 28, 2017

So what will it be so we can add it to v17. 4 as well

@arnova

This comment has been minimized.

Copy link
Member

commented Jul 29, 2017

@MartijnKaijser : Including this for 17.4 should be fine, although imo for master this could use some more thinking/refinement.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Jul 29, 2017

jenkins build this please

@MartijnKaijser MartijnKaijser added this to the L 18.0-alpha1 milestone Jul 29, 2017

@MartijnKaijser MartijnKaijser merged commit d933e1d into xbmc:master Jul 29, 2017

1 check failed

default Sorry, building this PR failed. Please check the logs for the errors.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.