mirror of
https://github.com/zed-industries/zed.git
synced 2026-08-21 15:04:51 +00:00
gpui: Fix crash when opening a popover from inside another popover (#60926)
Opening a popover from inside another popover (e.g. the branch picker's new filter menu, when the picker is hosted in the git panel or title bar) corrupted the frame-caching bookkeeping for whichever cached view contained them. The corruption was invisible until a later, unrelated redraw reused that view's cached subtree, at which point the window crashed (debug) or could misroute input events (release). Fixed by processing deferred draws in place so their recorded positions remain valid across frames; added a regression test reproducing the crash via a cached panel hosting nested popovers. Release Notes: - Fixed the branch picker menu in the Git Panel, which was previously broken due to popovers-within-popovers not being possible in GPUI.
This commit is contained in:
Danilo Leal
GitHub
parent
7cf50a771f
commit
5e982c6bdc
2 changed files with 155 additions and 36 deletions
|
|
@ -94,3 +94,113 @@ impl Deferred {
|
|||
self
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use crate::{
|
||||
Context, Entity, StyleRefinement, TestAppContext, Window, anchored, deferred, div, point,
|
||||
prelude::*, px, size,
|
||||
};
|
||||
|
||||
/// A stand-in for a dock panel hosting a popover (deferred draw) whose
|
||||
/// content opens another popover (a deferred draw created while
|
||||
/// prepainting the first one's content).
|
||||
struct PanelView;
|
||||
|
||||
impl Render for PanelView {
|
||||
fn render(&mut self, _window: &mut Window, _cx: &mut Context<Self>) -> impl IntoElement {
|
||||
div().key_context("Panel").size_full().child(
|
||||
deferred(
|
||||
anchored().position(point(px(10.), px(10.))).child(
|
||||
div().key_context("Popover").w(px(200.)).h(px(200.)).child(
|
||||
deferred(
|
||||
anchored().position(point(px(30.), px(30.))).child(
|
||||
div()
|
||||
.key_context("NestedMenu")
|
||||
.debug_selector(|| "NESTED_MENU".into())
|
||||
.w(px(50.))
|
||||
.h(px(50.)),
|
||||
),
|
||||
)
|
||||
.with_priority(2),
|
||||
),
|
||||
),
|
||||
)
|
||||
.with_priority(1),
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
struct RootView {
|
||||
panel: Entity<PanelView>,
|
||||
}
|
||||
|
||||
impl Render for RootView {
|
||||
fn render(&mut self, _window: &mut Window, _cx: &mut Context<Self>) -> impl IntoElement {
|
||||
div().key_context("Root").size_full().child(
|
||||
self.panel
|
||||
.clone()
|
||||
.cached(StyleRefinement::default().size_full()),
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/// Regression test for a crash with nested deferred draws (e.g. a popover
|
||||
/// menu inside a popover hosted by a cached dock panel). Prepaint indices
|
||||
/// recorded during the deferred draw rounds must index the same
|
||||
/// `deferred_draws` vector that `reuse_prepaint` slices on the next frame;
|
||||
/// previously they were measured against a transient per-round vector, so
|
||||
/// reusing the panel's subtree grafted the wrong deferred draws and
|
||||
/// panicked in the dispatch tree.
|
||||
#[gpui::test]
|
||||
fn test_nested_deferred_draws_with_reused_views(cx: &mut TestAppContext) {
|
||||
let window = cx.open_window(size(px(800.), px(600.)), |_, cx| {
|
||||
let panel = cx.new(|_| PanelView);
|
||||
RootView { panel }
|
||||
});
|
||||
cx.run_until_parked();
|
||||
|
||||
let menu_bounds = window
|
||||
.update(cx, |_, window, _| {
|
||||
window
|
||||
.rendered_frame
|
||||
.debug_bounds
|
||||
.get("NESTED_MENU")
|
||||
.copied()
|
||||
})
|
||||
.unwrap()
|
||||
.expect("NESTED_MENU debug bounds not found");
|
||||
assert_eq!(menu_bounds.size, size(px(50.), px(50.)));
|
||||
|
||||
// Re-render only the root view; the panel is cached, so its subtree -
|
||||
// including both deferred draw records - is reused from the previous
|
||||
// frame.
|
||||
window.update(cx, |_, _, cx| cx.notify()).unwrap();
|
||||
cx.run_until_parked();
|
||||
|
||||
// Reuse the subtree a second time, exercising ranges that were
|
||||
// themselves recorded during a reused frame.
|
||||
window.update(cx, |_, _, cx| cx.notify()).unwrap();
|
||||
cx.run_until_parked();
|
||||
|
||||
// Re-render the panel itself again to prove the popovers still draw.
|
||||
window
|
||||
.update(cx, |root, _, cx| {
|
||||
root.panel.update(cx, |_, cx| cx.notify());
|
||||
})
|
||||
.unwrap();
|
||||
cx.run_until_parked();
|
||||
|
||||
window
|
||||
.update(cx, |_, window, _| {
|
||||
assert_eq!(window.rendered_frame.deferred_draws.len(), 2);
|
||||
assert!(
|
||||
window
|
||||
.rendered_frame
|
||||
.debug_bounds
|
||||
.contains_key("NESTED_MENU")
|
||||
);
|
||||
})
|
||||
.unwrap();
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -2988,62 +2988,71 @@ impl Window {
|
|||
fn prepaint_deferred_draws(&mut self, cx: &mut App) {
|
||||
assert_eq!(self.element_id_stack.len(), 0);
|
||||
|
||||
let mut completed_draws = Vec::new();
|
||||
|
||||
// Process deferred draws in multiple rounds to support nesting.
|
||||
// Each round processes all current deferred draws, which may produce new ones.
|
||||
// Each round processes all current deferred draws, which may push new ones.
|
||||
//
|
||||
// The draws are processed in place rather than being moved out of
|
||||
// `next_frame.deferred_draws`: `prepaint_index` snapshots that vector's
|
||||
// length, so any prepaint range recorded during a round (view caches,
|
||||
// nested deferred draws) must index the same vector `reuse_prepaint`
|
||||
// slices on the next frame. Moving the draws out and re-appending them
|
||||
// shifts the indices of nested draws, causing reused subtrees to graft
|
||||
// the wrong deferred draws and panic in the dispatch tree.
|
||||
let mut round_start = 0;
|
||||
let mut depth = 0;
|
||||
loop {
|
||||
let round_end = self.next_frame.deferred_draws.len();
|
||||
if round_start == round_end {
|
||||
break;
|
||||
}
|
||||
// Limit maximum nesting depth to prevent infinite loops.
|
||||
assert!(depth < 10, "Exceeded maximum (10) deferred depth");
|
||||
depth += 1;
|
||||
let deferred_count = self.next_frame.deferred_draws.len();
|
||||
if deferred_count == 0 {
|
||||
break;
|
||||
}
|
||||
|
||||
// Sort by priority for this round
|
||||
let traversal_order = self.deferred_draw_traversal_order();
|
||||
let mut deferred_draws = mem::take(&mut self.next_frame.deferred_draws);
|
||||
// Sort this round by priority.
|
||||
let mut traversal_order = (round_start..round_end).collect::<SmallVec<[usize; 8]>>();
|
||||
traversal_order.sort_by_key(|ix| self.next_frame.deferred_draws[*ix].priority);
|
||||
|
||||
for deferred_draw_ix in traversal_order {
|
||||
let deferred_draw = &mut deferred_draws[deferred_draw_ix];
|
||||
self.element_id_stack
|
||||
.clone_from(&deferred_draw.element_id_stack);
|
||||
self.text_style_stack
|
||||
.clone_from(&deferred_draw.text_style_stack);
|
||||
self.next_frame
|
||||
.dispatch_tree
|
||||
.set_active_node(deferred_draw.parent_node);
|
||||
let (element, parent_node, current_view, rem_size, absolute_offset, prepaint_range) = {
|
||||
let deferred_draw = &mut self.next_frame.deferred_draws[deferred_draw_ix];
|
||||
self.element_id_stack
|
||||
.clone_from(&deferred_draw.element_id_stack);
|
||||
self.text_style_stack
|
||||
.clone_from(&deferred_draw.text_style_stack);
|
||||
(
|
||||
deferred_draw.element.take(),
|
||||
deferred_draw.parent_node,
|
||||
deferred_draw.current_view,
|
||||
deferred_draw.rem_size,
|
||||
deferred_draw.absolute_offset,
|
||||
deferred_draw.prepaint_range.clone(),
|
||||
)
|
||||
};
|
||||
self.next_frame.dispatch_tree.set_active_node(parent_node);
|
||||
|
||||
let prepaint_start = self.prepaint_index();
|
||||
if let Some(element) = deferred_draw.element.as_mut() {
|
||||
self.with_rendered_view(deferred_draw.current_view, |window| {
|
||||
window.with_rem_size(Some(deferred_draw.rem_size), |window| {
|
||||
window.with_absolute_element_offset(
|
||||
deferred_draw.absolute_offset,
|
||||
|window| {
|
||||
element.prepaint(window, cx);
|
||||
},
|
||||
);
|
||||
if let Some(mut element) = element {
|
||||
self.with_rendered_view(current_view, |window| {
|
||||
window.with_rem_size(Some(rem_size), |window| {
|
||||
window.with_absolute_element_offset(absolute_offset, |window| {
|
||||
element.prepaint(window, cx);
|
||||
});
|
||||
});
|
||||
})
|
||||
});
|
||||
self.next_frame.deferred_draws[deferred_draw_ix].element = Some(element);
|
||||
} else {
|
||||
self.reuse_prepaint(deferred_draw.prepaint_range.clone());
|
||||
self.reuse_prepaint(prepaint_range);
|
||||
}
|
||||
let prepaint_end = self.prepaint_index();
|
||||
deferred_draw.prepaint_range = prepaint_start..prepaint_end;
|
||||
self.next_frame.deferred_draws[deferred_draw_ix].prepaint_range =
|
||||
prepaint_start..prepaint_end;
|
||||
}
|
||||
|
||||
// Save completed draws and continue with newly added ones
|
||||
completed_draws.append(&mut deferred_draws);
|
||||
|
||||
self.element_id_stack.clear();
|
||||
self.text_style_stack.clear();
|
||||
round_start = round_end;
|
||||
}
|
||||
|
||||
// Restore all completed draws
|
||||
self.next_frame.deferred_draws = completed_draws;
|
||||
}
|
||||
|
||||
fn paint_deferred_draws(&mut self, cx: &mut App) {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue