Fixes a crash that can happen when playing a corrupted file

CHANGELOG.md

@@ -1,6 +1,7 @@
 7.41
 -----
 - Improve support section to help the user to send Apple Watch logs
+- Fixed a crash that could occur when playing some downloaded episodes (#901)
 
 7.40
 -----

PocketCastsTests/Tests/Analytics/AnalyticsCoordinatorTests.swift

@@ -0,0 +1,46 @@
+import XCTest
+
+@testable import podcasts
+
+final class AnalyticsCoordinatorTests: XCTestCase {
+    func testFallsBackToPreviousSourceOnNilCurrentSource() {
+        let coordinator = AnalyticsCoordinator()
+        coordinator.currentSource = .carPlay
+
+        // Resets the current source and sets the previous source
+        let previousSource = coordinator.currentAnalyticsSource
+
+        // The current source is expected to be nil, so this should reset to the original source value
+        coordinator.fallbackToPreviousSourceIfNeeded()
+
+        XCTAssertEqual(previousSource, coordinator.currentSource)
+    }
+
+    func testFallsBackToPreviousSourceOnUnknownCurrentSource() {
+        let coordinator = AnalyticsCoordinator()
+        coordinator.currentSource = .carPlay
+
+        // Resets the current source and sets the previous source
+        let _ = coordinator.currentAnalyticsSource
+        coordinator.currentSource = .unknown
+
+        // The current source is expected to be nil, so this should reset to the original source value
+        coordinator.fallbackToPreviousSourceIfNeeded()
+
+        XCTAssertEqual(coordinator.currentSource, .carPlay)
+    }
+
+    func testFallbackDoesntHappenIfCurrentSourceIsSet() {
+        let coordinator = AnalyticsCoordinator()
+        coordinator.currentSource = .carPlay
+
+        // Resets the current source and sets the previous source
+        let _ = coordinator.currentAnalyticsSource
+        coordinator.currentSource = .chooseFolder
+
+        // The current source is set, this should do nothing
+        coordinator.fallbackToPreviousSourceIfNeeded()
+
+        XCTAssertEqual(coordinator.currentSource, .chooseFolder)
+    }
+}

podcasts.xcodeproj/project.pbxproj

@@ -1480,6 +1480,7 @@
 		C7C4CAEE28AB0BF200CFC8CF /* TracksSubscriptionData.swift in Sources */ = {isa = PBXBuildFile; fileRef = C7C4CAED28AB0BF200CFC8CF /* TracksSubscriptionData.swift */; };
 		C7C4CAF328ABFD0900CFC8CF /* Notifications.swift in Sources */ = {isa = PBXBuildFile; fileRef = C7C4CAF228ABFD0900CFC8CF /* Notifications.swift */; };
 		C7CA0559293E8918000E41BD /* HolographicEffect.swift in Sources */ = {isa = PBXBuildFile; fileRef = C7CA0558293E8918000E41BD /* HolographicEffect.swift */; };
+		C7CDE14E2A4B41BF0081E7FF /* AnalyticsCoordinatorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C7CDE14D2A4B41BF0081E7FF /* AnalyticsCoordinatorTests.swift */; };
 		C7CE415A28CBCFC200AD063E /* Analytics.swift in Sources */ = {isa = PBXBuildFile; fileRef = C7AF5790289D87CF0089E435 /* Analytics.swift */; };
 		C7CE415B28CBD00A00AD063E /* AnalyticsEvent.swift in Sources */ = {isa = PBXBuildFile; fileRef = C72CED2C289DA14F0017883A /* AnalyticsEvent.swift */; };
 		C7CE415C28CBD01F00AD063E /* String+Analytics.swift in Sources */ = {isa = PBXBuildFile; fileRef = C72CED2E289DA1650017883A /* String+Analytics.swift */; };
@@ -3166,6 +3167,7 @@
 		C7C4CAED28AB0BF200CFC8CF /* TracksSubscriptionData.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TracksSubscriptionData.swift; sourceTree = "<group>"; };
 		C7C4CAF228ABFD0900CFC8CF /* Notifications.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Notifications.swift; sourceTree = "<group>"; };
 		C7CA0558293E8918000E41BD /* HolographicEffect.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = HolographicEffect.swift; sourceTree = "<group>"; };
+		C7CDE14D2A4B41BF0081E7FF /* AnalyticsCoordinatorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AnalyticsCoordinatorTests.swift; sourceTree = "<group>"; };
 		C7D6551328E5153200AD7174 /* Debounce.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Debounce.swift; sourceTree = "<group>"; };
 		C7D813782A0D4B89007F715F /* [email protected] */ = {isa = PBXFileReference; lastKnownFileType = image.png; path = "[email protected]"; sourceTree = "<group>"; };
 		C7D813792A0D4B89007F715F /* AppIcon-Patron-Chrome-ipad-pro.png */ = {isa = PBXFileReference; lastKnownFileType = image.png; path = "AppIcon-Patron-Chrome-ipad-pro.png"; sourceTree = "<group>"; };
@@ -6797,6 +6799,7 @@
 			children = (
 				C7D854F228ADD98700877E87 /* AppLifecyleAnalyticsTests.swift */,
 				8BA55A0F28CA6843002BECC5 /* AnalyticsPlaybackHelperTests.swift */,
+				C7CDE14D2A4B41BF0081E7FF /* AnalyticsCoordinatorTests.swift */,
 			);
 			path = Analytics;
 			sourceTree = "<group>";
@@ -8115,6 +8118,7 @@
 			buildActionMask = 2147483647;
 			files = (
 				8B317BA528906CAB00A26A13 /* TestingSceneDelegate.swift in Sources */,
+				C7CDE14E2A4B41BF0081E7FF /* AnalyticsCoordinatorTests.swift in Sources */,
 				8B2319432902CF170001C3DE /* EndOfYearManagerMock.swift in Sources */,
 				8B68C1D829421E3400CF25C5 /* FeatureFlagTests.swift in Sources */,
 				8B317BA328906C8100A26A13 /* TestingAppDelegate.swift in Sources */,

podcasts/Analytics/AnalyticsDescribable+Modules.swift

@@ -183,3 +183,24 @@ extension SocialAuthProvider: AnalyticsDescribable {
         }
     }
 }
+
+// MARK: - Players
+extension DefaultPlayer: AnalyticsDescribable {
+    var analyticsDescription: String {
+       "default"
+    }
+}
+
+#if !os(watchOS)
+extension EffectsPlayer: AnalyticsDescribable {
+    var analyticsDescription: String {
+       "effects"
+    }
+}
+
+extension GoogleCastPlayer: AnalyticsDescribable {
+    var analyticsDescription: String {
+       "google_cast"
+    }
+}
+#endif

podcasts/Analytics/AnalyticsEvent.swift

@@ -215,6 +215,7 @@ enum AnalyticsEvent: String {
 
     case playbackPlay
     case playbackPause
+    case playbackFailed
     case playbackSkipBack
     case playbackSkipForward
     case playbackSeek

podcasts/Analytics/Helpers/AnalyticsCoordinator.swift

@@ -41,7 +41,6 @@ enum AnalyticsSource: String, AnalyticsDescribable {
     case upNext = "up_next"
     case userEpisode = "user_episode"
     case videoPlayerSkipForwardLongPress = "video_player_skip_forward_long_press"
-    case playbackFailed = "playback_failed"
     case watch
     case unknown
 
@@ -52,9 +51,22 @@ class AnalyticsCoordinator {
     /// Sometimes the playback source can't be inferred, just inform it here
     var currentSource: AnalyticsSource?
 
+    /// Keep track of the analytics source that was used for the last event before it was reset
+    var previousSource: AnalyticsSource?
+
+    /// If the current source is not available, attempt to fallback to previous value before it was reset
+    func fallbackToPreviousSourceIfNeeded() {
+        guard currentSource == nil || currentSource == .unknown else {
+            return
+        }
+
+        currentSource = previousSource
+    }
+
     #if !os(watchOS)
         var currentAnalyticsSource: AnalyticsSource {
-            if let currentSource = currentSource {
+            if let currentSource {
+                previousSource = currentSource
                 self.currentSource = nil
                 return currentSource
             }

podcasts/Analytics/Helpers/AnalyticsPlaybackHelper.swift

@@ -25,6 +25,10 @@ class AnalyticsPlaybackHelper: AnalyticsCoordinator {
         track(.playbackSkipForward)
     }
 
+    func playbackFailed(errorMessage: String, episodeUuid: String, player: PlaybackProtocol?) {
+        track(.playbackFailed, properties: [ "error": errorMessage, "episode_uuid": episodeUuid, "player": player ?? "unknown" ])
+    }
+
     func seek(from: TimeInterval, to: TimeInterval, duration: TimeInterval) {
         // Currently ignore a seek event that is triggered by a sync process
         // Using the skip buttons triggers a seek, ignore this as well

podcasts/AudioReadTask.swift

@@ -372,6 +372,11 @@ class AudioReadTask {
         let totalSeconds = totalFrames / audioFile.fileFormat.sampleRate
         let percentSeek = time / totalSeconds
 
+        // Ignore any invalid values
+        guard percentSeek.isNumeric else {
+            return(0, false)
+        }
+
         return (Int64(totalFrames * percentSeek), percentSeek >= 1)
     }
 }

podcasts/DefaultPlayer.swift

@@ -228,7 +228,10 @@ class DefaultPlayer: PlaybackProtocol, Hashable {
 
     private func playerStatusDidChange() {
         if player?.currentItem?.status == .failed {
-            PlaybackManager.shared.playbackDidFail(logMessage: "AVPlayerItemStatusFailed on currentItem", userMessage: nil)
+            // Attempt to provide more specific information about the error if its available
+            // This only returns the domain and code to help normalize it across different languages
+            let message = (player?.currentItem?.error as? NSError).map { "Domain: \($0.domain) - Code: \($0.code)"}
+            PlaybackManager.shared.playbackDidFail(logMessage: message ?? "AVPlayerItemStatusFailed on currentItem", userMessage: nil)
 
             return
         }

podcasts/EffectsPlayer.swift

@@ -98,6 +98,15 @@ 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)
+
+                    // Throw an error if the frame count is 0
+                    // `cachedFrameCount` is used to calculate the duration of the episode and a 0 value
+                    // will result in the episode immediately being marked as played or can cause Inf/NaN issues.
+                    //
+                    // For more info, see: https://github.com/Automattic/pocket-casts-ios/issues/900
+                    if strongSelf.cachedFrameCount == 0 {
+                        throw PlaybackError.effectsPlayerFrameCountZero
+                    }
                 }
             } catch {
                 objc_sync_exit(strongSelf.playerLock)

podcasts/PlaybackManager.swift

@@ -814,7 +814,10 @@ class PlaybackManager: ServerPlaybackDelegate {
 
     func playbackDidFail(logMessage: String?, userMessage: String?) {
         FileLog.shared.addMessage("playbackDidFail: \(logMessage ?? "No error provided")")
-        AnalyticsPlaybackHelper.shared.currentSource = .playbackFailed
+
+        // If there is no current analytics source, then use the previous one
+        // This helps prevent an `unknown` from being used if this is called right after another event, such as playbackPlay
+        analyticsPlaybackHelper.fallbackToPreviousSourceIfNeeded()
 
         guard let episode = currentEpisode() else {
             endPlayback()
@@ -828,7 +831,11 @@ class PlaybackManager: ServerPlaybackDelegate {
         // - Is the duration actually reasonable?
         // if either of these is false, flag it as an error, otherwise we got close enough to the end
         if episode.playedUpTo < 1.minutes || episode.duration <= 0 || ((episode.playedUpTo + 3.minutes) < episode.duration) {
-            pause()
+            analyticsPlaybackHelper.playbackFailed(errorMessage: logMessage ?? "Unknown",
+                                                   episodeUuid: episode.uuid,
+                                                   player: player)
+
+            pause(userInitiated: false)
             NotificationCenter.postOnMainThread(notification: Constants.Notifications.playbackPaused)
 
             if episode.downloaded(pathFinder: DownloadManager.shared) {
@@ -841,7 +848,6 @@ class PlaybackManager: ServerPlaybackDelegate {
             }
 
             NotificationCenter.postOnMainThread(notification: Constants.Notifications.playbackFailed)
-
             return
         }
 

podcasts/PlaybackProtocol.swift

@@ -33,7 +33,19 @@ import PocketCastsDataModel
     func internalPlayerForVideoPlayback() -> AVPlayer?
 }
 
-enum PlaybackError: Error {
+enum PlaybackError: LocalizedError {
     case unableToOpenFile
+    case effectsPlayerFrameCountZero
     case errorDuringPlayback
+
+    var errorDescription: String? {
+        switch self {
+        case .unableToOpenFile:
+            return "PlaybackError: unableToOpenFile"
+        case .effectsPlayerFrameCountZero:
+            return "EffectsPlayer frameCount was 0 while opening the file"
+        case .errorDuringPlayback:
+            return "PlaybackError: errorDuringPlayback"
+        }
+    }
 }