Hi, any updates on this PR?

Bumping the "any updates?" question - the crash after quitting is not a huge issue, but it sure is annoying.

@low-batt seems like uiryuu requested to have the guard statement before switch. IINA getting a crash report every time CMD + Q is quite annoying :(

Sorry, was away from IINA during the holidays. Special thanks to @danshao who in my absence addressed the review comment in another PR.

I've updated this PR because with further testing I found an additional check was needed.

Hopefully we can finish the review and get this fix merged soon.

@low-batt I took at look at the changes and I have a couple suggestions:

• If you haven't already, try doing a text search for all Swift files in the workspace for the string mpv. (no spaces). It should give you all the places where the MPVController is referenced, though with a few false positives. My search found 21 files, but eliminating the ones for mpv.mpvVersion, matches in comments, and plugin stuff (Javascript*), I counted 15 files containing potential problem points. Not terrible. But ideally there would be checks before all of these calls. Fortunately, probably over 2/3 of them require an explicit user action or request, which would be highly unlikely between the start of close to the finish of close.
• Consider moving isClosing from MainWindowController to its parent, PlayerWindowController. Even if you don't need to reference it right now, it will probably be needed there in the future because this issue can theoretically happen with all types of player windows.

If you haven't noticed already, you need to cover these cases too:

1. At the start of PlayerCore's syncUI(). This gets called several times a second, even when video is paused (hopefully that can be nailed down someday...) and looks like a prime failure point. There is already a guard mainWindow.loaded else { return } at its beginning which looks like a good place to add another check.
2. At the start of MainWindowController's showUI(). This is called by mouseEntered() which looks most dangerous, but it's called from other places too, and since a simple boolean check costs basically nothing in terms of clock cycles, adding a redundant check to prevent even a 0.001% chance of a crash is still definitely worth it.

OK after I wrote all of this, I noticed something else. Try looking through the code for checks for mainWindow.loaded. It looks like almost all its guard checks will also apply to isClosing! For example, I'm a little bit concerned about VideoView's refreshEdrMode() which is called asynchronously from all sorts of places, and saw that it is also making sure loaded is true. Again, when in doubt it's a better strategy to just add more checks than necessary, given that they're trivial.

The fully deterministic way to enforce the mpv requirement to not use the mpv context once mpv has been told to shutdown would be to enforce it in MPVController. This would require using thread locks similar to the protection that was added in Logger. To reduce the amount of locking it might be better to enforce it at the PlayerCore level and force most code to go through that class. Even if we did this MPVController methods that return values are a problem. If coded from scratch we might have made such methods optionally return values. Lots of code changes to retrofit that. Another choice would be to throw some sort of "shutting down" exception that is then handled specially. I'm trying to avoid such extensive changes.

The idea behind the current termination sequence is to at a high level shutdown the code that would call into mpv. For example mouse events should not be a problem because the window is closed. Issue #4020 has to do with events occurring as the window is being closed when in full screen mode, especially when legacy full scree mode is not being used.

What I am concerned about is that IINA allows mpv to initiate termination. This is what this code from MPVController is detecting:

    case MPV_EVENT_SHUTDOWN:
      let quitByMPV = !player.isShuttingDown

Seems like this would allow a window where IINA could be operating on mpv while it is shutting down. But so far I've not been able to trigger a problem.

I've been testing using various scripts that start IINA and quit IINA over and over again. This is one example:

#!/bin/bash

while true; do

    echo Starting IINA
    open -a /Users/low-batt/Library/Developer/Xcode/DerivedData/iina-gwhhvjgxybcchtdcwvmukizbfeip/Build/Products/Debug/IINA.app ~/Movies/earth.mp4

    sleep 2

    echo Instructing  IINA to Quit
    osascript -e 'activate application "IINA"' \
	      -e 'tell application "System Events" to keystroke "q" using command down'

    sleep 2

done

With the changes in the PR I've not been able to generate a crash.

Hi GitHubIINA,
As it turns out this:

IINA v1.3.0 is fine when quitting full screen. No matter macOS version or CPU architecture.

Is not true. In 1.3.0 and earlier versions there is a thread race condition that sometimes results in various crashes. It also causes the Resume last playback position feature to be unreliable. The code sends a command to mpv to shutdown using a synchronous interface. Turns out mpv treats this as a special case and starts processing the shutdown asynchronously in a background thread while the IINA code thinking that has been completed continues with application termination on the main thread. Depending upon timing of these two threads bad things happen. The code is broken.

For 1.3.1 IINA was changed to properly wait for mpv to shutdown. That change had a side effect of increasing the window of vulnerability for thread coordination problems to appear. Multiple problems that were rarely experienced with 1.3.0 and earlier releases became frequent. Issue #4020 is one of those. It was on the list to fix for 1.3.1, but that release was cut early in a rush due to Apple releasing macOS Ventura which broke HDR.

Hi GitHubIINA, As it turns out this:

IINA v1.3.0 is fine when quitting full screen. No matter macOS version or CPU architecture.

Is not true. In 1.3.0 and earlier versions there is a thread race condition that sometimes results in various crashes. It also causes the Resume last playback position feature to be unreliable. The code sends a command to mpv to shutdown using a synchronous interface. Turns out mpv treats this as a special case and starts processing the shutdown asynchronously in a background thread while the IINA code thinking that has been completed continues with application termination on the main thread. Depending upon timing of these two threads bad things happen. The code is broken.

For 1.3.1 IINA was changed to properly wait for mpv to shutdown. That change had a side effect of increasing the window of vulnerability for thread coordination problems to appear. Multiple problems that were rarely experienced with 1.3.0 and earlier releases became frequent. Issue #4020 is one of those. It was on the list to fix for 1.3.1, but that release was cut early in a rush due to Apple releasing macOS Ventura which broke HDR.

Hi. Thank you for the response. Waiting for the next version of IINA.

Felt compelled to attempt a (loose) analogy of the current problem.

Imagine there is a freeway for carrying traffic between IINA and mpv, which goes through an underwater tunnel, because the two are on separate islands. There are many semi-independent sections of IINA and they all have access to the freeway. If one needs something from mpv, or needs to send it a message, it sends a car onto the freeway and through the tunnel toward mpv. But when mpv gets the signal to shut down, it immediately blows up the tunnel. This destroys any cars which are in it at the time, and any cars IINA sends to it afterwards, because once on the freeway they can't stop. But if IINA ever loses a single car, the operating system detects it and crashes IINA. But most of the components in IINA are not even aware that mpv has been sent the shutdown signal, and there is a small amount of time before they are told that IINA itself is shutting down, and until then they carry on like usual. Because there is a degree of randomness in traffic getting to the onramps, they may or may not get on before IINA quits and thus may only cause a crash infrequently.

So, using the analogy, the agreed-upon fix is to add traffic signals to the on-ramps of that freeway, so that a given on-ramp's signal goes red as soon as IINA sends the shutdown signal, preventing tragedy.

But it's a lot of work to add red lights to 200+ on-ramps, sure... My only objection is that this fix doesn't go far enough - this fix adds a red light only at the places which have been reported as crashing; whereas my analysis of the code points a few more potential danger points. They may not fail now but they could sometime in the future due either to updates in IINA, updates to MacOS's scheduling system, or hardware changes. These would be nice to avoid, given IINA's relatively long time between even bugfix releases... But I don't oppose this fix. It should go in ASAP.

I've marked issue #4020 as high priority. The plan is to fix this in the next IINA release, either with this PR or with a different fix depending upon feedback from other reviewers.