Fix crash in PlayerCore.fileStarted, #3822

This commit will: - Add Atomic to PlaybackInfo.matchedSubs - Add a getMatchedSubs method to PlaybackInfo - Change AutoFileMatcher, PlayerCore and PlaylistViewController to hold a lock while accessing matchedSubs - Add Atomic to PlayerCore.backgroundQueueTicket - Change PlayerCore.fileStarted to hold a lock while incrementing backgroundQueueTicket - Add code to PlayerCore.stop that increments backgroundQueueTicket These changes insure the mutable Swift dictionary matchedSubs is only accessed by one thread at a time. Accessing a mutable dictionary from multiple threads without such coordination can result in a crash as seen in issue #3822. These changes also attempt to insure the background task that finds and loads subtitle files stops its work if playback is stopped before that process completes.
iina · Apr 9, 2023 · acac1c1 · acac1c1
1 parent f6f0eb7
commit acac1c1
Show file tree

Hide file tree

Showing 4 changed files with 34 additions and 27 deletions.
diff --git a/iina/AutoFileMatcher.swift b/iina/AutoFileMatcher.swift
@@ -213,7 +213,7 @@ class AutoFileMatcher {
               video.relatedSubs.append(sub)
               if sub.prefix == matchedSubPrefix {
                 try checkTicket()
-                player.info.matchedSubs[video.path, default: []].append(sub.url)
+                player.info.$matchedSubs.withLock { $0[video.path, default: []].append(sub.url) }
                 sub.isMatched = true
                 matchedSubs.insert(sub)
               }
@@ -231,7 +231,7 @@ class AutoFileMatcher {
         }.forEach { sub in
           try checkTicket()
           Logger.log("Matched \(sub.filename) and \(video.filename)", level: .verbose, subsystem: subsystem)
-          player.info.matchedSubs[video.path, default: []].append(sub.url)
+          player.info.$matchedSubs.withLock { $0[video.path, default: []].append(sub.url) }
           sub.isMatched = true
           matchedSubs.insert(sub)
         }
@@ -261,14 +261,16 @@ class AutoFileMatcher {
             minOccurrences = sub.priorityStringOccurrences
           }
         }
-        try matchedSubs
-          .filter { $0.priorityStringOccurrences > minOccurrences }  // eliminate false positives in filenames
-          .compactMap { player.info.matchedSubs[video.path]!.firstIndex(of: $0.url) }  // get index
-          .forEach {  // move the sub with index to first
-            try checkTicket()
-            Logger.log("Move \(player.info.matchedSubs[video.path]![$0]) to front", level: .verbose, subsystem: subsystem)
-            if let s = player.info.matchedSubs[video.path]?.remove(at: $0) {
-              player.info.matchedSubs[video.path]!.insert(s, at: 0)
+        try player.info.$matchedSubs.withLock { subs in
+          try matchedSubs
+            .filter { $0.priorityStringOccurrences > minOccurrences }  // eliminate false positives in filenames
+            .compactMap { subs[video.path]!.firstIndex(of: $0.url) }   // get index
+            .forEach { // move the sub with index to first
+              try checkTicket()
+              Logger.log("Move \(subs[video.path]![$0]) to front", level: .verbose, subsystem: subsystem)
+              if let s = subs[video.path]?.remove(at: $0) {
+                subs[video.path]!.insert(s, at: 0)
+              }
             }
         }
         Logger.log("Finished", level: .verbose, subsystem: subsystem)
@@ -314,7 +316,9 @@ class AutoFileMatcher {
         try checkTicket()
         unmatchedSubs
           .filter { video.dist[$0]! == minDistToSub && $0.minDist.contains(video) }
-          .forEach { player.info.matchedSubs[video.path, default: []].append($0.url) }
+          .forEach { sub in
+            player.info.$matchedSubs.withLock { $0[video.path, default: []].append(sub.url) }
+          }
       }
     }
   }
@@ -368,5 +372,4 @@ class AutoFileMatcher {
       return
     }
   }
-
 }
diff --git a/iina/PlaybackInfo.swift b/iina/PlaybackInfo.swift
@@ -185,7 +185,10 @@ class PlaybackInfo {
   var chapters: [MPVChapter] = []
   var chapter = 0
 
-  var matchedSubs: [String: [URL]] = [:]
+  @Atomic var matchedSubs: [String: [URL]] = [:]
+
+  func getMatchedSubs(_ file: String) -> [URL]? { $matchedSubs.withLock { $0[file] } }
+
   var currentSubsInfo: [FileInfo] = []
   var currentVideosInfo: [FileInfo] = []
 

diff --git a/iina/PlayerCore.swift b/iina/PlayerCore.swift
@@ -118,7 +118,7 @@ class PlayerCore: NSObject {
 
    `autoLoadFilesInCurrentFolder(ticket:)`
    */
-  var backgroundQueueTicket = 0
+  @Atomic var backgroundQueueTicket = 0
 
   var mainWindow: MainWindowController!
   var initialWindow: InitialWindowController!
@@ -595,13 +595,17 @@ class PlayerCore: NSObject {
 
   /// Stop playback and unload the media.
   func stop() {
+    // If the user immediately closes the player window it is possible the background task may still
+    // be working to load subtitles. Invalidate the ticket to get that task to abandon the work.
+    $backgroundQueueTicket.withLock { $0 += 1 }
+
     savePlaybackPosition()
 
     mainWindow.videoView.stopDisplayLink()
     invalidateTimer()
 
     info.currentFolder = nil
-    info.matchedSubs.removeAll()
+    info.$matchedSubs.withLock { $0.removeAll() }
 
     // Do not send a stop command to mpv if it is already stopped. This happens when quitting is
     // initiated directly through mpv.
@@ -1381,7 +1385,7 @@ class PlayerCore: NSObject {
     }
 
     // Auto load
-    backgroundQueueTicket += 1
+    $backgroundQueueTicket.withLock { $0 += 1 }
     let shouldAutoLoadFiles = info.shouldAutoLoadFiles
     let currentTicket = backgroundQueueTicket
     backgroundQueue.async {
@@ -1391,7 +1395,7 @@ class PlayerCore: NSObject {
         self.autoLoadFilesInCurrentFolder(ticket: currentTicket)
       }
       // auto load matched subtitles
-      if let matchedSubs = self.info.matchedSubs[path] {
+      if let matchedSubs = self.info.getMatchedSubs(path) {
         Logger.log("Found \(matchedSubs.count) subs for current file", subsystem: self.subsystem)
         for sub in matchedSubs {
           guard currentTicket == self.backgroundQueueTicket else { return }

diff --git a/iina/PlaylistViewController.swift b/iina/PlaylistViewController.swift
@@ -542,7 +542,7 @@ class PlaylistViewController: NSViewController, NSTableViewDataSource, NSTableVi
         }
         // sub button
         if !info.isMatchingSubtitles,
-          let matchedSubs = player.info.matchedSubs[item.filename], !matchedSubs.isEmpty {
+          let matchedSubs = player.info.getMatchedSubs(item.filename), !matchedSubs.isEmpty {
           cellView.setDisplaySubButton(true)
         } else {
           cellView.setDisplaySubButton(false)
@@ -679,7 +679,7 @@ class PlaylistViewController: NSViewController, NSTableViewDataSource, NSTableVi
     Utility.quickMultipleOpenPanel(title: NSLocalizedString("alert.choose_media_file.title", comment: "Choose Media File"), dir: fileURL, canChooseDir: true) { subURLs in
       for subURL in subURLs {
         guard Utility.supportedFileExt[.sub]!.contains(subURL.pathExtension.lowercased()) else { return }
-        self.player.info.matchedSubs[filename, default: []].append(subURL)
+        self.player.info.$matchedSubs.withLock { $0[filename, default: []].append(subURL) }
       }
       self.playlistTableView.reloadData(forRowIndexes: selectedRows, columnIndexes: IndexSet(integersIn: 0...1))
     }
@@ -689,10 +689,9 @@ class PlaylistViewController: NSViewController, NSTableViewDataSource, NSTableVi
     guard let selectedRows = selectedRows else { return }
     for index in selectedRows {
       let filename = player.info.playlist[index].filename
-      player.info.matchedSubs[filename]?.removeAll()
+      player.info.$matchedSubs.withLock { $0[filename]?.removeAll() }
       playlistTableView.reloadData(forRowIndexes: selectedRows, columnIndexes: IndexSet(integersIn: 0...1))
     }
-
   }
 
   @IBAction func contextOpenInBrowser(_ sender: NSMenuItem) {
@@ -721,7 +720,7 @@ class PlaylistViewController: NSViewController, NSTableViewDataSource, NSTableVi
 
     if !rows.isEmpty {
       let firstURL = player.info.playlist[rows.first!]
-      let matchedSubCount = player.info.matchedSubs[firstURL.filename]?.count ?? 0
+      let matchedSubCount = player.info.getMatchedSubs(firstURL.filename)?.count ?? 0
       let title: String = isSingleItem ?
         firstURL.filenameForDisplay :
         String(format: NSLocalizedString("pl_menu.title_multi", comment: "%d Items"), rows.count)
@@ -942,22 +941,21 @@ class SubPopoverViewController: NSViewController, NSTableViewDelegate, NSTableVi
   }
 
   func tableView(_ tableView: NSTableView, objectValueFor tableColumn: NSTableColumn?, row: Int) -> Any? {
-    guard let matchedSubs = player.info.matchedSubs[filePath] else { return nil }
+    guard let matchedSubs = player.info.getMatchedSubs(filePath) else { return nil }
     return matchedSubs[row].lastPathComponent
   }
 
   func numberOfRows(in tableView: NSTableView) -> Int {
-    return player.info.matchedSubs[filePath]?.count ?? 0
+    return player.info.getMatchedSubs(filePath)?.count ?? 0
   }
 
   @IBAction func wrongSubBtnAction(_ sender: AnyObject) {
-    player.info.matchedSubs[filePath]?.removeAll()
+    player.info.$matchedSubs.withLock { $0[filePath]?.removeAll() }
     tableView.reloadData()
     if let row = player.info.playlist.firstIndex(where: { $0.filename == filePath }) {
       playlistTableView.reloadData(forRowIndexes: IndexSet(integer: row), columnIndexes: IndexSet(integersIn: 0...1))
     }
   }
-
 }
 
 class ChapterTableCellView: NSTableCellView {
@@ -968,4 +966,3 @@ class ChapterTableCellView: NSTableCellView {
     textField?.toolTip = title
   }
 }
-