gpui_macos: Replace per-window CVDisplayLinks with a per-display registry (#60696)

Fixes ZED-7XR

CVDisplayLink has no safe teardown: `CVDisplayLinkStop` only flags the
io thread to exit "soon" and returns, `CVDisplayLinkRelease` frees the
link immediately, and there is no way to learn when a final output
callback has finished. This produced two distinct use-after-free crash
classes on the display-link thread: the io thread reading the freed
link's internals (`CVHWTime::reset`, worked around by leaking the link
in #32116), and a straggler output callback dereferencing the freed
dispatch source through its context pointer
(`dispatch_source_merge_data`, ZED-7XR). Because we tore down a
`DisplayLink` on every resize tick, window activation, and occlusion
change, the existing workaround also leaked a stopped CVDisplayLink per
teardown, unboundedly.

This replaces per-window links with a static registry holding one
immortal `CVDisplayLink` per display. Windows subscribe to their
display's link with the dispatch source that paces their frames; the
output callback's context is a display id rather than a pointer, so the
only memory it ever dereferences is the registry itself, under its lock.
Links start and stop at subscriber-count edges (so multiple windows on
one display can't interleave starts/stops incorrectly), and a window's
dispatch source lives for the life of the window, is removed from the
registry on close, and is then genuinely released. Both crash classes
become unreachable — the link because it is never released, the source
because a straggler callback either finds it live in the registry or
doesn't find it at all — and there are no leaks and no `mem::forget`.
Since we never hold the registry lock across CoreVideo calls, no lock
cycle through CVDisplayLink's undocumented internal locks is possible;
the module doc comment spells out the invariants.

This supersedes #60481, which fixed ZED-7XR by leaking the dispatch
source on teardown.

Release Notes:

- Fixed a rare crash on macOS when closing, resizing, or switching
between windows, and eliminated a memory leak in display-link teardown

crates/gpui_macos/src/display_link.rs

@@ -1,38 +1,241 @@
+//! Frame pacing for macOS windows, built on `CVDisplayLink`.
+//!
+//! CVDisplayLink has no safe teardown: `CVDisplayLinkStop` merely flags the
+//! link's io thread to exit "soon" and returns immediately, and there is no
+//! way to learn when (or whether) a final output callback has finished. Two
+//! crash classes came from ignoring this:
+//!
+//! * releasing the link after `stop` let the still-running io thread read the
+//!   freed link's internals (segfault in `CVHWTime::reset`, fixed by leaking
+//!   the link in #32116);
+//! * releasing the dispatch source that the output callback dereferenced via
+//!   its context pointer let a straggler callback read freed memory (segfault
+//!   in `dispatch_source_merge_data`, Sentry issue ZED-7XR).
+//!
+//! Instead of leaking one link and one source per teardown (and teardown
+//! happens on every resize tick, activation, and occlusion change), we keep a
+//! single immortal `CVDisplayLink` per display in a static registry and have
+//! windows subscribe to it:
+//!
+//! * Links are created lazily and never released, so the release race cannot
+//!   occur. The output callback's context is the display id (an integer, not
+//!   a pointer), and the only memory the callback dereferences is the static
+//!   registry, so a straggler callback after `stop` is a harmless no-op.
+//! * Each window owns one dispatch source for the life of the window. On
+//!   window close it is removed from the registry under the lock (making it
+//!   unreachable from the callback), cancelled, and genuinely released.
+//! * A display's link runs iff it has subscribers; windows never start or
+//!   stop links directly, so interleaved starts/stops of windows sharing a
+//!   display cannot conflict.
+//!
+//! One tradeoff of immortal entries: a link created for a given
+//! `CGDirectDisplayID` is reused forever, including after the display is
+//! unplugged and one reappears with the same id, or after mode/refresh-rate
+//! changes. `CVDisplayLink` looks up display timing dynamically, so a cached
+//! link keeps pacing correctly; if that ever proves untrue the fix is to
+//! also refresh entries from a display-reconfiguration callback, not to
+//! release links (which would reintroduce the teardown race).
+//!
+//! Lock ordering: the output callback runs on the link's io thread and takes
+//! the registry lock, possibly while holding CVDisplayLink-internal locks. To
+//! avoid a lock cycle through those (undocumented) internals, we never call a
+//! CoreVideo function while holding the registry lock. Registry mutations and
+//! link start/stop happen on the main thread only, which keeps the `running`
+//! flag and the link's actual state consistent without holding the lock
+//! across the calls.
+//!
+//! `std::sync::Mutex` rather than `parking_lot` is deliberate: on macOS it is
+//! currently backed by `os_unfair_lock`, whose priority donation resolves
+//! inversions between the high-priority io thread and the main thread. (That
+//! is a std implementation detail, not a guarantee; if it changes, the cost
+//! is added latency under contention, not incorrectness.)
+
 use anyhow::Result;
 use core_graphics::display::CGDirectDisplayID;
 use dispatch2::{
     _dispatch_source_type_data_add, DispatchObject, DispatchQueue, DispatchRetained, DispatchSource,
 };
 use gpui_util::ResultExt;
-use std::ffi::c_void;
+use std::{
+    collections::{BTreeMap, btree_map},
+    ffi::c_void,
+    sync::{Mutex, MutexGuard, PoisonError},
+};
 
-pub struct DisplayLink {
-    display_link: Option<sys::DisplayLink>,
-    frame_requests: DispatchRetained<DispatchSource>,
+static REGISTRY: Mutex<Registry> = Mutex::new(Registry::new());
+
+struct Registry {
+    displays: BTreeMap<CGDirectDisplayID, DisplayEntry>,
+    next_subscriber_id: u64,
 }
 
-impl DisplayLink {
-    pub fn new(
-        display_id: CGDirectDisplayID,
-        data: *mut c_void,
-        callback: extern "C" fn(*mut c_void),
-    ) -> Result<DisplayLink> {
-        unsafe extern "C" fn display_link_callback(
-            _display_link_out: *mut sys::CVDisplayLink,
-            _current_time: *const sys::CVTimeStamp,
-            _output_time: *const sys::CVTimeStamp,
-            _flags_in: i64,
-            _flags_out: *mut i64,
-            frame_requests: *mut c_void,
-        ) -> i32 {
-            unsafe {
-                let frame_requests = &*(frame_requests as *const DispatchSource);
-                frame_requests.merge_data(1);
-                0
-            }
+impl Registry {
+    const fn new() -> Self {
+        Registry {
+            displays: BTreeMap::new(),
+            next_subscriber_id: 0,
         }
+    }
+}
 
-        unsafe {
+struct DisplayEntry {
+    link: sys::DisplayLink,
+    running: bool,
+    subscribers: Vec<(SubscriberId, DispatchRetained<DispatchSource>)>,
+}
+
+// SAFETY: Both fields wrapping raw pointers are refcounted handles to
+// thread-safe objects that are valid on any thread: `sys::DisplayLink` to a
+// CoreVideo object, and each subscriber's `DispatchRetained<DispatchSource>`
+// to a GCD object (which the display's io thread really does use, calling
+// `merge_data` from the output callback). All mutation of the entry itself
+// is serialized by the registry lock.
+unsafe impl Send for DisplayEntry {}
+
+#[derive(Copy, Clone, PartialEq, Eq)]
+struct SubscriberId(u64);
+
+fn lock_registry() -> MutexGuard<'static, Registry> {
+    // Proceeding past poison is safe here (the map's invariants hold after
+    // any partial mutation), and panicking instead would abort the process
+    // when this is reached from the extern "C" output callback.
+    REGISTRY.lock().unwrap_or_else(PoisonError::into_inner)
+}
+
+fn debug_assert_main_thread() {
+    #[cfg(debug_assertions)]
+    {
+        use objc::{class, msg_send, sel, sel_impl};
+        let is_main_thread: objc::runtime::BOOL =
+            unsafe { msg_send![class!(NSThread), isMainThread] };
+        debug_assert!(
+            is_main_thread == objc::runtime::YES,
+            "display link registry mutations must happen on the main thread; \
+             the registry's lock ordering and state consistency depend on it"
+        );
+    }
+}
+
+unsafe extern "C" fn display_link_output_callback(
+    _display_link_out: *mut sys::CVDisplayLink,
+    _current_time: *const sys::CVTimeStamp,
+    _output_time: *const sys::CVTimeStamp,
+    _flags_in: i64,
+    _flags_out: *mut i64,
+    display_id: *mut c_void,
+) -> i32 {
+    let display_id = display_id as usize as CGDirectDisplayID;
+    let registry = lock_registry();
+    if let Some(entry) = registry.displays.get(&display_id) {
+        for (_, frame_requests) in &entry.subscribers {
+            frame_requests.merge_data(1);
+        }
+    }
+    0
+}
+
+fn subscribe(
+    display_id: CGDirectDisplayID,
+    frame_requests: DispatchRetained<DispatchSource>,
+) -> Result<SubscriberId> {
+    debug_assert_main_thread();
+
+    let needs_link = !lock_registry().displays.contains_key(&display_id);
+    let new_link = if needs_link {
+        // Created outside the registry lock; see the lock ordering note above.
+        Some(unsafe {
+            sys::DisplayLink::new(
+                display_id,
+                display_link_output_callback,
+                display_id as usize as *mut c_void,
+            )?
+        })
+    } else {
+        None
+    };
+
+    let (subscriber_id, link_to_start) = {
+        let mut registry = lock_registry();
+        let registry = &mut *registry;
+        let subscriber_id = SubscriberId(registry.next_subscriber_id);
+        registry.next_subscriber_id += 1;
+        let entry = match (registry.displays.entry(display_id), new_link) {
+            // If an entry appeared since the check above, dropping `new_link`
+            // is safe: a never-started link has no io thread, so releasing it
+            // doesn't race.
+            (btree_map::Entry::Occupied(entry), _) => entry.into_mut(),
+            (btree_map::Entry::Vacant(vacant), Some(link)) => vacant.insert(DisplayEntry {
+                link,
+                running: false,
+                subscribers: Vec::new(),
+            }),
+            (btree_map::Entry::Vacant(_), None) => {
+                // Entries are never removed, so an entry observed above cannot
+                // have disappeared while subscriptions stay on the main thread.
+                anyhow::bail!("display link registry entry vanished for display {display_id}");
+            }
+        };
+        entry.subscribers.push((subscriber_id, frame_requests));
+        let link_to_start = if entry.running {
+            None
+        } else {
+            entry.running = true;
+            // Clone the refcounted handle so the CVDisplayLinkStart call can
+            // happen after the lock is released.
+            Some(entry.link.clone())
+        };
+        (subscriber_id, link_to_start)
+    };
+
+    if let Some(mut link) = link_to_start {
+        if let Err(error) = unsafe { link.start() } {
+            let mut registry = lock_registry();
+            if let Some(entry) = registry.displays.get_mut(&display_id) {
+                entry.running = false;
+                entry.subscribers.retain(|(id, _)| *id != subscriber_id);
+            }
+            return Err(error);
+        }
+    }
+
+    Ok(subscriber_id)
+}
+
+fn unsubscribe(display_id: CGDirectDisplayID, subscriber_id: SubscriberId) {
+    debug_assert_main_thread();
+
+    let link_to_stop = {
+        let mut registry = lock_registry();
+        let Some(entry) = registry.displays.get_mut(&display_id) else {
+            return;
+        };
+        entry.subscribers.retain(|(id, _)| *id != subscriber_id);
+        if entry.subscribers.is_empty() && entry.running {
+            entry.running = false;
+            Some(entry.link.clone())
+        } else {
+            None
+        }
+    };
+
+    if let Some(mut link) = link_to_stop {
+        // A final output callback can still fire after this returns; it finds
+        // no subscribers for this display and does nothing.
+        unsafe { link.stop().log_err() };
+    }
+}
+
+/// A per-window source of frame requests, paced by the display the window is
+/// on. The wrapped dispatch source coalesces vsync ticks from the display's
+/// io thread and invokes `callback(data)` on the main queue.
+pub struct WindowFrameSource {
+    frame_requests: DispatchRetained<DispatchSource>,
+    registration: Option<(CGDirectDisplayID, SubscriberId)>,
+}
+
+impl WindowFrameSource {
+    pub fn new(data: *mut c_void, callback: extern "C" fn(*mut c_void)) -> Self {
+        let frame_requests = unsafe {
             let frame_requests = DispatchSource::new(
                 &raw const _dispatch_source_type_data_add as *mut _,
                 0,
@@ -41,47 +244,39 @@ impl DisplayLink {
             );
             frame_requests.set_context(data);
             frame_requests.set_event_handler_f(callback);
+            // Resume before this source can ever be dropped: destroying a
+            // suspended dispatch source is undefined behavior (#50875).
             frame_requests.resume();
-
-            let display_link = sys::DisplayLink::new(
-                display_id,
-                display_link_callback,
-                &*frame_requests as *const DispatchSource as *mut c_void,
-            )?;
-
-            Ok(Self {
-                display_link: Some(display_link),
-                frame_requests,
-            })
+            frame_requests
+        };
+        Self {
+            frame_requests,
+            registration: None,
         }
     }
 
-    pub fn start(&mut self) -> Result<()> {
-        unsafe {
-            self.display_link.as_mut().unwrap().start()?;
-        }
+    pub fn start(&mut self, display_id: CGDirectDisplayID) -> Result<()> {
+        self.stop();
+        let subscriber_id = subscribe(display_id, self.frame_requests.clone())?;
+        self.registration = Some((display_id, subscriber_id));
         Ok(())
     }
 
-    pub fn stop(&mut self) -> Result<()> {
-        unsafe {
-            self.display_link.as_mut().unwrap().stop()?;
+    pub fn stop(&mut self) {
+        if let Some((display_id, subscriber_id)) = self.registration.take() {
+            unsubscribe(display_id, subscriber_id);
         }
-        Ok(())
     }
 }
 
-impl Drop for DisplayLink {
+impl Drop for WindowFrameSource {
     fn drop(&mut self) {
-        self.stop().log_err();
-        // We see occasional segfaults on the CVDisplayLink thread.
-        //
-        // It seems possible that this happens because CVDisplayLinkRelease releases the CVDisplayLink
-        // on the main thread immediately, but the background thread that CVDisplayLink uses for timers
-        // is still accessing it.
-        //
-        // We might also want to upgrade to CADisplayLink, but that requires dropping old macOS support.
-        std::mem::forget(self.display_link.take());
+        self.stop();
+        // Unsubscribing makes this source unreachable from the output
+        // callback, so unlike before (ZED-7XR) it is safe to actually release
+        // it. Cancelling first guarantees the event handler never runs again;
+        // its context points at the window's native view, which may be
+        // deallocated after this.
         self.frame_requests.cancel();
     }
 }

crates/gpui_macos/src/window.rs

@@ -1,6 +1,6 @@
 use crate::{
-    BoolExt, DisplayLink, MacDisplay, NSRange, NSStringExt, TISCopyCurrentKeyboardInputSource,
-    TISGetInputSourceProperty, events::platform_input_from_native,
+    BoolExt, MacDisplay, NSRange, NSStringExt, TISCopyCurrentKeyboardInputSource,
+    TISGetInputSourceProperty, WindowFrameSource, events::platform_input_from_native,
     kTISPropertyInputSourceIsASCIICapable, kTISPropertyInputSourceType, kTISTypeKeyboardInputMode,
     ns_string, renderer,
 };
@@ -497,7 +497,7 @@ struct MacWindowState {
     background_appearance: WindowBackgroundAppearance,
     cursor_style: CursorStyle,
     cursor_visible: Arc<AtomicBool>,
-    display_link: Option<DisplayLink>,
+    frame_source: Option<WindowFrameSource>,
     renderer: renderer::Renderer,
     request_frame_callback: Option<Box<dyn FnMut(RequestFrameOptions)>>,
     event_callback: Option<Box<dyn FnMut(PlatformInput) -> gpui::DispatchEventResult>>,
@@ -671,16 +671,17 @@ impl MacWindowState {
             // AppKit can temporarily report no screen while displays are being reconfigured.
             return;
         };
-        if let Some(mut display_link) =
-            DisplayLink::new(display_id, self.native_view.as_ptr() as *mut c_void, step).log_err()
-        {
-            display_link.start().log_err();
-            self.display_link = Some(display_link);
-        }
+        let data = self.native_view.as_ptr() as *mut c_void;
+        self.frame_source
+            .get_or_insert_with(|| WindowFrameSource::new(data, step))
+            .start(display_id)
+            .log_err();
     }
 
     fn stop_display_link(&mut self) {
-        self.display_link = None;
+        if let Some(frame_source) = self.frame_source.as_mut() {
+            frame_source.stop();
+        }
     }
 
     fn is_maximized(&self) -> bool {
@@ -891,7 +892,7 @@ impl MacWindow {
                 background_appearance: WindowBackgroundAppearance::Opaque,
                 cursor_style: CursorStyle::Arrow,
                 cursor_visible,
-                display_link: None,
+                frame_source: None,
                 renderer: renderer::new_renderer(
                     renderer_context,
                     native_window as *mut _,
@@ -1171,7 +1172,7 @@ impl Drop for MacWindow {
         this.renderer.destroy();
         let window = this.native_window;
         let sheet_parent = this.sheet_parent.take();
-        this.display_link.take();
+        this.frame_source.take();
         unsafe {
             this.native_window.setDelegate_(nil);
         }