pocket-casts-ios
pocket-casts-ios copied to clipboard
Fixes a crash that can happen when playing a corrupted file
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
- some reasons could be:
- The
framePositionForTime
calculation results in123 / 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
- Locate the following episode: https://pca.st/22iph0yt
- Start streaming it
- While it's streaming, download it
- Wait until the download is complete
- ✅ 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, I tested and I don't get the crash anymore, but I have a few of questions:
- The episode does stop playing when download is complete, but it also gets archived. Is that expected?
- 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.
- 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?
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.
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