diff --git a/crates/gpui/src/app.rs b/crates/gpui/src/app.rs index 857b792dd1..65a5331142 100644 --- a/crates/gpui/src/app.rs +++ b/crates/gpui/src/app.rs @@ -1235,7 +1235,7 @@ impl App { // on windows we quite frequently lose the race and return a window that has never rendered, which leads to a crash // where DispatchTree::root_node_id asserts on empty nodes let clear = window.draw(cx); - clear.clear(); + clear.clear(cx); cx.window_handles.insert(id, window.handle); cx.windows.get_mut(id).unwrap().replace(Box::new(window)); @@ -1624,7 +1624,7 @@ impl App { }) .collect::>() { - self.update_window(window, |_, window, cx| window.draw(cx).clear()) + self.update_window(window, |_, window, cx| window.draw(cx).clear(cx)) .unwrap(); } diff --git a/crates/gpui/src/app/test_app.rs b/crates/gpui/src/app/test_app.rs index 268fa891b5..d241863883 100644 --- a/crates/gpui/src/app/test_app.rs +++ b/crates/gpui/src/app/test_app.rs @@ -486,7 +486,7 @@ impl TestAppWindow { let mut app = self.app.borrow_mut(); let any_handle: AnyWindowHandle = self.handle.into(); app.update_window(any_handle, |_, window, cx| { - window.draw(cx).clear(); + window.draw(cx).clear(cx); }) .unwrap(); } diff --git a/crates/gpui/src/app/test_context.rs b/crates/gpui/src/app/test_context.rs index 582e1ad840..c80c259345 100644 --- a/crates/gpui/src/app/test_context.rs +++ b/crates/gpui/src/app/test_context.rs @@ -900,7 +900,7 @@ impl VisualTestContext { E: Element, { self.update(|window, cx| { - let _arena_scope = ElementArenaScope::enter(&cx.element_arena); + let arena_scope = ElementArenaScope::enter(&cx.element_arena); window.invalidator.set_phase(DrawPhase::Prepaint); let mut element = Drawable::new(f(window, cx)); @@ -914,7 +914,7 @@ impl VisualTestContext { window.refresh(); drop(element); - cx.element_arena.borrow_mut().clear(); + arena_scope.exit(&cx.element_arena).clear(cx); (request_layout_state, prepaint_state) }) diff --git a/crates/gpui/src/arena.rs b/crates/gpui/src/arena.rs index 9898c8056a..2ba7a2f605 100644 --- a/crates/gpui/src/arena.rs +++ b/crates/gpui/src/arena.rs @@ -53,11 +53,20 @@ impl Chunk { } fn allocate(&mut self, layout: alloc::Layout) -> Option> { - let aligned = unsafe { self.offset.add(self.offset.align_offset(layout.align())) }; - let next = unsafe { aligned.add(layout.size()) }; + // Compute the allocation bounds in integer address space so that the + // bounds check happens before any pointer offsetting. Offsetting a + // pointer past the end of its allocation is undefined behavior even if + // the result is never dereferenced (as happens on the chunk-spill + // path), so `ptr::add` cannot be used until we know the result stays + // in bounds. `checked_add` also handles the documented case where + // `align_offset` returns `usize::MAX`. + let base = self.offset.addr(); + let aligned_addr = base.checked_add(self.offset.align_offset(layout.align()))?; + let next_addr = aligned_addr.checked_add(layout.size())?; - if next <= self.end { - self.offset = next; + if next_addr <= self.end.addr() { + let aligned = self.offset.with_addr(aligned_addr); + self.offset = self.offset.with_addr(next_addr); NonNull::new(aligned) } else { None @@ -75,11 +84,12 @@ pub struct Arena { valid: Rc>, current_chunk_index: usize, chunk_size: NonZeroUsize, + scope_depth: usize, } impl Drop for Arena { fn drop(&mut self) { - self.clear(); + self.force_clear(); } } @@ -92,6 +102,7 @@ impl Arena { valid: Rc::new(Cell::new(true)), current_chunk_index: 0, chunk_size, + scope_depth: 0, } } @@ -99,7 +110,44 @@ impl Arena { self.chunks.len() * self.chunk_size.get() } + /// Marks the start of a scope (e.g. a window draw) whose allocations must stay + /// live until the scope ends, even if `clear` is called by a nested scope in + /// the meantime. + pub fn begin_scope(&mut self) { + self.scope_depth += 1; + } + + /// Ends the innermost scope started with `begin_scope`. + /// + /// Panics if no scope is active: an unbalanced `end_scope` would let `clear` + /// run while an enclosing scope still references arena memory, which is + /// exactly the use-after-free this bookkeeping exists to prevent, so failing + /// loudly here is preferable. + pub fn end_scope(&mut self) { + self.scope_depth = self + .scope_depth + .checked_sub(1) + .expect("Arena::end_scope called without a matching begin_scope"); + } + + /// Drops all allocations and resets the arena, unless a scope is still active. + /// + /// When a draw triggers a nested draw (e.g. re-entrant window procedure + /// invocations on Windows, or opening a window from within a draw), the nested + /// draw's clear must not free memory the outer draw still references, so it is + /// deferred: the outer draw's own clear will drop both draws' allocations. pub fn clear(&mut self) { + if self.scope_depth == 0 { + self.force_clear(); + } else { + log::debug!( + "deferring arena clear; {} enclosing scope(s) still active", + self.scope_depth + ); + } + } + + fn force_clear(&mut self) { self.valid.set(false); self.valid = Rc::new(Cell::new(true)); self.elements.clear(); @@ -286,4 +334,63 @@ mod tests { arena.clear(); let _read_value = *value; } + + #[test] + fn test_clear_deferred_while_scope_active() { + struct DropCounter(Rc>); + impl Drop for DropCounter { + fn drop(&mut self) { + self.0.set(self.0.get() + 1); + } + } + + let drops = Rc::new(Cell::new(0)); + let mut arena = Arena::new(1024); + + // Outer draw starts and allocates. + arena.begin_scope(); + let outer = arena.alloc(|| 42u64); + arena.alloc({ + let drops = drops.clone(); + || DropCounter(drops) + }); + + // Nested draw runs to completion and requests a clear. + arena.begin_scope(); + let inner = arena.alloc(|| 7u64); + arena.alloc({ + let drops = drops.clone(); + || DropCounter(drops) + }); + arena.end_scope(); + arena.clear(); + + // The clear must be deferred: the outer draw's allocations are still live. + assert_eq!(*outer, 42); + assert_eq!(*inner, 7); + assert_eq!(drops.get(), 0); + + // Once the outer draw finishes, its clear drops both draws' allocations. + arena.end_scope(); + arena.clear(); + assert_eq!(drops.get(), 2); + } + + #[test] + fn test_clear_without_scope_is_immediate() { + let mut arena = Arena::new(1024); + let value = arena.alloc(|| 1u64); + assert_eq!(*value, 1); + arena.clear(); + assert!(!value.valid.get()); + } + + #[test] + #[should_panic(expected = "Arena::end_scope called without a matching begin_scope")] + fn test_unbalanced_end_scope_panics() { + let mut arena = Arena::new(1024); + arena.begin_scope(); + arena.end_scope(); + arena.end_scope(); + } } diff --git a/crates/gpui/src/elements/div.rs b/crates/gpui/src/elements/div.rs index c02bd16b77..383d380636 100644 --- a/crates/gpui/src/elements/div.rs +++ b/crates/gpui/src/elements/div.rs @@ -4186,7 +4186,7 @@ mod tests { }); let window = AnyWindowHandle::from(window); - cx.update_window(window, |_, window, cx| window.draw(cx).clear()) + cx.update_window(window, |_, window, cx| window.draw(cx).clear(cx)) .unwrap(); assert_eq!(anonymous_paint_count.get(), 0); assert_eq!(stateful_width.get(), px(10.)); @@ -4386,7 +4386,7 @@ mod tests { test_app .update_window(any_window, |_, window, cx| { - window.draw(cx).clear(); + window.draw(cx).clear(cx); }) .unwrap(); @@ -4406,7 +4406,7 @@ mod tests { test_app .update_window(any_window, |_, window, cx| { - window.draw(cx).clear(); + window.draw(cx).clear(cx); }) .unwrap(); @@ -4578,7 +4578,7 @@ mod tests { test_app .update_window(any_window, |_, window, cx| { - window.draw(cx).clear(); + window.draw(cx).clear(cx); }) .unwrap(); @@ -4595,7 +4595,7 @@ mod tests { let _receiver = window.prompt(crate::PromptLevel::Warning, "message", None, &["Ok"], cx); assert!(window.has_active_prompt()); - window.draw(cx).clear(); + window.draw(cx).clear(cx); }) .unwrap(); @@ -4693,7 +4693,7 @@ mod tests { .unwrap(); cx.run_until_parked(); cx.update_window(window, |_, window, cx| { - window.draw(cx).clear(); + window.draw(cx).clear(cx); }) .unwrap(); } @@ -4885,7 +4885,7 @@ mod tests { } }) .into(); - cx.update_window(window, |_, window, cx| window.draw(cx).clear()) + cx.update_window(window, |_, window, cx| window.draw(cx).clear(cx)) .unwrap(); // Focus the *second* group's container, then advance like Tab would. diff --git a/crates/gpui/src/window.rs b/crates/gpui/src/window.rs index 8667d605a3..cab88b718b 100644 --- a/crates/gpui/src/window.rs +++ b/crates/gpui/src/window.rs @@ -278,6 +278,20 @@ thread_local! { static CURRENT_ELEMENT_ARENA: Cell>> = const { Cell::new(None) }; } +/// Whether a window draw is currently in progress on this thread. +/// +/// This holds exactly while an `ElementArenaScope` is active: nested scopes +/// restore the previous (still set) arena pointer, so `CURRENT_ELEMENT_ARENA` +/// is `Some` from the outermost draw's start to its end. +/// +/// The `on_request_frame` callback uses this to defer draw requests that +/// arrive re-entrantly while a draw is already on the stack (e.g. via nested +/// message pumping in the Windows window procedure), instead of running a +/// nested draw or panicking on the already-borrowed App. +fn draw_in_progress() -> bool { + CURRENT_ELEMENT_ARENA.with(|current| current.get().is_some()) +} + /// Allocates an element in the current arena. Uses the app-specific arena if one /// is active (during draw), otherwise falls back to the thread-local ELEMENT_ARENA. pub(crate) fn with_element_arena(f: impl FnOnce(&mut Arena) -> R) -> R { @@ -293,52 +307,122 @@ pub(crate) fn with_element_arena(f: impl FnOnce(&mut Arena) -> R) -> R { }) } -/// RAII guard that sets CURRENT_ELEMENT_ARENA for the duration of a draw operation. -/// When dropped, restores the previous arena (supporting nested draws). +/// Scope guard that sets CURRENT_ELEMENT_ARENA for the duration of a draw +/// operation and tracks the arena's scope depth, so that a nested draw's +/// `ArenaClearNeeded::clear` is deferred rather than freeing memory the outer +/// draw still references (see `Arena::clear`). +/// +/// Call [`ElementArenaScope::exit`] with the same arena that was entered to +/// obtain the [`ArenaClearNeeded`] token the draw now owes; requiring `exit` +/// makes it impossible to request a clear before the scope has ended. The +/// scope's teardown — restoring the thread-local and balancing `begin_scope` +/// with `end_scope` — happens in `Drop`, so the arena's scope depth stays +/// balanced on every path, including when a panic unwinds a draw before `exit` +/// is reached. (If teardown lived only in `exit`, such a panic would leave the +/// scope depth permanently elevated and defer every future clear, leaking +/// memory unboundedly.) pub(crate) struct ElementArenaScope { + /// The entered arena: compared against the argument in `exit`, and + /// dereferenced in `Drop` to end its scope (see the SAFETY note there). + entered: *const RefCell, previous: Option<*const RefCell>, + exited: bool, } impl ElementArenaScope { /// Enter a scope where element allocations use the given arena. pub(crate) fn enter(arena: &RefCell) -> Self { + arena.borrow_mut().begin_scope(); let previous = CURRENT_ELEMENT_ARENA.with(|current| { let prev = current.get(); current.set(Some(arena as *const RefCell)); prev }); - Self { previous } + Self { + entered: arena as *const RefCell, + previous, + exited: false, + } + } + + /// End the scope: restores the previously-current arena and ends the + /// arena's clear-deferral scope. Returns the token for the arena clear the + /// draw now owes; producing it here makes it impossible to request a clear + /// before the scope has ended (which would be silently deferred forever). + /// + /// Panics if passed a different arena than was entered: ending the scope + /// of the wrong arena would unbalance two arenas' scope depths, allowing + /// one of them to clear while a draw still references its memory. + pub(crate) fn exit(mut self, arena: &RefCell) -> ArenaClearNeeded { + assert!( + std::ptr::eq(self.entered, arena), + "ElementArenaScope::exit called with a different arena than was entered" + ); + self.exited = true; + // Teardown (restoring the thread-local and ending the arena's + // clear-deferral scope) runs in `Drop`, which fires both here — `self` + // is dropped as `exit` returns, before the token reaches the caller — + // and when a panic unwinds the draw before `exit` is reached. + ArenaClearNeeded::new(arena) } } impl Drop for ElementArenaScope { fn drop(&mut self) { + // Teardown lives here (rather than in `exit`) so it runs exactly once on + // every path: `exit` consumes and drops the guard on the normal path, + // and unwinding drops it on the panic path. Balancing `begin_scope` here + // keeps the arena's scope depth correct even when a draw panics; if this + // only happened in `exit`, a panic between `enter` and `exit` would leave + // the depth elevated and defer every future clear. CURRENT_ELEMENT_ARENA.with(|current| { current.set(self.previous); }); + // SAFETY: `entered` came from a `&RefCell` in `enter`, and the + // arena (owned by the `App` being drawn) outlives this guard on both the + // normal and unwinding paths, since the guard is a local of the draw. + unsafe { &*self.entered }.borrow_mut().end_scope(); + if !self.exited && !std::thread::panicking() { + debug_assert!(false, "ElementArenaScope dropped without calling exit()"); + log::error!( + "ElementArenaScope dropped without calling exit(); \ + the arena clear for this draw was never requested" + ); + } } } /// Returned when the element arena has been used and so must be cleared before the next draw. #[must_use] pub struct ArenaClearNeeded { + /// Identity of the arena that was drawn into. Only ever compared against + /// another pointer in `clear`; never dereferenced. arena: *const RefCell, } impl ArenaClearNeeded { - /// Create a new ArenaClearNeeded that will clear the given arena. - pub(crate) fn new(arena: &RefCell) -> Self { + /// Create a new ArenaClearNeeded token for the App whose arena was drawn + /// into. Private: the only way to obtain one is [`ElementArenaScope::exit`]. + fn new(arena: &RefCell) -> Self { Self { arena: arena as *const RefCell, } } - /// Clear the element arena. - pub fn clear(self) { - // SAFETY: The arena pointer is valid because ArenaClearNeeded is created - // at the end of draw() and must be cleared before the next draw. - let arena_cell = unsafe { &*self.arena }; - arena_cell.borrow_mut().clear(); + /// Clear the element arena of the App the draw ran against. If an enclosing + /// draw is still in progress (this draw was nested inside it), the clear is + /// deferred to the enclosing draw's own `ArenaClearNeeded` so that its live + /// allocations aren't freed. + /// + /// Panics if passed a different App than the draw ran against, since + /// clearing another App's arena could free memory its draws still + /// reference. + pub fn clear(self, cx: &mut App) { + assert!( + std::ptr::eq(self.arena, &cx.element_arena), + "ArenaClearNeeded::clear called with a different App than the draw ran against" + ); + cx.element_arena.borrow_mut().clear(); } } @@ -1470,7 +1554,34 @@ impl Window { let needs_present = needs_present.clone(); let next_frame_callbacks = next_frame_callbacks.clone(); let input_rate_tracker = input_rate_tracker.clone(); + let mut deferred_force_render = false; move |request_frame_options| { + // This must be checked before anything else: if this request + // arrived re-entrantly while a draw is on this thread's stack + // (e.g. via a nested message pump in the Windows window + // procedure), drawing would nest draws, and even touching the + // App would panic on its already-mutable borrow. Skip instead; + // the platform leaves the window invalidated (or re-invalidates + // it), so a fresh request arrives once the in-progress draw + // unwinds. Remember force_render so the deferred frame still + // bypasses the view cache. + // + // Returning here skips `complete_frame`, which on Wayland would + // stall the window's frame callbacks (no `surface.commit()`) — + // but calling it would hit the App borrow panic above, and this + // branch is unreachable there in practice: only Windows pumps + // platform events (and thus requests frames) mid-draw. + if draw_in_progress() { + log::debug!("deferring re-entrant window draw request"); + deferred_force_render |= request_frame_options.force_render; + return; + } + // Take the deferred flag first: `||` short-circuits, and leaving + // the flag set when this request already forces a render would + // force a second, redundant render on the next frame. + let force_render = + mem::take(&mut deferred_force_render) || request_frame_options.force_render; + let thermal_state = handle .update(&mut cx, |_, _, cx| cx.thermal_state()) .log_err(); @@ -1478,7 +1589,7 @@ impl Window { // Throttle frame rate based on conditions: // - Thermal pressure (Serious/Critical): cap to ~60fps // - Inactive window (not focused): cap to ~30fps to save energy - let min_frame_interval = if !request_frame_options.force_render + let min_frame_interval = if !force_render && !request_frame_options.require_presentation && next_frame_callbacks.borrow().is_empty() { @@ -1496,6 +1607,8 @@ impl Window { if let Some(last_frame) = last_frame_time.get() && now.duration_since(last_frame) < min_interval { + // Don't lose a pending forced render to throttling. + deferred_force_render |= force_render; // Must still complete the frame on platforms that require it. // On Wayland, `surface.frame()` was already called to request the // next frame callback, so we must call `surface.commit()` (via @@ -1526,18 +1639,18 @@ impl Window { || needs_present.get() || (active.get() && input_rate_tracker.borrow_mut().is_high_rate()); - if invalidator.is_dirty() || request_frame_options.force_render { + if invalidator.is_dirty() || force_render { measure("frame duration", || { handle .update(&mut cx, |_, window, cx| { - if request_frame_options.force_render { + if force_render { // Bypass cached view reuse so we don't replay stale // atlas tile references after a GPU device recovery. window.refresh(); } let arena_clear_needed = window.draw(cx); window.present(); - arena_clear_needed.clear(); + arena_clear_needed.clear(cx); }) .log_err(); }) @@ -2684,7 +2797,7 @@ impl Window { // Set up the per-App arena for element allocation during this draw. // This ensures that multiple test Apps have isolated arenas. - let _arena_scope = ElementArenaScope::enter(&cx.element_arena); + let arena_scope = ElementArenaScope::enter(&cx.element_arena); self.invalidate_entities(); cx.entities.clear_accessed(); @@ -2798,7 +2911,9 @@ impl Window { }); } - ArenaClearNeeded::new(&cx.element_arena) + // Exit the scope to obtain the arena-clear token this draw owes; the + // scope's teardown itself happens in `ElementArenaScope::drop`. + arena_scope.exit(&cx.element_arena) } fn record_entities_accessed(&mut self, cx: &mut App) { @@ -4873,7 +4988,7 @@ impl Window { fn dispatch_key_event(&mut self, event: &dyn Any, cx: &mut App) { if self.invalidator.is_dirty() { - self.draw(cx).clear(); + self.draw(cx).clear(cx); } let node_id = self.focus_node_id_in_rendered_frame(self.focus); @@ -6487,12 +6602,71 @@ pub fn outline( #[cfg(test)] mod tests { + use std::{cell::Cell, rc::Rc}; + use crate::{ AppContext as _, Bounds, Context, FocusHandle, InteractiveElement as _, IntoElement, - ParentElement as _, Pixels, Render, Styled as _, TestAppContext, Window, canvas, div, px, - size, + ParentElement, Pixels, Render, Styled, TestAppContext, Window, WindowOptions, canvas, div, + px, size, }; - use std::{cell::Cell, rc::Rc}; + + struct EmptyView; + + impl Render for EmptyView { + fn render(&mut self, _window: &mut Window, _cx: &mut Context) -> impl IntoElement { + div() + } + } + + struct OpensWindowOnPaint { + opened: Rc>, + } + + impl Render for OpensWindowOnPaint { + fn render(&mut self, _window: &mut Window, _cx: &mut Context) -> impl IntoElement { + let opened = self.opened.clone(); + div() + .size_full() + .child(canvas( + |_, _, _| {}, + move |_, _, _window, cx| { + if !opened.replace(true) { + cx.open_window(WindowOptions::default(), |_, cx| cx.new(|_| EmptyView)) + .unwrap(); + } + }, + )) + // Siblings painted after the canvas: their elements were + // allocated in the arena before the nested draw, so they detect + // a mid-draw arena clear when painted afterwards. + .child(div().child("after")) + } + } + + /// Opening a window synchronously draws it and requests an element arena + /// clear. When that happens from within another window's draw (here: from + /// an element's paint), the clear must be deferred until the outer draw + /// finishes, or the outer draw's arena-allocated elements would be freed + /// out from under it. + #[test] + fn test_window_opened_during_draw_defers_arena_clear() { + let mut cx = TestAppContext::single(); + + let opened = Rc::new(Cell::new(false)); + // add_window draws once, which runs the nested open_window mid-draw. + let window = cx.add_window({ + let opened = opened.clone(); + move |_, _| OpensWindowOnPaint { opened } + }); + + assert!(opened.get()); + assert_eq!(cx.windows().len(), 2); + + // The deferred clear must actually run once the outer draw unwinds: + // subsequent draws of both windows work against a fresh arena. + cx.update_window(window.into(), |_, window, cx| window.draw(cx).clear(cx)) + .unwrap(); + } struct RootView { explicit_size: bool, @@ -6531,7 +6705,7 @@ mod tests { let viewport_size = cx .update_window(window.into(), |_, window, cx| { - window.draw(cx).clear(); + window.draw(cx).clear(cx); window.viewport_size() }) .unwrap(); @@ -6552,7 +6726,7 @@ mod tests { }); cx.update_window(window.into(), |_, window, cx| { - window.draw(cx).clear(); + window.draw(cx).clear(cx); }) .unwrap(); diff --git a/crates/gpui_windows/src/events.rs b/crates/gpui_windows/src/events.rs index b293763bd9..9f54872267 100644 --- a/crates/gpui_windows/src/events.rs +++ b/crates/gpui_windows/src/events.rs @@ -1,4 +1,4 @@ -use std::{rc::Rc, sync::atomic::Ordering}; +use std::{cell::Cell, rc::Rc, sync::atomic::Ordering}; use anyhow::Context as _; use gpui_util::ResultExt; @@ -31,6 +31,52 @@ pub(crate) const WM_GPUI_KEYDOWN: u32 = WM_USER + 8; const SIZE_MOVE_LOOP_TIMER_ID: usize = 1; +/// Coordinates window draws on the UI thread. Owned by the platform and +/// shared with every window (like `WindowsPlatformState::cursor_visible`), +/// because the coordination is inherently cross-window: while window A is +/// drawing, a re-entrant paint request for window B must be deferred. +pub(crate) struct DrawCoordinator { + /// Whether some window is currently inside `draw_window`. Win32 can + /// re-enter the window procedure while a draw is in progress (e.g. + /// cross-thread `SendMessage` dispatch during message pumping, or modal + /// message loops entered by COM calls), and drawing re-entrantly would + /// nest GPUI draws. Nested draws are wasted work whose output is + /// immediately redrawn, so we defer them instead. + drawing: Cell, +} + +impl DrawCoordinator { + pub(crate) fn new() -> Self { + Self { + drawing: Cell::new(false), + } + } + + fn try_begin_draw(&self) -> Option> { + // This only covers the `draw_window` span (which extends past the GPUI + // draw, through presentation and IME updates). Requests that arrive + // re-entrantly during GPUI-initiated draws (e.g. key dispatch or + // opening a window draws synchronously) are deferred by GPUI's + // `on_request_frame` callback itself, which no-ops in that case. + if self.drawing.get() { + None + } else { + self.drawing.set(true); + Some(DrawWindowGuard { coordinator: self }) + } + } +} + +struct DrawWindowGuard<'a> { + coordinator: &'a DrawCoordinator, +} + +impl Drop for DrawWindowGuard<'_> { + fn drop(&mut self) { + self.coordinator.drawing.set(false); + } +} + impl WindowsWindowInner { pub(crate) fn handle_msg( self: &Rc, @@ -1214,7 +1260,7 @@ impl WindowsWindowInner { // from the forced WM_GPUI_FORCE_UPDATE_WINDOW or a stray WM_PAINT in // between) is treated as a forced render so it both clears // `skip_draws` and bypasses the view cache. - self.state.force_render_after_recovery.set(true); + self.state.force_render_pending.set(true); Some(0) } @@ -1225,6 +1271,20 @@ impl WindowsWindowInner { #[inline] fn draw_window(&self, handle: HWND, force_render: bool) -> Option { + let Some(_guard) = self.state.draw_coordinator.try_begin_draw() else { + log::debug!("deferring re-entrant draw of window {handle:?}"); + if force_render { + self.state.force_render_pending.set(true); + } + // Validate the region so a nested message pump doesn't keep + // re-dispatching WM_PAINT for the still-invalid region in a busy + // loop until the in-progress draw unwinds. The vsync thread + // re-invalidates every window on each vsync (see + // `begin_vsync_thread`), so the deferred frame still gets drawn, + // at most one vsync late. + unsafe { ValidateRect(Some(handle), None).ok().log_err() }; + return Some(0); + }; let mut request_frame = self.state.callbacks.request_frame.take()?; self.state.direct_manipulation.update(); @@ -1239,7 +1299,7 @@ impl WindowsWindowInner { } } - let force_render = force_render || self.state.force_render_after_recovery.take(); + let force_render = force_render || self.state.force_render_pending.take(); if force_render { // Re-enable drawing after a device loss recovery. The forced render // will rebuild the scene with fresh atlas textures. diff --git a/crates/gpui_windows/src/platform.rs b/crates/gpui_windows/src/platform.rs index f8939e9e4c..d3bb150407 100644 --- a/crates/gpui_windows/src/platform.rs +++ b/crates/gpui_windows/src/platform.rs @@ -69,6 +69,9 @@ pub(crate) struct WindowsPlatformState { pub(crate) current_cursor: Cell>, /// Shared with each window so `WM_SETCURSOR` can read it directly. pub(crate) cursor_visible: Arc, + /// Shared with each window to coordinate draws across windows on the UI + /// thread; see [`DrawCoordinator`]. + pub(crate) draw_coordinator: Rc, directx_devices: RefCell>, } @@ -95,6 +98,7 @@ impl WindowsPlatformState { jump_list: RefCell::new(jump_list), current_cursor: Cell::new(current_cursor), cursor_visible: Arc::new(AtomicBool::new(true)), + draw_coordinator: Rc::new(DrawCoordinator::new()), directx_devices: RefCell::new(directx_devices), menus: RefCell::new(Vec::new()), } @@ -239,6 +243,7 @@ impl WindowsPlatform { disable_direct_composition: self.disable_direct_composition, directx_devices: self.inner.state.directx_devices.borrow().clone().unwrap(), invalidate_devices: self.invalidate_devices.clone(), + draw_coordinator: self.inner.state.draw_coordinator.clone(), } } @@ -1132,6 +1137,8 @@ pub(crate) struct WindowCreationInfo { /// Flag to instruct the `VSyncProvider` thread to invalidate the directx devices /// as resizing them has failed, causing us to have lost at least the render target. pub(crate) invalidate_devices: Arc, + /// Shared with [`WindowsPlatformState::draw_coordinator`] and every other window. + pub(crate) draw_coordinator: Rc, } struct PlatformWindowCreateContext { diff --git a/crates/gpui_windows/src/window.rs b/crates/gpui_windows/src/window.rs index 49a935bafe..a8ee1bbb13 100644 --- a/crates/gpui_windows/src/window.rs +++ b/crates/gpui_windows/src/window.rs @@ -63,12 +63,14 @@ pub struct WindowsWindowState { pub direct_manipulation: DirectManipulationHandler, pub renderer: RefCell, - /// Set after a GPU device-lost recovery so the next `draw_window` call is - /// treated as a forced render. This guarantees the next frame both - /// re-enables drawing (via `mark_drawable`) and bypasses the GPUI view - /// cache, which would otherwise replay stale atlas tile references from - /// the previous frame and panic in `DirectXAtlasState::texture`. - pub force_render_after_recovery: Cell, + /// Set when the next `draw_window` call must be treated as a forced + /// render. Used after a GPU device-lost recovery, where the next frame + /// must both re-enable drawing (via `mark_drawable`) and bypass the GPUI + /// view cache (which would otherwise replay stale atlas tile references + /// from the previous frame and panic in `DirectXAtlasState::texture`), + /// and when a forced render was requested while another draw was in + /// progress and had to be deferred. + pub force_render_pending: Cell, pub click_state: ClickState, pub current_cursor: Cell>, @@ -80,6 +82,8 @@ pub struct WindowsWindowState { /// Flag to instruct the `VSyncProvider` thread to invalidate the directx devices /// as resizing them has failed, causing us to have lost at least the render target. pub invalidate_devices: Arc, + /// Shared with [`WindowsPlatformState::draw_coordinator`] and every other window. + pub(crate) draw_coordinator: Rc, fullscreen: Cell>, initial_placement: Cell>, hwnd: HWND, @@ -115,6 +119,7 @@ impl WindowsWindowState { appearance: WindowAppearance, disable_direct_composition: bool, invalidate_devices: Arc, + draw_coordinator: Rc, ) -> Result { let scale_factor = { let monitor_dpi = unsafe { GetDpiForWindow(hwnd) } as f32; @@ -168,7 +173,7 @@ impl WindowsWindowState { last_reported_capslock: Cell::new(last_reported_capslock), hovered: Cell::new(hovered), renderer: RefCell::new(renderer), - force_render_after_recovery: Cell::new(false), + force_render_pending: Cell::new(false), click_state, current_cursor: Cell::new(current_cursor), cursor_visible, @@ -178,6 +183,7 @@ impl WindowsWindowState { initial_placement: Cell::new(initial_placement), hwnd, invalidate_devices, + draw_coordinator, direct_manipulation, a11y: RefCell::new(None), }) @@ -255,6 +261,7 @@ impl WindowsWindowInner { context.appearance, context.disable_direct_composition, context.invalidate_devices.clone(), + context.draw_coordinator.clone(), )?; Ok(Rc::new(Self { @@ -402,6 +409,7 @@ struct WindowCreateContext { disable_direct_composition: bool, directx_devices: DirectXDevices, invalidate_devices: Arc, + draw_coordinator: Rc, parent_hwnd: Option, } @@ -429,6 +437,7 @@ impl WindowsWindow { disable_direct_composition, directx_devices, invalidate_devices, + draw_coordinator, } = creation_info; register_window_class(icon); let parent_hwnd = if params.kind == WindowKind::Dialog { @@ -513,6 +522,7 @@ impl WindowsWindow { disable_direct_composition, directx_devices, invalidate_devices, + draw_coordinator, parent_hwnd, }; let creation_result = unsafe { diff --git a/crates/recent_projects/src/recent_projects.rs b/crates/recent_projects/src/recent_projects.rs index 8b82c2dd60..94d72ee14f 100644 --- a/crates/recent_projects/src/recent_projects.rs +++ b/crates/recent_projects/src/recent_projects.rs @@ -2560,7 +2560,7 @@ mod tests { } fn draw(cx: &mut VisualTestContext) { - cx.update(|window, cx| window.draw(cx).clear()); + cx.update(|window, cx| window.draw(cx).clear(cx)); } fn build_picker( diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index 879d0201fc..64daf89945 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -3478,7 +3478,7 @@ impl Workspace { // (Note that the tests always do this implicitly, so you must manually test with something like: // "bindings": { "g z": ["workspace::SendKeystrokes", ": j u"]} // ) - window.draw(cx).clear(); + window.draw(cx).clear(cx); return true; } false diff --git a/crates/zed/src/visual_test_runner.rs b/crates/zed/src/visual_test_runner.rs index bbcd909315..cea858e72e 100644 --- a/crates/zed/src/visual_test_runner.rs +++ b/crates/zed/src/visual_test_runner.rs @@ -1161,7 +1161,7 @@ fn run_breakpoint_hover_visual_tests( // Step 1: Initial draw to register mouse listeners cx.update_window(workspace_window.into(), |_, window, cx| { - window.draw(cx).clear(); + window.draw(cx).clear(cx); })?; cx.run_until_parked(); @@ -1179,7 +1179,7 @@ fn run_breakpoint_hover_visual_tests( // Step 4: Draw again to pick up the indicator state change cx.update_window(workspace_window.into(), |_, window, cx| { - window.draw(cx).clear(); + window.draw(cx).clear(cx); })?; cx.run_until_parked(); @@ -1193,7 +1193,7 @@ fn run_breakpoint_hover_visual_tests( // Step 6: Final draw cx.update_window(workspace_window.into(), |_, window, cx| { - window.draw(cx).clear(); + window.draw(cx).clear(cx); })?; cx.run_until_parked(); @@ -1219,7 +1219,7 @@ fn run_breakpoint_hover_visual_tests( // Draw to register the button's tooltip hover listener cx.update_window(workspace_window.into(), |_, window, cx| { - window.draw(cx).clear(); + window.draw(cx).clear(cx); })?; cx.run_until_parked(); @@ -1237,7 +1237,7 @@ fn run_breakpoint_hover_visual_tests( // Draw to render the tooltip cx.update_window(workspace_window.into(), |_, window, cx| { - window.draw(cx).clear(); + window.draw(cx).clear(cx); })?; cx.run_until_parked(); @@ -2457,7 +2457,7 @@ fn run_tool_permissions_visual_tests( // Refresh and redraw so the "Test Your Rules" input is present cx.update_window(settings_window, |_, window, cx| { - window.draw(cx).clear(); + window.draw(cx).clear(cx); }) .log_err(); cx.run_until_parked(); @@ -2486,7 +2486,7 @@ fn run_tool_permissions_visual_tests( // Refresh and redraw cx.update_window(settings_window, |_, window, cx| { - window.draw(cx).clear(); + window.draw(cx).clear(cx); }) .log_err(); cx.run_until_parked(); diff --git a/crates/zed/src/zed.rs b/crates/zed/src/zed.rs index 09e968308c..19fe94d3ef 100644 --- a/crates/zed/src/zed.rs +++ b/crates/zed/src/zed.rs @@ -4539,7 +4539,7 @@ mod tests { .unwrap(); cx.update(|window, cx| { - window.draw(cx).clear(); + window.draw(cx).clear(cx); }); // mouse_wheel_zoom is disabled by default — zoom should not work. @@ -4571,7 +4571,7 @@ mod tests { }); cx.update(|window, cx| { - window.draw(cx).clear(); + window.draw(cx).clear(cx); }); cx.simulate_event(gpui::ScrollWheelEvent { @@ -4590,7 +4590,7 @@ mod tests { ); cx.update(|window, cx| { - window.draw(cx).clear(); + window.draw(cx).clear(cx); }); cx.simulate_event(gpui::ScrollWheelEvent { @@ -4621,7 +4621,7 @@ mod tests { cx.update(|_, cx| ThemeSettings::get_global(cx).buffer_font_size(cx).as_f32()); cx.update(|window, cx| { - window.draw(cx).clear(); + window.draw(cx).clear(cx); }); cx.simulate_event(gpui::ScrollWheelEvent {