diff --git a/crates/gpui_macos/src/display_link.rs b/crates/gpui_macos/src/display_link.rs index 468f3d8bde..5387cb6e35 100644 --- a/crates/gpui_macos/src/display_link.rs +++ b/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, - frame_requests: DispatchRetained, +static REGISTRY: Mutex = Mutex::new(Registry::new()); + +struct Registry { + displays: BTreeMap, + next_subscriber_id: u64, } -impl DisplayLink { - pub fn new( - display_id: CGDirectDisplayID, - data: *mut c_void, - callback: extern "C" fn(*mut c_void), - ) -> Result { - 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)>, +} + +// 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` +// 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, +) -> Result { + 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, + 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(); } } diff --git a/crates/gpui_macos/src/window.rs b/crates/gpui_macos/src/window.rs index 011ece9d91..4314d07cc8 100644 --- a/crates/gpui_macos/src/window.rs +++ b/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, - display_link: Option, + frame_source: Option, renderer: renderer::Renderer, request_frame_callback: Option>, event_callback: Option 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); }