pocket-casts-ios icon indicating copy to clipboard operation
pocket-casts-ios copied to clipboard

Fixes a crash that can happen when playing a corrupted file

Open emilylaguna opened this issue 1 year ago • 3 comments

Fixes #898

This fixes a crash that can happen if the EffectsPlayer tries to play a corrupted episode and adds error handling to the EffectsPlayer.

What's the crash

The specific crash we get is:

Fatal error: Double value cannot be converted to Int64 because it is either infinite or NaN

This can happen if:

  • The file is being played with the EffectsPlayer (either by being downloaded or otherwise)
  • The file is unable to be read by the player but doesn't throw an error and results in a 0 length file
  • The player tries to skip forward when its being created
    • some reasons could be:
      • Because there's playback progress
      • the podcast is set to skip forward
  • The framePositionForTime calculation results in 123 / 0 = +Inf

What's the fix?

This PR applies an isNumeric check on the percent seeking value which checks for inf/nan and returns false for either of them.

This results in the Inf value being ignored, and a default value being returned.

This also adds a check to verify the frameCount value is not 0 after loading it, and will throw an error if that happens.

⚠️ This only fixes the crash, not the underlaying issue of a corrupt file not being handled correctly by the EffectsPlayer

I want to note that this only fixes this crash from happening and not the reason why a file would get to this state. We'll need to investigate this further, I have made an issue for it here: https://github.com/Automattic/pocket-casts-ios/issues/900

To test

  1. Locate the following episode: https://pca.st/22iph0yt
  2. Start streaming it
  3. While it's streaming, download it
  4. Wait until the download is complete
  5. ✅ Verify the episode stops playing and the app doesn't crash

Checklist

  • [x] I have considered if this change warrants user-facing release notes and have added them to CHANGELOG.md if necessary.
  • [x] I have considered adding unit tests for my changes.
  • [x] I have updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics.

emilylaguna avatar Jun 12 '23 21:06 emilylaguna

@emilylaguna, I tested and I don't get the crash anymore, but I have a few of questions:

  1. The episode does stop playing when download is complete, but it also gets archived. Is that expected?
  2. Is the episode stopping the desired behavior or is it just a side affect of a temporary fix? its hard to know from the description.
  3. I understand we want to fix the crash first, but if the behaviors are not expected and just temporary. should we give the issue you opened high priority? If so, can we label it as such?

yaelirub avatar Jun 12 '23 23:06 yaelirub

The episode does stop playing when download is complete, but it also gets archived. Is that expected?

That is expected when the Auto Archive when played setting is enabled. This is because the player treats it as a completed episode instead of an error.

While looking into this I did try an idea that would check the cachedFrameCount value after reading the length and if it was still 0 it would throwing an error and result in showing a "this episode might be corrupted" error.

I didn't use this because adding that check changes how the player works for everyone and could cause more issues if for some reason there's a state where having a 0 length is okay. While I don't think that's likely based on my current knowledge, I am not confident enough to commit to it without more investigation and testing.

Is the episode stopping the desired behavior or is it just a side affect of a temporary fix? its hard to know from the description.

The episode stopping or being marked as played is not the desired behavior, ideally the episode should play correctly as it does when its being streamed.

However once the episode is downloaded the EffectsPlayer will not play it, nor will forcing the player to use the DefaultPlayer, which is what is used when the episode is being streamed.

The default player does throw an error and says the episode is damaged:

Error Domain=AVFoundationErrorDomain Code=-11829 "Cannot Open" UserInfo={NSLocalizedFailureReason=This media may be damaged., NSLocalizedDescription=Cannot Open, NSUnderlyingError=0x28357e610 {Error Domain=NSOSStatusErrorDomain Code=-12848 "(null)"}}

I understand we want to fix the crash first, but if the behaviors are not expected and just temporary. should we give the https://github.com/Automattic/pocket-casts-ios/issues/900 you opened high priority? If so, can we label it as such?

Looking at the issue on Sentry, this crash has only occurred for 7 users and only from 1 or 2 episodes (I was only able to see 1 episode from the breadcrumbs, but since this first appeared before that episode was even published I'm assuming there is at least 1 other episode with this issue). Because of the low counts I don't think its necessary to mark this as high priority.

emilylaguna avatar Jun 13 '23 17:06 emilylaguna

I pushed a change that will throw an error if the EffectsPlayer fails due to a 0 frameCount.

I also pushed 2 concept branches that convert the AAC m4a files to AAC ones, and adds fallback handling if a player fails to play:

  • https://github.com/Automattic/pocket-casts-ios/compare/fix/player-crash...fix/aac-to-m4a-files?expand=1
  • https://github.com/Automattic/pocket-casts-ios/compare/fix/aac-to-m4a-files...task/add-player-fallback?expand=1

emilylaguna avatar Jun 17 '23 02:06 emilylaguna