Call CGGetActiveDisplayList once to avoid panic (#4069)

Thorsten Ball created 2 years ago

This contains two changes in order to hopefully avoid the panic we saw
in #panics:

```
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value'
crates/gpui/src/platform/mac/display.rs:30
<backtrace::capture::Backtrace as core::fmt::Debug>::fmt
<backtrace::capture::Backtrace as core::fmt::Debug>::fmt
Zed::init_panic_hook::{closure#0}
std::panicking::rust_panic_with_hook
std::panicking::rust_panic_with_hook
std::sys_common::backtrace::output_filename
<std::panicking::begin_panic_handler::FormatStringPayload as core::panic::PanicPayload>::take_box
core::panicking::panic_fmt
core::panicking::panic
gpui::platform::mac::display::display_bounds_to_native
<gpui::platform::mac::window::MacWindow as gpui::platform::PlatformWindow>::bounds
<gpui::window::WindowContext>::window_bounds_changed
<gpui::window::Window>::new::{closure#1}
gpui::platform::mac::window::view_did_change_backing_properties
<gpui::platform::mac::platform::MacPlatform as gpui::platform::Platform>::run
Zed::main
<core::marker::PhantomData<core::option::Option<Zed::LocationData>> as serde::de::DeserializeSeed>::deserialize::<&mut serde_json::de::Deserializer<serde_json::read::StrRead>>
workspace::open_new::<Zed::restore_or_create_workspace::{closure#0}::{closure#0}::{closure#0}::{closure#2}::{closure#0}>::{closure#0}::{closure#0}
std::rt::lang_start_internal
serde_json::de::from_trait::<serde_json::read::StrRead, core::option::Option<Zed::Panic>>
```

Two changes:
* Reduce the number of calls to `CGGetActiveDisplayList` in the hopes
that this will also reduce chances of it returning an empty list.
Previously, in the worst case, we called it 6 times. Now the worst case
is that we call it 3 times.
* Instead of calling `CGGetActiveDisplayList` to get the primary display
we do what Chromium does and construct the primary display from
NSScreen.

Release Notes:

- Fixed a bug that caused a panic that could happen when Zed attempted
to render on a sleeping display.

Change summary

crates/gpui/src/platform/mac/display.rs | 38 ++++++++++++++++++++++----
crates/gpui/src/platform/mac/window.rs  |  2 
2 files changed, 32 insertions(+), 8 deletions(-)

Detailed changes

crates/gpui/src/platform/mac/display.rs

@@ -1,10 +1,16 @@
 use crate::{point, size, Bounds, DisplayId, GlobalPixels, PlatformDisplay};
 use anyhow::Result;
+use cocoa::{
+    appkit::NSScreen,
+    base::{id, nil},
+    foundation::{NSDictionary, NSString},
+};
 use core_foundation::uuid::{CFUUIDGetUUIDBytes, CFUUIDRef};
 use core_graphics::{
     display::{CGDirectDisplayID, CGDisplayBounds, CGGetActiveDisplayList},
     geometry::{CGPoint, CGRect, CGSize},
 };
+use objc::{msg_send, sel, sel_impl};
 use std::any::Any;
 use uuid::Uuid;
 
@@ -27,23 +33,41 @@ impl MacDisplay {
     /// Get the primary screen - the one with the menu bar, and whose bottom left
     /// corner is at the origin of the AppKit coordinate system.
     pub fn primary() -> Self {
-        Self::all().next().unwrap()
+        // Instead of iterating through all active systems displays via `all()` we use the first
+        // NSScreen and gets its CGDirectDisplayID, because we can't be sure that `CGGetActiveDisplayList`
+        // will always return a list of active displays (machine might be sleeping).
+        //
+        // The following is what Chromium does too:
+        //
+        // https://chromium.googlesource.com/chromium/src/+/66.0.3359.158/ui/display/mac/screen_mac.mm#56
+        unsafe {
+            let screens = NSScreen::screens(nil);
+            let screen = cocoa::foundation::NSArray::objectAtIndex(screens, 0);
+            let device_description = NSScreen::deviceDescription(screen);
+            let screen_number_key: id = NSString::alloc(nil).init_str("NSScreenNumber");
+            let screen_number = device_description.objectForKey_(screen_number_key);
+            let screen_number: CGDirectDisplayID = msg_send![screen_number, unsignedIntegerValue];
+            Self(screen_number)
+        }
     }
 
     /// Obtains an iterator over all currently active system displays.
     pub fn all() -> impl Iterator<Item = Self> {
         unsafe {
-            let mut display_count: u32 = 0;
-            let result = CGGetActiveDisplayList(0, std::ptr::null_mut(), &mut display_count);
+            // We're assuming there aren't more than 32 displays connected to the system.
+            let mut displays = Vec::with_capacity(32);
+            let mut display_count = 0;
+            let result = CGGetActiveDisplayList(
+                displays.capacity() as u32,
+                displays.as_mut_ptr(),
+                &mut display_count,
+            );
 
             if result == 0 {
-                let mut displays = Vec::with_capacity(display_count as usize);
-                CGGetActiveDisplayList(display_count, displays.as_mut_ptr(), &mut display_count);
                 displays.set_len(display_count as usize);
-
                 displays.into_iter().map(MacDisplay)
             } else {
-                panic!("Failed to get active display list");
+                panic!("Failed to get active display list. Result: {result}");
             }
         }
     }

crates/gpui/src/platform/mac/window.rs

@@ -484,7 +484,7 @@ impl MacWindow {
 
             let display = options
                 .display_id
-                .and_then(|display_id| MacDisplay::all().find(|display| display.id() == display_id))
+                .and_then(MacDisplay::find_by_id)
                 .unwrap_or_else(MacDisplay::primary);
 
             let mut target_screen = nil;