Fix crash in mpv_set_property during termination, #11

This was also reported against IINA in issue iina#3596

The commit in the pull request will:
- Rename the PlayerCore property isMpvTerminated to isMpvTerminating to
  reflect that termination has been initiated and being processed
  asynchronously
- Change existing references to  isMpvTerminated  to use new name
- Change PlayerCore.terminateMPV to set isMpvTerminating to true right
  before starting the termination process
- Change PlayerCore.syncUI to do nothing if mpv is terminating
- Change MPVController.mpvQuit to remove observers MPVController
  registered before sending the quit command to mpv
- Change many MPVController methods to check to see if mpv is being
  terminated

iina/MPVController.swift

@@ -417,8 +417,27 @@ class MPVController: NSObject {
     return flags & UInt64(MPV_RENDER_UPDATE_FRAME.rawValue) > 0
   }
 
+  /// Remove registered observers for mpv properties and IINA preferences.
+  ///
+  /// This method is used when terminating mpv. Once the controller has initiated the shutdown of the mpv context we don't want to
+  /// continue using the context while mpv is asynchronously shutting it down.
+  private func removeObservers() {
+    // Remove observers for mpv properties. Because 0 was passed for reply_userdata when registering
+    // mpv property observers all observers can be removed in one call.
+    mpv_unobserve_property(mpv, 0)
+    // Remove observers for IINA preferences.
+    ObjcUtils.silenced {
+      self.optionObservers.forEach { (k, _) in
+        UserDefaults.standard.removeObserver(self, forKeyPath: k)
+      }
+    }
+  }
+
   // Basically send quit to mpv
   func mpvQuit() {
+    removeObservers()
+    // The quit command executes asynchronously. A MPV_EVENT_SHUTDOWN event is emitted when the quit
+    // command finishes.
     command(.quit)
   }
 
@@ -475,36 +494,43 @@ class MPVController: NSObject {
 
   // Set property
   func setFlag(_ name: String, _ flag: Bool) {
+    guard !player.isMpvTerminating else { return }
     var data: Int = flag ? 1 : 0
     mpv_set_property(mpv, name, MPV_FORMAT_FLAG, &data)
   }
 
   func setInt(_ name: String, _ value: Int) {
+    guard !player.isMpvTerminating else { return }
     var data = Int64(value)
     mpv_set_property(mpv, name, MPV_FORMAT_INT64, &data)
   }
 
   func setDouble(_ name: String, _ value: Double) {
+    guard !player.isMpvTerminating else { return }
     var data = value
     mpv_set_property(mpv, name, MPV_FORMAT_DOUBLE, &data)
   }
 
   func setFlagAsync(_ name: String, _ flag: Bool) {
+    guard !player.isMpvTerminating else { return }
     var data: Int = flag ? 1 : 0
     mpv_set_property_async(mpv, 0, name, MPV_FORMAT_FLAG, &data)
   }
 
   func setIntAsync(_ name: String, _ value: Int) {
+    guard !player.isMpvTerminating else { return }
     var data = Int64(value)
     mpv_set_property_async(mpv, 0, name, MPV_FORMAT_INT64, &data)
   }
 
   func setDoubleAsync(_ name: String, _ value: Double) {
+    guard !player.isMpvTerminating else { return }
     var data = value
     mpv_set_property_async(mpv, 0, name, MPV_FORMAT_DOUBLE, &data)
   }
 
   func setString(_ name: String, _ value: String) {
+    guard !player.isMpvTerminating else { return }
     mpv_set_property_string(mpv, name, value)
   }
 
@@ -627,7 +653,7 @@ class MPVController: NSObject {
 
     switch eventId {
     case MPV_EVENT_SHUTDOWN:
-      let quitByMPV = !player.isMpvTerminated
+      let quitByMPV = !player.isMpvTerminating
       if quitByMPV {
         DispatchQueue.main.sync {
           NSApp.terminate(nil)
@@ -768,7 +794,7 @@ class MPVController: NSObject {
 
   private func onVideoReconfig() {
     // If loading file, video reconfig can return 0 width and height
-    if player.info.fileLoading {
+    if player.info.fileLoading, player.isMpvTerminating {
       return
     }
     var dwidth = getInt(MPVProperty.dwidth)
@@ -791,6 +817,10 @@ class MPVController: NSObject {
   // MARK: - Property listeners
 
   private func handlePropertyChange(_ name: String, _ property: mpv_event_property) {
+    // Once mpv starts terminating we no longer care about property changes and especially do not
+    // want to process changes that could trigger calls to mpv. Observers are being deregistered so
+    // there is only a brief window where this might trigger.
+    guard !player.isMpvTerminating else { return }
 
     var needReloadQuickSettingsView = false
 

iina/MainWindowController.swift

@@ -1110,7 +1110,7 @@ class MainWindowController: PlayerWindowController {
       }
     }
     // stop playing
-    if !player.isMpvTerminated {
+    if !player.isMpvTerminating {
       if case .fullscreen(legacy: true, priorWindowedFrame: _) = fsState {
         restoreDockSettings()
       }

iina/MiniPlayerWindowController.swift

@@ -186,7 +186,7 @@ class MiniPlayerWindowController: PlayerWindowController, NSPopoverDelegate {
   func windowWillClose(_ notification: Notification) {
     player.switchedToMiniPlayerManually = false
     player.switchedBackFromMiniPlayerManually = false
-    if !player.isMpvTerminated {
+    if !player.isMpvTerminating {
       // not needed if called when terminating the whole app
       player.switchBackFromMiniPlayer(automatically: true, showMainWindow: false)
     }

iina/PlayerCore.swift

@@ -131,7 +131,7 @@ class PlayerCore: NSObject {
 
   var displayOSD: Bool = true
 
-  var isMpvTerminated: Bool = false
+  var isMpvTerminating: Bool = false
 
   var isInMiniPlayer = false
   var switchedToMiniPlayerManually = false
@@ -377,14 +377,14 @@ class PlayerCore: NSObject {
 
   // Terminate mpv
   func terminateMPV(sendQuit: Bool = true) {
-    guard !isMpvTerminated else { return }
+    guard !isMpvTerminating else { return }
+    isMpvTerminating = true
     savePlaybackPosition()
     invalidateTimer()
     uninitVideo()
     if sendQuit {
       mpv.mpvQuit()
     }
-    isMpvTerminated = true
   }
 
   /// Wait until this player core has been terminated.
@@ -1361,7 +1361,9 @@ class PlayerCore: NSObject {
 
   func syncUI(_ option: SyncUIOption) {
     // if window not loaded, ignore
-    guard mainWindow.loaded else { return }
+    // No need for updating the UI if currently terminating mpv. Requesting property values from mpv
+    // can cause crashes depending upon how far into the asynchronous termination sequence mpv is.
+    guard mainWindow.loaded, !isMpvTerminating else { return }
     Logger.log("Syncing UI \(option)", level: .verbose, subsystem: subsystem)
 
     switch option {