munki / munki

Managed software installation for macOS —
https://www.munki.org/munki/
Other
3.11k stars 349 forks source link

Overlay on loginwindow sometimes is not the right size #1129

Closed nixtar closed 2 months ago

nixtar commented 2 years ago

Hello,

Part of our onboarding process for macs is to run an inhouse developed app that binds the mac to the domain, waits for the MDM to deploy munki and its configuration profile, then sets the flag for munki to run on next logonwindow and reboots the device.

With this process munki always does its first deployment of apps on the logon screen.

This works great but occasionally we see the transparent overlay not be the right size and leave part of the loginwindow visible and accessible.

Luckily its just the top right corner so a user cant do something like click shutdown/restart or login. So its mostly cosmetic issue and is why I haven't been too bothered by it. See screenshot: image

I've only seen this on MacBooks, both Intel and Apple Silicon. It first started happening on Big Sur. However its a lot more prone to happening on Apple Silicon devices. If I had to make a wild guess.. Happens 10-15% of the time on Intel and ~50% on Apple Silicon. It also only happens on the first time munki runs after a reboot. If I "break" a package to intentionally cause a check for updates and install loop it doesn't happen after the first time.

My assumption is that there is a race condition here where the munki process is being started before the logonwindow has fully loaded and its grabbing the wrong window size. A hack could be to check the window size in another thread every minute or so and resize when its suddenly changed? Not sure if this kind of thing can done with purely constraints on the view? I assume this same behaviour would happen if you changed the display output (on a mac mini for eg) to one with a different res while munki was running.

gregneagle commented 2 years ago

I've seen this as well when "bootstrapping" an M1 Mac. It's difficult to reproduce on demand. I would not hold my breath looking for a fix from me in the near term.

nixtar commented 2 years ago

Agree. Its not urgent in the slightest. Just figured I'd log it and maybe next time someone is touching that code they may see this and have a look.

My swift/cocoa is a bit noob tier but maybe one day I'll have a peek.

Best way I could reproduce it was to have a nopkg that was always detected as not installed, then in the pre install I called reboot. Basically putting the device into a reboot > bootstrap loop. That was how I was able to make it happen on Intel's.

nixtar commented 2 months ago

Hey @gregneagle

As the overlay had some love in 6.6.0 I wanted to retest this.

Unfortunately, it appears to still be an issue and its a bit more jarring when it occurs now. LWScreenShot 2024-07-31 at 11 16 43 AM

I've not been able to reproduce it on a logoff, post reboot seems be the best way to reproduce it.

This is hacky... But what about making the background 1.5x the expected size and centering it? That way even if it does this it will cover the whole screen. I got the idea when I noticed that the progress window is being centred correctly.

PS: When its sized correctly it looks good. :D

nixtar commented 2 months ago

Decided to have a bit of a play with this.

I wanted to know if the background was the wrong size or the wrong position.

So in BlurWindowController I added self.window?.center() under https://github.com/munki/munki/blob/cc7252d0dceb5f1b351fe230ff182f7b94fba099/code/apps/MunkiStatus/MunkiStatus/BlurredBackgroundController.swift#L25:

EG:

if let screen {
    self.window?.setFrame((screen.frame), display: true)
    self.window?.center()
}

This resulted in the following: LWScreenShot 2024-07-31 at 11 59 59 AM

To me this confirms that the background is the correct size, just in the wrong spot. The issue with this is that it's trying to account for the menu bar.

I wrote the following function that attempts to centre the screen while ignoring the menu bar. I call it in place of self.window?.center() from above.

func centerWindowIgnoringMenuBar(window: NSWindow, onScreen screen: NSScreen) {
    let screenFrame = screen.frame
    let visibleFrame = screen.visibleFrame
    let menuBarHeight = screenFrame.height - visibleFrame.height

    let newX = (screenFrame.width - window.frame.width) / 2
    let newY = (screenFrame.height - window.frame.height - menuBarHeight) / 2

    window.setFrameOrigin(NSPoint(x: newX, y: newY))
}

And this gets even closer: LWScreenShot 2024-07-31 at 12 36 57 PM

This got me thinking. Clearly the screen object is correct, or this should have resulted in the same issue as in my original post. As a test I added the following self.window?.setFrameOrigin(screen.frame.origin) under the setFrame call. (In place of the various attempts to recenter the window.

And: LWScreenShot 2024-07-31 at 12 53 37 PM

This doesn't quite make sense to me as setFrame should also be setting the frame origin...

Full loadwindow method:

override func loadWindow() {
    window = BlurWindow(contentRect: CGRect(x: 0, y: 0, width: 100, height: 100), styleMask: [], backing: .buffered, defer: true)
    self.window?.contentViewController = BlurViewController()
    if let screen {
        self.window?.setFrame((screen.frame), display: true)
        self.window?.setFrameOrigin(screen.frame.origin)
    }
    self.window?.collectionBehavior = [.canJoinAllSpaces]
    if atLoginWindow() {
        self.window?.canBecomeVisibleWithoutLogin = true
    }
}

With default 6.6.0 installed I can reproduce the issue 95% of the time post reboot on an M2 MacBook air running macOS 14.5 by doing the following:

touch /Users/Shared/.com.googlecode.munki.checkandinstallatstartup
sudo reboot

So far I have run this ~25 times with the above setFrameOrigin change and I've not been able to reproduce it since.

gregneagle commented 2 months ago

Thanks for this. Can you create a pull request with your changes?

gregneagle commented 2 months ago

I think I figured out your change: you've modified the override func loadWindow() in the class BlurWindowController in the BlurredBackgroundController.swift file, yes?

gregneagle commented 2 months ago

Here's the committed change: https://github.com/munki/munki/commit/faa048110f971da6d4ffb64ded5f931be429ad96#diff-91b75354eeb2d6ab90f653a47069322b014e058ff3cba1558712c72eaa478681

gregneagle commented 2 months ago

This change is in 6.6 RC1 and will be in the official 6.6 release. Going to close this; we can re-open if it's discovered the fix doesn't completely address the issue.

nixtar commented 2 days ago

Hey @gregneagle finally had a chance to test this and it looks like you only applied the fix in BlurredBackgroundController.swift for Managed Software Center but not MunkiStatus :).

gregneagle commented 1 day ago

...and this is why Pull Requests are appreciated.

gregneagle commented 1 day ago

https://github.com/munki/munki/commit/b385bd83e4b247fc7811751ebf85756d731fe804