gpui: Defer element arena clears while an enclosing draw is in progress (#60295)
GPUI allocates every element for a draw in a single per-App bump arena, and `Arena::clear()` assumed no draw was in progress. But draws can nest: on Windows, the window procedure re-enters whenever the main thread pumps messages mid-draw (cross-thread `SendMessage` dispatch, modal message loops entered by COM/OLE calls such as clipboard reads), and on any platform a draw can be triggered from within another draw (e.g. `open_window`). When a nested draw finished, its arena clear freed and rewound memory the outer draw was still using. The `ArenaBox` validity flag only guards new derefs, so `&mut` references already held by the outer draw silently pointed into reused memory. That use-after-free corrupted element-state keys (`GlobalElementId`/`SharedString` Arcs) and heap metadata, crashing later in innocent-looking frames — seen in the wild as `EXCEPTION_ACCESS_VIOLATION_READ / 0xffffffffffffffff` in `Frame::finish` and element allocation ([ZED-9QN](https://zed-dev.sentry.io/issues/7577818040/), [ZED-7JC](https://sentry.io/organizations/zed-dev/issues/7465778926/), [ZED-7C6](https://zed-dev.sentry.io/issues/7460258444/)), plus occasional "attempted to dereference an ArenaRef after its Arena was cleared" panics (ZED-96P, ZED-8XN), Windows-dominant and spanning versions 1.1.7–1.8.2. The fix makes the arena sound under nesting: `ElementArenaScope` tracks a scope depth on the arena, and `Arena::clear()` is deferred while any scope is active — the outermost draw's clear drops both draws' allocations. Since arena chunks are stable heap blocks and allocation only appends, nested allocation was already safe; the mid-draw clear was the only destructive operation. Scopes are ended via a consuming `exit(arena)` call that asserts arena identity (by pointer comparison, never dereferencing), and `ArenaClearNeeded::clear(cx)` reaches the arena through the App, so this bookkeeping contains no unsafe code beyond what existed before. A panic that unwinds a draw balances the scope depth in the guard's `Drop`, so later clears still run; only the unwound draw's own clear token is never produced, leaving the arena populated until the next draw's clear — a one-frame leak at worst, never a use-after-free. Additionally, draws are no longer run re-entrantly: GPUI's `on_request_frame` callback skips requests that arrive while a draw is already on the thread's stack (remembering `force_render` for the next frame), and on Windows a `DrawCoordinator` owned by the platform and shared with every window additionally guards the wider `draw_window` span (presentation, IME updates). Deferred windows validate their update region (so nested message pumps don't busy-loop on WM_PAINT) and are repainted at most one vsync later by the vsync thread's existing per-tick invalidation. An integration test opens a window from within an element's paint; without the deferred clear it reproduces the exact "ArenaRef after its Arena was cleared" panic seen in the wild. Deferrals are logged so the diagnosis can be confirmed from user logs. Closes FR-110 Closes FR-114 Release Notes: - Fixed a crash on Windows caused by re-entrant window drawing corrupting UI element memory.
This commit is contained in:
1 parent
9cbae806f2
commit
396cdc37c1
13 files changed
+421
-63
No files matched your search
@@ -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::<Vec<_>>()
|
||||
{
|
||||
self.update_window(window, |_, window, cx| window.draw(cx).clear())
|
||||
self.update_window(window, |_, window, cx| window.draw(cx).clear(cx))
|
||||
.unwrap();
|
||||
}
|
||||
|
||||
|
||||
@@ -486,7 +486,7 @@ impl<V: 'static + Render> TestAppWindow<V> {
|
||||
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();
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
|
||||
+112
-5
@@ -53,11 +53,20 @@ impl Chunk {
|
||||
}
|
||||
|
||||
fn allocate(&mut self, layout: alloc::Layout) -> Option<NonNull<u8>> {
|
||||
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<Cell<bool>>,
|
||||
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<Cell<usize>>);
|
||||
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();
|
||||
}
|
||||
}
|
||||
@@ -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.
|
||||
|
||||
+197
-23
@@ -278,6 +278,20 @@ thread_local! {
|
||||
static CURRENT_ELEMENT_ARENA: Cell<Option<*const RefCell<Arena>>> = 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<R>(f: impl FnOnce(&mut Arena) -> R) -> R {
|
||||
@@ -293,52 +307,122 @@ pub(crate) fn with_element_arena<R>(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<Arena>,
|
||||
previous: Option<*const RefCell<Arena>>,
|
||||
exited: bool,
|
||||
}
|
||||
|
||||
impl ElementArenaScope {
|
||||
/// Enter a scope where element allocations use the given arena.
|
||||
pub(crate) fn enter(arena: &RefCell<Arena>) -> Self {
|
||||
arena.borrow_mut().begin_scope();
|
||||
let previous = CURRENT_ELEMENT_ARENA.with(|current| {
|
||||
let prev = current.get();
|
||||
current.set(Some(arena as *const RefCell<Arena>));
|
||||
prev
|
||||
});
|
||||
Self { previous }
|
||||
Self {
|
||||
entered: arena as *const RefCell<Arena>,
|
||||
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<Arena>) -> 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<Arena>` 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<Arena>,
|
||||
}
|
||||
|
||||
impl ArenaClearNeeded {
|
||||
/// Create a new ArenaClearNeeded that will clear the given arena.
|
||||
pub(crate) fn new(arena: &RefCell<Arena>) -> 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<Arena>) -> Self {
|
||||
Self {
|
||||
arena: arena as *const RefCell<Arena>,
|
||||
}
|
||||
}
|
||||
|
||||
/// 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<Self>) -> impl IntoElement {
|
||||
div()
|
||||
}
|
||||
}
|
||||
|
||||
struct OpensWindowOnPaint {
|
||||
opened: Rc<Cell<bool>>,
|
||||
}
|
||||
|
||||
impl Render for OpensWindowOnPaint {
|
||||
fn render(&mut self, _window: &mut Window, _cx: &mut Context<Self>) -> 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();
|
||||
|
||||
|
||||
@@ -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<bool>,
|
||||
}
|
||||
|
||||
impl DrawCoordinator {
|
||||
pub(crate) fn new() -> Self {
|
||||
Self {
|
||||
drawing: Cell::new(false),
|
||||
}
|
||||
}
|
||||
|
||||
fn try_begin_draw(&self) -> Option<DrawWindowGuard<'_>> {
|
||||
// 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<Self>,
|
||||
@@ -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<isize> {
|
||||
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.
|
||||
|
||||
@@ -69,6 +69,9 @@ pub(crate) struct WindowsPlatformState {
|
||||
pub(crate) current_cursor: Cell<Option<HCURSOR>>,
|
||||
/// Shared with each window so `WM_SETCURSOR` can read it directly.
|
||||
pub(crate) cursor_visible: Arc<AtomicBool>,
|
||||
/// Shared with each window to coordinate draws across windows on the UI
|
||||
/// thread; see [`DrawCoordinator`].
|
||||
pub(crate) draw_coordinator: Rc<DrawCoordinator>,
|
||||
directx_devices: RefCell<Option<DirectXDevices>>,
|
||||
}
|
||||
|
||||
@@ -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<AtomicBool>,
|
||||
/// Shared with [`WindowsPlatformState::draw_coordinator`] and every other window.
|
||||
pub(crate) draw_coordinator: Rc<DrawCoordinator>,
|
||||
}
|
||||
|
||||
struct PlatformWindowCreateContext {
|
||||
|
||||
@@ -63,12 +63,14 @@ pub struct WindowsWindowState {
|
||||
pub direct_manipulation: DirectManipulationHandler,
|
||||
|
||||
pub renderer: RefCell<DirectXRenderer>,
|
||||
/// 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<bool>,
|
||||
/// 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<bool>,
|
||||
|
||||
pub click_state: ClickState,
|
||||
pub current_cursor: Cell<Option<HCURSOR>>,
|
||||
@@ -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<AtomicBool>,
|
||||
/// Shared with [`WindowsPlatformState::draw_coordinator`] and every other window.
|
||||
pub(crate) draw_coordinator: Rc<DrawCoordinator>,
|
||||
fullscreen: Cell<Option<StyleAndBounds>>,
|
||||
initial_placement: Cell<Option<WindowOpenStatus>>,
|
||||
hwnd: HWND,
|
||||
@@ -115,6 +119,7 @@ impl WindowsWindowState {
|
||||
appearance: WindowAppearance,
|
||||
disable_direct_composition: bool,
|
||||
invalidate_devices: Arc<AtomicBool>,
|
||||
draw_coordinator: Rc<DrawCoordinator>,
|
||||
) -> Result<Self> {
|
||||
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<AtomicBool>,
|
||||
draw_coordinator: Rc<DrawCoordinator>,
|
||||
parent_hwnd: Option<HWND>,
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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 <enter> u"]}
|
||||
// )
|
||||
window.draw(cx).clear();
|
||||
window.draw(cx).clear(cx);
|
||||
return true;
|
||||
}
|
||||
false
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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 {
|
||||
|
||||
Reference in new issue
Block a user