Merge pull request #1032 from TortugaPower/fix-stream-crash

Fix crash after streaming a partially corrupted file
TortugaPower · Oct 31, 2023 · bd332a7 · bd332a7
2 parents b75ba59 + 78127ef
commit bd332a7
Show file tree

Hide file tree

Showing 5 changed files with 59 additions and 66 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -18,9 +18,9 @@ jobs:
     - name: Swiftlint
       run: swiftlint
     - name: Set Xcode version
-      run: sudo xcode-select -s "/Applications/Xcode_15.0.app/Contents/Developer"
+      run: sudo xcode-select -s "/Applications/Xcode_15.0.1.app/Contents/Developer"
     - name: Resolve dependencies
       run: xcodebuild -resolvePackageDependencies
     - name: Build and Run tests
-      run: xcodebuild -scheme BookPlayer test -testPlan Unit\ Tests -destination 'platform=iOS Simulator,name=iPhone 15,OS=17.0'
+      run: xcodebuild -scheme BookPlayer test -testPlan Unit\ Tests -destination 'platform=iOS Simulator,name=iPhone 15,OS=17.0.1'
 
diff --git a/BookPlayer/Coordinators/LibraryListCoordinator.swift b/BookPlayer/Coordinators/LibraryListCoordinator.swift
@@ -214,12 +214,10 @@ class LibraryListCoordinator: ItemListCoordinator, UINavigationControllerDelegat
   override func syncList() {
     Task { @MainActor in
       do {
-        let lastPlayed: SyncableItem?
-
         if UserDefaults.standard.bool(forKey: Constants.UserDefaults.hasScheduledLibraryContents) == true {
-          lastPlayed = try await syncService.syncListContents(at: nil)
+          try await syncService.syncListContents(at: nil)
         } else {
-          lastPlayed = try await syncService.syncLibraryContents()
+          try await syncService.syncLibraryContents()
 
           UserDefaults.standard.set(
             true,
@@ -228,9 +226,6 @@ class LibraryListCoordinator: ItemListCoordinator, UINavigationControllerDelegat
         }
 
         reloadItemsWithPadding()
-        if let lastPlayed {
-          reloadLastBook(relativePath: lastPlayed.relativePath)
-        }
       } catch BPSyncError.reloadLastBook(let relativePath) {
         reloadItemsWithPadding()
         reloadLastBook(relativePath: relativePath)

diff --git a/BookPlayer/Generated/AutoMockable.generated.swift b/BookPlayer/Generated/AutoMockable.generated.swift
@@ -1380,20 +1380,15 @@ class SyncServiceProtocolMock: SyncServiceProtocol {
     }
     var syncListContentsAtReceivedRelativePath: String?
     var syncListContentsAtReceivedInvocations: [String?] = []
-    var syncListContentsAtReturnValue: SyncableItem?
-    var syncListContentsAtClosure: ((String?) async throws -> SyncableItem?)?
-    func syncListContents(at relativePath: String?) async throws -> SyncableItem? {
+    var syncListContentsAtClosure: ((String?) async throws -> Void)?
+    func syncListContents(at relativePath: String?) async throws {
         if let error = syncListContentsAtThrowableError {
             throw error
         }
         syncListContentsAtCallsCount += 1
         syncListContentsAtReceivedRelativePath = relativePath
         syncListContentsAtReceivedInvocations.append(relativePath)
-        if let syncListContentsAtClosure = syncListContentsAtClosure {
-            return try await syncListContentsAtClosure(relativePath)
-        } else {
-            return syncListContentsAtReturnValue
-        }
+        try await syncListContentsAtClosure?(relativePath)
     }
     //MARK: - syncLibraryContents
 
@@ -1402,18 +1397,13 @@ class SyncServiceProtocolMock: SyncServiceProtocol {
     var syncLibraryContentsCalled: Bool {
         return syncLibraryContentsCallsCount > 0
     }
-    var syncLibraryContentsReturnValue: SyncableItem?
-    var syncLibraryContentsClosure: (() async throws -> SyncableItem?)?
-    func syncLibraryContents() async throws -> SyncableItem? {
+    var syncLibraryContentsClosure: (() async throws -> Void)?
+    func syncLibraryContents() async throws {
         if let error = syncLibraryContentsThrowableError {
             throw error
         }
         syncLibraryContentsCallsCount += 1
-        if let syncLibraryContentsClosure = syncLibraryContentsClosure {
-            return try await syncLibraryContentsClosure()
-        } else {
-            return syncLibraryContentsReturnValue
-        }
+        try await syncLibraryContentsClosure?()
     }
     //MARK: - syncBookmarksList
 

diff --git a/BookPlayer/Player/PlayerManager.swift b/BookPlayer/Player/PlayerManager.swift
@@ -64,6 +64,8 @@ final class PlayerManager: NSObject, PlayerManagerProtocol {
   @Published private var playbackQueued: Bool?
   /// Flag determining if it's in the process of fetching the URL for playback
   @Published private var isFetchingRemoteURL: Bool?
+  /// Prevent loop from automatic URL refreshes
+  private var canFetchRemoteURL = true
   private var hasObserverRegistered = false
   private var observeStatus: Bool = false {
     didSet {
@@ -623,6 +625,9 @@ extension PlayerManager {
     /// Ignore play commands if there's no item loaded
     guard let currentItem else { return }
 
+    /// Allow refetching remote URL if the action was initiating by the user
+    canFetchRemoteURL = true
+
     guard let playerItem else {
       /// Check if the playbable item is in the process of being set
       if observeStatus == false {
@@ -724,37 +729,47 @@ extension PlayerManager {
   // swiftlint:disable block_based_kvo
   // Using this instead of new form, because the new one wouldn't work properly on AVPlayerItem
   override func observeValue(forKeyPath keyPath: String?, of object: Any?, change: [NSKeyValueChangeKey: Any]?, context: UnsafeMutableRawPointer?) {
-    guard let path = keyPath,
-          path == "status",
-          let item = object as? AVPlayerItem else {
-      super.observeValue(forKeyPath: keyPath,
-                         of: object,
-                         change: change,
-                         context: context)
-      return
-    }
-
-    guard item.status == .readyToPlay else {
-      if item.status == .failed {
-        if (item.error as? NSError)?.code == NSURLErrorResourceUnavailable,
-           let currentItem {
-          loadAndRefreshURL(item: currentItem)
-        } else {
-          playbackQueued = nil
-          observeStatus = false
-          showErrorAlert(title: "\("error_title".localized) AVPlayerItem", item.error?.localizedDescription)
-        }
-      }
+    guard 
+      let path = keyPath,
+      path == "status",
+      let item = object as? AVPlayerItem
+    else {
+      super.observeValue(
+        forKeyPath: keyPath,
+        of: object,
+        change: change,
+        context: context
+      )
       return
     }
 
-    self.observeStatus = false
+    switch item.status {
+    case .readyToPlay:
+      self.observeStatus = false
 
-    if self.playbackQueued == true {
-      self.play()
+      if self.playbackQueued == true {
+        self.play()
+      }
+      // Clean up flag
+      self.playbackQueued = nil
+    case .failed:
+      if canFetchRemoteURL,
+        (item.error as? NSError)?.code == NSURLErrorResourceUnavailable,
+        let currentItem {
+        loadAndRefreshURL(item: currentItem)
+        canFetchRemoteURL = false
+      } else {
+        playbackQueued = nil
+        observeStatus = false
+        playerItem = nil
+        showErrorAlert(title: "\("error_title".localized) AVPlayerItem", item.error?.localizedDescription)
+      }
+    case .unknown:
+      /// Do not handle .unknown states, as we're only interested in the success and failure states
+      fallthrough
+    @unknown default:
+      break
     }
-    // Clean up flag
-    self.playbackQueued = nil
   }
   // swiftlint:enable block_based_kvo
 

diff --git a/Shared/Services/Sync/SyncService.swift b/Shared/Services/Sync/SyncService.swift
@@ -32,11 +32,11 @@ public protocol SyncServiceProtocol {
   var downloadProgressPublisher: PassthroughSubject<(String, String, String?, Double), Never> { get }
 
   /// Fetch the contents at the relativePath and override local contents with the remote repsonse
-  func syncListContents(at relativePath: String?) async throws -> SyncableItem?
+  func syncListContents(at relativePath: String?) async throws
 
   /// Fetch the synced identifiers and upload new local items
   /// Note: Should only be called once when the user logs in
-  func syncLibraryContents() async throws -> SyncableItem?
+  func syncLibraryContents() async throws
 
   func syncBookmarksList(relativePath: String) async throws -> [SimpleBookmark]?
 
@@ -152,7 +152,7 @@ public final class SyncService: SyncServiceProtocol, BPLogger {
 
   public func syncListContents(
     at relativePath: String?
-  ) async throws -> SyncableItem? {
+  ) async throws {
     guard isActive else {
       throw BookPlayerError.networkError("Sync is not enabled")
     }
@@ -180,11 +180,9 @@ public final class SyncService: SyncServiceProtocol, BPLogger {
     let response = try await fetchContents(at: relativePath)
 
     try await processContentsResponse(response, parentFolder: relativePath, canDelete: true)
-
-    return response.lastItemPlayed
   }
 
-  public func syncLibraryContents() async throws -> SyncableItem? {
+  public func syncLibraryContents() async throws {
     guard
       isActive,
       UserDefaults.standard.bool(forKey: Constants.UserDefaults.hasQueuedJobs) == false
@@ -205,8 +203,6 @@ public final class SyncService: SyncServiceProtocol, BPLogger {
     let response = try await fetchContents(at: nil)
 
     try await processContentsResponse(response, parentFolder: nil, canDelete: false)
-
-    return response.lastItemPlayed
   }
 
   func processContentsResponse(
@@ -250,14 +246,11 @@ public final class SyncService: SyncServiceProtocol, BPLogger {
     }
 
     /// Only update the time if the remote last played timestamp is greater than the local timestamp
-    guard
-      let remoteLastPlayDateTimestamp = item.lastPlayDateTimestamp,
-      remoteLastPlayDateTimestamp > localLastPlayDateTimestamp
-    else {
-      return
+    if let remoteLastPlayDateTimestamp = item.lastPlayDateTimestamp,
+       remoteLastPlayDateTimestamp > localLastPlayDateTimestamp {
+      await libraryService.updateInfo(for: item)
+      throw BPSyncError.reloadLastBook(item.relativePath)
     }
-
-    await libraryService.updateInfo(for: item)
   }
 
   public func syncBookmarksList(relativePath: String) async throws -> [SimpleBookmark]? {