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

Fixes a crash that can happen when playing a corrupted file #901

Open
wants to merge 20 commits into
base: trunk
Choose a base branch
from

Conversation

emilylaguna
Copy link
Contributor

@emilylaguna emilylaguna commented Jun 12, 2023

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: #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

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

@emilylaguna emilylaguna added playback Issues related to playback crash Crash related issues labels Jun 12, 2023
@emilylaguna emilylaguna added this to the 7.41 milestone Jun 12, 2023
@emilylaguna emilylaguna requested a review from a team as a code owner June 12, 2023 21:14
@yaelirub
Copy link
Contributor

@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?

@emilylaguna
Copy link
Contributor Author

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 #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 emilylaguna modified the milestones: 7.41 ❄️, 7.42 Jun 17, 2023
@emilylaguna emilylaguna changed the title Add a isNumeric check to prevent a crash Fixes a crash that can happen when playing a corrupted file Jun 17, 2023
@emilylaguna
Copy link
Contributor Author

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:

@@ -98,6 +98,11 @@ class EffectsPlayer: PlaybackProtocol, Hashable {
// we haven't cached a frame count for this episode, do that now
strongSelf.cachedFrameCount = strongSelf.audioFile!.length
DataManager.sharedManager.saveFrameCount(episode: episode, frameCount: strongSelf.cachedFrameCount)

// If the count is still 0 then way may not be able to read the file correctly, so throw an error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be we instead of way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🫣 oops, fixed here: cf19c0e


// If the count is still 0 then way may not be able to read the file correctly, so throw an error
if strongSelf.cachedFrameCount == 0 {
throw PlaybackError.unableToOpenFile
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add tracking here to know how often this happens?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea! I did this in a separate PR since there were some changes that were a bit more involved: #918

@emilylaguna emilylaguna modified the milestones: 7.42 ❄️, 7.43 Jul 6, 2023
@emilylaguna emilylaguna modified the milestones: 7.43, Future Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crash Crash related issues playback Issues related to playback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash: App crashes during playback for specific episode
2 participants