Crash in mpv_set_property during termination

[low-batt]

System and IINA version:

• macOS 12.1
• IINA 1.2.0 + changes

Expected behavior:
IINA quits without crashing.

Actual behavior:
If IINA is running in full-screen mode it may crash when quitting.

This issue filed against IINA-Plus: CarterLi#11 reports a crash during shutdown. IINA-Plus already contains the changes in this IINA PR #3591 which cause IINA to properly wait for mpv to shutdown. This makes it more likely that existing issues with coordination of threads in the shutdown process will be exposed. Therefore this same crash may be possible in IINA, just currently less likely.

The full crash report is available in the IINA-Plus issue, so I have only included the top of the stack off the crashing thread here.

Crash report:

Thread 0 Crashed::  Dispatch queue: com.apple.main-thread
0   libmpv.1.dylib                	       0x10553410c mpv_set_property + 28
1   IINA                          	       0x10269219c 0x10261c000 + 483740
2   IINA                          	       0x10269219c 0x10261c000 + 483740
3   IINA                          	       0x10269e490 0x10261c000 + 533648
4   IINA                          	       0x1026a16d0 0x10261c000 + 546512
5   CoreFoundation                	       0x1bdd45200 __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 28

Steps to reproduce:
This is an event / thread race condition, so very difficult to reproduce.

I have already reproduced the problem. I will post an analysis and proposed fix shortly.

• MPV does not have this problem.

This is an IINA problem.

How often does this happen?
If the fix to wait for mpv to shutdown is applied it is not hard to reproduce.

[low-batt]

Analysis

This is a race condition during shutdown.

Failing Call Sequence

The failing call sequence is as follows:

The method MainWindowController.windowDidExitFullScreen calls finishAnimating on the fsState property:

func windowDidExitFullScreen(_ notification: Notification) {
	...
  videoViewConstraints.values.forEach { $0.constant = 0 }
  videoView.needsLayout = true
  videoView.layoutSubtreeIfNeeded()
  videoView.videoLayer.resume()

  fsState.finishAnimating()

The method finishAnimating in the enum FullScreenState sets self to .windowed:

enum FullScreenState: Equatable {

  mutating func finishAnimating() {
    switch self {
    case .windowed, .fullscreen: assertionFailure("something went wrong with the state of the world. One must be .animating to finishAnimating. Not \(self)")
    case .animating(let toFullScreen, let legacy, let frame):
      if toFullScreen {
        self = .fullscreen(legacy: legacy, priorWindowedFrame: frame)
      } else{
        self = .windowed

Changing the value of MainWindowController.fsState causes didSet to be called which then calls PlayerCore.mpv.setFlag:

var fsState: FullScreenState = .windowed {
  didSet {
    switch fsState {
    case .fullscreen: player.mpv.setFlag(MPVOption.Window.fullscreen, true)
    case .animating:  break
    case .windowed:   player.mpv.setFlag(MPVOption.Window.fullscreen, false)

The method MPVController.setFlag calls mpv_set_property:

func setFlag(_ name: String, _ flag: Bool) {
  var data: Int = flag ? 1 : 0
  mpv_set_property(mpv, name, MPV_FORMAT_FLAG, &data)
}

The mpv method mpv_set_property then triggers a segmentation fault because the mpv context setFlag passed in the above call was nil. See the next section for why.

Failing Event Sequence

The failing event sequence is as follows:

• The method AppDelegate.applicationShouldTerminate calls PlayerCore.terminateMPV which starts an asynchronous termination of mpv
• The method MPVController.handleEvent receives the mpv event MPV_EVENT_SHUTDOWN and destroys the mpv context
• The method MainWindowController.windowDidExitFullScreen is called resulting in the above call sequence that attempts to use the destroyed context

Whether a crash occurs or not depends upon when the framework calls MainWindowController.windowDidExitFullScreen.

IINA needs to coordinate mpv termination such that once the quit command has been sent to mpv IINA no longer attempts to use the mpv context for anything other than completing the termination.

Layering Violations

From code reading it seemed like PlayerCore should be in charge of coordinating mpv termination. The class already contains the property isMpvTerminated and CONTRIBUTING specifies this requirement:

• PlayerCore encapsulates general playback functions.
  • Setting options/properties directly through MPVController is discouraged.

But it seems like PlayerCore is being by-passed in a lot of classes. I cut down the output to just show some of the violations:

low-batt@gag iina (develop $=)$ grep -inr --include \*.swift '\.mpv\.' *
iina/InspectorWindowController.swift:246:      return PlayerCore.active.mpv.getString(property) ?? "<Error>"
iina/MainWindowController.swift:101:    return player.mpv.getInt(MPVProperty.videoParamsRotate)
iina/MainMenuActions.swift:52:      let index = player.mpv.getInt(MPVProperty.playlistPos)
iina/PlaylistViewController.swift:140:    let loopStatus = player.mpv.getString(MPVOption.PlaybackControl.loopPlaylist)
iina/QuickSettingViewController.swift:213:    let speed = player.mpv.getDouble(MPVOption.PlaybackControl.speed)
iina/AppDelegate.swift:399:        player.mpv.setFlag(MPVOption.Window.fullscreen, true)
iina/MenuController.swift:440:    let isLoop = player.mpv.getFlag(MPVOption.PlaybackControl.loopFile)
iina/PlayerWindowController.swift:23:      player.mpv.setFlag(MPVOption.Window.ontop, isOntop)
iina/AutoFileMatcher.swift:125:        let count = player.mpv.getInt(MPVProperty.playlistCount)

This suggests MPVController needs to be more involved when mpv is being terminated.

Fixing

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

[low-batt]

The fix in PR #3961 closes windows before shutting down mpv. That should prevent this crash.

[low-batt]

This is fixed in the released version of IINA.