[mythtv] Ticket #12709: Segmentation fault playing a recording with file missing

[SIDDONS ROGER]

> 
>     On 07 April 2016 at 02:22 MythTV <noreply at mythtv.org> wrote:
> 
> 
>     #12709: Segmentation fault playing a recording with file missing
>     -----------------------------------------+-----------------------------
>     Reporter: Peter Bennett <pgbennett@…> | Owner: jyavenard
>     Type: Bug Report - Crash | Status: new
>     Priority: minor | Milestone: 0.28
>     Component: MythTV - Video Playback | Version: Master Head
>     Severity: medium | Resolution:
>     Keywords: | Ticket locked: 0
>     -----------------------------------------+-----------------------------
> 
>     Comment (by knight):
> 
>     Hi Roger!
> 
>     Replying to [comment:2 rsiddons]:
>     > The video player (mythfrontend::main::internal_play_media()) plays video
>     from any source. For instance Gallery uses it for camera videos and
>     !MythBrowser and !SetupWizard use it for downloaded videos. So it has to
>     fail in a generic/independent way.
>     >
>     > "Try it and handle failure" is a more robust strategy than "Check first,
>     do it and handle unexpected failure" (file disappears between check and
>     play ?). It also avoids having to retrieve a remote file twice.
> 
>     Assuming I correctly understood what you said I do not fully agree with
>     this...
> 
>     The null pointer check should be there regardless but it sounds to me like
>     it is a null pointer because the file is missing so we already know before
>     we reach that point that something is wrong.
> 
>     Why should we rely on a side effect of protecting our code against coding
>     errors and the like to handle something which should be handle long before
>     starting to initialize the UI.
> 
>     I have not tried Peter's patch but unless you tell me that with it in
>     place the user is told what is actually wrong I do not consider that only
>     handling the null pointer at this point is a good solution. From a
>     programmer standpoint we "handled" the problem and did not crash but it
>     would not be very user friendly.
> 
>     If we want it to fail in a generic/independent way maybe we should
>     actually remove all the code that does file handling from all of this
>     function/method client's and (Gallery, MythBrowser, etc...), make it
>     common and make it handle all of the possible failures this should
>     encounter.
> 
>     Yes, we should protect our code against using null pointers but this
>     should not be the only thing we do to handle those kind of errors...
> 
>     I am very far from a C++ guru but in the languages I do program in I both
>     program my code against using null pointers (even if it supposed to have
>     been taken care of earlier) and try to catch the error as soon as we know
>     it exists...
> 
>     We are way too close to release to do little more than what Peter put in
>     place however so this solution so Peter's current forthcoming solution
>     will have to do...
> 

The offending commit is only in Master. It doesn't affect RC-1 or fixes/0.28.

The NULL indicates that the player didn't start for some undefined reason. A
User Notification is raised separately with the specific reason.

However, looking closer, your/Peter's original question is good.
HandleStateChange initialises the UI at the end even though it already knows the
player has failed.

https://code.mythtv.org/trac/browser/mythtv/mythtv/libs/libmythtv/tv_play.cpp?rev=5ca0ddeab014daaaa2d7e8eeb5bbf6b095202a86#L2595

As a quick test, I ignored this block when "ok == false" and it worked fine: it
failed a lot quicker without all the flashing. So IMO that's something worth
pursuing. Maybe someone who's familiar with this code will comment ?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mythtv.org/pipermail/mythtv-dev/attachments/20160407/33514e2d/attachment.html>