Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Black windows on the other screens won’t disappear when exiting full screen #4683

Closed
coin3x opened this issue Nov 9, 2023 · 8 comments · Fixed by #4684
Closed

Black windows on the other screens won’t disappear when exiting full screen #4683

coin3x opened this issue Nov 9, 2023 · 8 comments · Fixed by #4684

Comments

@coin3x
Copy link
Contributor

coin3x commented Nov 9, 2023

System and IINA version:

  • macOS 14.1
  • IINA 1.3.3

Expected behavior:
Black windows on the other screens disappear when IINA exits full screen.

Actual behavior:
Black windows did not disappear.

Steps to reproduce:

  1. Turn on “Black out other monitors while in full screen”.
  2. With a dual-monitor setup, play a video in full screen.
  3. Let the computer go to sleep.
  4. Wake up the computer but do not unlock.
  5. Wait for the other monitor to reconnect.
  6. Unlock the computer.
  7. Try to exit full screen and observe the black window not being removed.

Applying the following patch before reproducing the problem results in these log entries:

Patch
diff --git a/iina/MainWindowController.swift b/iina/MainWindowController.swift
index de1ac6e..367cdc7 100644
--- a/iina/MainWindowController.swift
+++ b/iina/MainWindowController.swift
@@ -618,6 +618,7 @@ class MainWindowController: PlayerWindowController {
       // This observer handles a situation that the user connected a new screen or removed a screen
       let screenCount = NSScreen.screens.count
       if self.fsState.isFullscreen && Preference.bool(for: .blackOutMonitor) && self.cachedScreenCount != screenCount {
+        Logger.log("didChangeScreenParametersNotification, cachedScreenCount: \(self.cachedScreenCount), newCount = \(screenCount)")
         self.removeBlackWindow()
         self.blackOutOtherMonitors()
       }
@@ -1341,6 +1342,7 @@ class MainWindowController: PlayerWindowController {
     videoView.videoLayer.resume()
 
     if Preference.bool(for: .blackOutMonitor) {
+      Logger.log("windowDidEnterFullScreen")
       blackOutOtherMonitors()
     }
 
@@ -1425,6 +1427,7 @@ class MainWindowController: PlayerWindowController {
     fsState.finishAnimating()
 
     if Preference.bool(for: .blackOutMonitor) {
+      Logger.log("windowDidExitFullScreen")
       removeBlackWindow()
     }
 
@@ -1730,6 +1733,7 @@ class MainWindowController: PlayerWindowController {
     super.windowDidBecomeMain(notification)
 
     if fsState.isFullscreen && Preference.bool(for: .blackOutMonitor) {
+      Logger.log("windowDidBecomeMain")
       blackOutOtherMonitors()
     }
     player.events.emit(.windowMainStatusChanged, data: true)
@@ -1738,6 +1742,7 @@ class MainWindowController: PlayerWindowController {
   override func windowDidResignMain(_ notification: Notification) {
     super.windowDidResignMain(notification)
     if Preference.bool(for: .blackOutMonitor) {
+      Logger.log("windowDidResignMain")
       removeBlackWindow()
     }
     player.events.emit(.windowMainStatusChanged, data: false)
@@ -2562,8 +2567,10 @@ class MainWindowController: PlayerWindowController {
   // MARK: - UI: Others
 
   private func blackOutOtherMonitors() {
+    Logger.log("blackOutOtherMonitors")
     screens = NSScreen.screens.filter { $0 != window?.screen }
 
+    Logger.log("blackWindows.count: \(blackWindows.count)")
     blackWindows = []
 
     for screen in screens {
@@ -2579,6 +2586,7 @@ class MainWindowController: PlayerWindowController {
   }
 
   private func removeBlackWindow() {
+    Logger.log("removeBlackWindow")
     for window in blackWindows {
       window.orderOut(self)
     }

image

It appears that these events happened in sequence:

  1. The monitor reconnected.
  2. Black windows were recreated.
  3. The user unlocked the computer and IINA’s MainWindow became main.
  4. Black windows were recreated again but without the previously created ones removed.
@coin3x coin3x changed the title Black windows on the other screens does not disappear when exiting full screen Black windows on the other screens won’t disappear when exiting full screen Nov 9, 2023
@low-batt
Copy link
Contributor

low-batt commented Nov 9, 2023

Excellent report. I'm having trouble reproducing this. I'm wondering if the behavior of macOS has changed. I see this was reported against 14.1. I am running 14.1.1. Anyway, after almost giving up on reproducing this I am now looking at a black window on my external monitor. This is after I have quit IINA. To trigger the bad behavior I'm experiencing I had to unplug the external display and plug it back in while the Mac was asleep.

@coin3x Are you experiencing the black window sticking around after IINA has terminated? Did this start happening after you upgraded to macOS Sonoma?

@svobs Can you test this issue in your environment?

In another issue under macOS 14.0 @svobs noticed hundreds of didChangeScreenParametersNotification occuring. I did not experience that when testing before upgrading to Sonoma.

Now that I'm on Sonoma the Open Recent list seems to be malfunctioning.

I've not looked into this IINA feature before. Although some of this might be due to macOS Sonoma regressions, I'm thinking we need to review this code. I'm concerned about this:

  private func removeBlackWindow() {
    Logger.log("removeBlackWindow")
    for window in blackWindows {
      window.orderOut(self)
    }
    blackWindows = []
  }

From the documentation for orderOut:

If the window is the key or main window, the window object immediately behind it is made key or main in its place. Calling orderOut(_:) causes the window to be removed from the screen, but does not cause it to be released. See the close() method for information on when a window is released. Calling orderOut(_:) on a child window causes the window to be removed from its parent window before being removed.

I'm not liking the "but does not cause it to be released" part. Does this mean we are leaking windows?

I'm concerned about this as well:

  private func blackOutOtherMonitors() {
    Logger.log("blackOutOtherMonitors")
    screens = NSScreen.screens.filter { $0 != window?.screen }

    Logger.log("blackWindows.count: \(blackWindows.count)")
    blackWindows = []

    for screen in screens {
      var screenRect = screen.frame
      screenRect.origin = CGPoint(x: 0, y: 0)
      let blackWindow = NSWindow(contentRect: screenRect, styleMask: [], backing: .buffered, defer: false, screen: screen)
      blackWindow.backgroundColor = .black
      blackWindow.level = .iinaBlackScreen

      blackWindows.append(blackWindow)
      blackWindow.makeKeyAndOrderFront(nil)
    }
  }

From the documentation for makeKeyAndOrderFront:

Moves the window to the front of the screen list, within its level, and makes it the key window; that is, it shows the window.

I believe that is triggering this error seen in the Xcode console:

Warning: -[NSWindow makeKeyWindow] called on NSWindow 0x7fe08908a850 which returned NO from -[NSWindow canBecomeKeyWindow].

From the documentation for canBecomeMain:

Attempts to make the window the main window are abandoned if the value of this property is false. The value of the property is true if the window is visible, is not an NSPanel object, and has a title bar or a resize mechanism. Otherwise, the value is false.

I've run out of time today to investigate further, but at the moment I'm thinking there may be multiple problems in this code.

@low-batt low-batt added the bug label Nov 9, 2023
@low-batt low-batt linked a pull request Nov 9, 2023 that will close this issue
2 tasks
@coin3x
Copy link
Contributor Author

coin3x commented Nov 9, 2023

@coin3x Are you experiencing the black window sticking around after IINA has terminated?

No, quitting IINA makes them go away.

Did this start happening after you upgraded to macOS Sonoma?

It's been a while and I'm not sure.

Does this mean we are leaking windows?

They are deallocated after IINA exits full screen:

image

Quartz Debug's Window List shows either zero (when not in full screen) or one black window after toggling full screen multiple times, unlike when the problem I'm reporting was triggered.

I think as long as we don't keep references to them, it's fine.

@svobs
Copy link
Contributor

svobs commented Nov 10, 2023

@low-batt Reproduced on 14.1.1. I went to full screen, kept my external display plugged in, but disconnected my power adapter, and then closed my MacBook's lid. Verified that the external display showed that it lost input. Then opened the lid again, entered my login info to unlock and then exited full screen. The black window was still there until I quit IINA.

Also tried @coin3x's solution and it appears to have fixed the problem.

It has been a couple of months since I dealt with this issue so my memory is fuzzy, but I do recall trying to deal with this, and the whole black window mechanism being kind of a headache. Strange that causing close() on a black widow caused a program crash. And also remember having trouble trying to reuse them, and having to overwrite the array to truly get rid of them. I guess even though NSWindows have references everywhere and are associated with the app, those are all weak references, and they are set to uninit if you don't keep a reference to it. (It wouldn't be the only AppKit object which is funky like that. NSLayoutConstraint is even worse because setting isActive = false on it will actually dispose of the object if you are only using weak references for it...).

I also found an issue when running in legacy full screen and using an external tool (BetterTouchTool in my case) to move the video window to the next display, which ended up moving it behind the black window. This left me with one black display and one display showing the desktop. More of a fringe case I guess.

I never experienced black windows lingering after IINA quit. That sounds pretty bad and implies something is wrong with MacOS.

Now that I'm on Sonoma the Open Recent list seems to be malfunctioning.

I upgraded from MacOS 14.0 to 14.1.1 yesterday and am now seeing the same thing. Looks like a new one!

@low-batt
Copy link
Contributor

@coin3x Before I forget again, extra THANKS for posting the patch containing the debug logging you were using. Very helpful.

Another busy day, but I was able to investigate some more. I first tested this on my other MacBook Pro that is running macOS Ventura 13.6.1. The way I've been testing reproduced the problem reported in the issue. It did not reproduce after applying the proposed fix. I was unable to reproduce the behavior where the black window remains after IINA terminates.

I went back to my MacBook Pro running macOS 14.1.1. I was finally able to reproduce the problem under the released version of IINA. Running under Xcode without the fix the behavior is not the same in the test case I'm running. The external monitor remains black after IINA has terminated. As if IINA has left a zombie window. To get rid of the window I have to unplug the external monitor and then plug it back in. The proposed fix also corrects this.

I suspect we are experiencing defects in the latest version of AppKit available in macOS Sonoma. Building IINA with Xcode 15 allows use of this new version of AppKit. I suspect that is also behind the problem with the Open Recent menu. I would think others would be encountering the problem with Open Recent. From a quick search I did not find a discussion of this.

I will enter these other problems as separate issues to keep this issue focused solely on the black window behavior for which the posted PR fixes.

@svobs
Copy link
Contributor

svobs commented Nov 11, 2023

I suspect we are experiencing defects in the latest version of AppKit available in macOS Sonoma. Building IINA with Xcode 15 allows use of this new version of AppKit. I suspect that is also behind the problem with the Open Recent menu. I would think others would be encountering the problem with Open Recent. From a quick search I did not find a discussion of this.

@low-batt I know this is off-topic, but I noticed a pattern. For me, Open Recent stays populated across launches...of the same exact IINA binary. For example, I can launch my release copy of IINA 1.3.3 and everything works, but as soon as I launch my dev build, it gets blown away. In fact, I can relaunch the dev build and the recents are preserved, but if I make any changes which need a recompile, the recents are erased.

This may be a hasty security fix which tries to couple the recent documents to the application's binary checksum or something along those lines. Because it's easy to conceive of an exploit which would allow a malignant app to read IINA's recent documents. Or...it may just be a bug. But it's easy to see why end users aren't reporting anything.

@svobs
Copy link
Contributor

svobs commented Nov 11, 2023

The external monitor remains black after IINA has terminated. As if IINA has left a zombie window. To get rid of the window I have to unplug the external monitor and then plug it back in. The proposed fix also corrects this.

That is concerning. At least it will also be fixed by this change.

@low-batt
Copy link
Contributor

The fix for this issue has been merged into the IINA develop branch. GitHub automatically closed the linked issue in reaction to the merge. I am reopening this issue until the fix is available in an official release of IINA so that users intending to report this problem can easily locate this existing report. Once the fix for this issue has been included in an official release this issue will be closed.

@low-batt low-batt reopened this Dec 26, 2023
@low-batt
Copy link
Contributor

low-batt commented Jan 1, 2024

This has been fixed in IINA 1.3.4.

Thanks @coin3x for the fix!

@low-batt low-batt closed this as completed Jan 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants