Skip to content

Commit

Permalink
Fix double read panic in nav history (#22754)
Browse files Browse the repository at this point in the history
This one seems to be triggered when the assistant's
`View<ContextEditor>` is leased during the call into
`NavHistory::for_each_entry`, which then tries to read it again through
the `ItemHandle` interface. Fix it by skipping entries that can't be
read in the history iteration.

Release Notes:

- N/A
  • Loading branch information
cole-miller authored Jan 8, 2025
1 parent ef583e6 commit 1d8bd15
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 4 deletions.
4 changes: 4 additions & 0 deletions crates/assistant/src/assistant_panel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4272,6 +4272,10 @@ impl Item for ContextEditor {
None
}
}

fn include_in_nav_history() -> bool {
false
}
}

impl SearchableItem for ContextEditor {
Expand Down
9 changes: 9 additions & 0 deletions crates/workspace/src/item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,10 @@ pub trait Item: FocusableView + EventEmitter<Self::Event> {
fn preserve_preview(&self, _cx: &AppContext) -> bool {
false
}

fn include_in_nav_history() -> bool {
true
}
}

pub trait SerializableItem: Item {
Expand Down Expand Up @@ -464,6 +468,7 @@ pub trait ItemHandle: 'static + Send {
fn downgrade_item(&self) -> Box<dyn WeakItemHandle>;
fn workspace_settings<'a>(&self, cx: &'a AppContext) -> &'a WorkspaceSettings;
fn preserve_preview(&self, cx: &AppContext) -> bool;
fn include_in_nav_history(&self) -> bool;
}

pub trait WeakItemHandle: Send + Sync {
Expand Down Expand Up @@ -877,6 +882,10 @@ impl<T: Item> ItemHandle for View<T> {
fn preserve_preview(&self, cx: &AppContext) -> bool {
self.read(cx).preserve_preview(cx)
}

fn include_in_nav_history(&self) -> bool {
T::include_in_nav_history()
}
}

impl From<Box<dyn ItemHandle>> for AnyView {
Expand Down
10 changes: 8 additions & 2 deletions crates/workspace/src/pane.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3095,8 +3095,14 @@ impl Render for Pane {

impl ItemNavHistory {
pub fn push<D: 'static + Send + Any>(&mut self, data: Option<D>, cx: &mut WindowContext) {
self.history
.push(data, self.item.clone(), self.is_preview, cx);
if self
.item
.upgrade()
.is_some_and(|item| item.include_in_nav_history())
{
self.history
.push(data, self.item.clone(), self.is_preview, cx);
}
}

pub fn pop_backward(&mut self, cx: &mut WindowContext) -> Option<NavigationEntry> {
Expand Down
6 changes: 4 additions & 2 deletions crates/workspace/src/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2975,13 +2975,15 @@ impl Workspace {
match target {
Some(ActivateInDirectionTarget::Pane(pane)) => cx.focus_view(&pane),
Some(ActivateInDirectionTarget::Dock(dock)) => {
dock.update(cx, |dock, cx| {
// Defer this to avoid a panic when the dock's active panel is already on the stack.
cx.defer(move |cx| {
let dock = dock.read(cx);
if let Some(panel) = dock.active_panel() {
panel.focus_handle(cx).focus(cx);
} else {
log::error!("Could not find a focus target when in switching focus in {direction} direction for a {:?} dock", dock.position());
}
});
})
}
None => {}
}
Expand Down

0 comments on commit 1d8bd15

Please sign in to comment.