From 050e6f3407e0a003ca45a572b8d4132cef5b6dfd Mon Sep 17 00:00:00 2001 From: Yevhen Nakonechnyi Date: Mon, 6 Jul 2026 06:47:21 -0700 Subject: [PATCH] text_finder: Fix crash when dismissing the finder (#60437) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Objective Fixes #60436 Pressing Escape (or triggering any workspace-level action that closes modals, such as `pane::DeploySearch` / `search::NewSearch`) while the Text Finder was open crashes the app with: ``` cannot read workspace::Workspace while it is already being updated crates/gpui/src/app/entity_map.rs:164 ``` `TextFinder::on_before_dismiss` read the `Workspace` entity (`workspace.read(cx).database_id()`) to persist the last search query. Dismissal can be initiated from inside a `workspace.register_action` handler — e.g. `buffer_search`'s `SearchActionsRegistrar` calls `workspace.hide_modal(...)` while the `Workspace` entity is leased — so the modal layer invokes `on_before_dismiss` synchronously under that lease, and the read trips GPUI's re-entrancy guard. Since the finder seeds the last query on open, the query is non-empty immediately, making the crash reproducible on the very first Escape. Dump file analysis confirms the diagnosis. ## Solution Stop reading the `Workspace` entity in the dismiss path. Both places that create the modal (`TextFinder::open` and `TextFinder::open_from_project_search`) already run inside `workspace.update_in`, where `workspace.database_id()` is a plain field access on `&mut Workspace`. Capture the `Option` there, store it on `TextFinder`, and have `on_before_dismiss` use the stored id. The dismiss path now touches no entity that can be mid-update, so it is safe regardless of which code path initiates `hide_modal`. The remaining reads in `on_before_dismiss` (`picker` and its query editor) are never leased in any dismissal chain. The `id` is captured once at creation; a workspace that gains a database id during the modal's lifetime would persist under `None` (i.e. skip persistence), which matches the previous behavior for unpersisted workspaces. ## Testing - Verified the fix by code review and re-tested in a rebuilt bundle (local build). - To reproduce/verify: open the Text Finder (`text_finder::Toggle`), type a query (or rely on a seeded one), then press Escape with a binding that resolves to `editor::Cancel`, or press `cmd-shift-f` (`DeploySearch`) while the finder is open. Before this change the app crashes. After it, the modal dismisses and the query is persisted. - Tested on macOS 26.5.1 (arm64); the affected code is platform-independent. ## Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks — no unsafe blocks - [x] The content adheres to Zed's UI standards — no UI changes - [x] Tests cover the new/changed behavior — added - [x] Performance impact has been considered and is acceptable — one `Option` copy at modal creation --- Release Notes: - Fixed a crash when closing the Text Finder while a workspace action (e.g. Escape via `editor::Cancel`, or deploying search) triggered the dismissal --- crates/search/src/text_finder.rs | 86 +++++++++++++++++++++++++++++--- 1 file changed, 79 insertions(+), 7 deletions(-) diff --git a/crates/search/src/text_finder.rs b/crates/search/src/text_finder.rs index 500f8e4fe9e..06679887155 100644 --- a/crates/search/src/text_finder.rs +++ b/crates/search/src/text_finder.rs @@ -34,6 +34,7 @@ actions!(text_finder, [ToProjectSearch, Fold, Unfold, ToggleFoldAll]); pub struct TextFinder { picker: Entity>, init_modifiers: Option, + workspace_id: Option, _subscription: Subscription, } @@ -158,8 +159,9 @@ impl TextFinder { workspace .update_in(cx, |workspace, window, cx| { remove_project_search_tab(project_search_item_id, workspace, window, cx); + let workspace_id = workspace.database_id(); workspace.toggle_modal(window, cx, |window, cx| { - Self::new(delegate, None, window, cx) + Self::new(delegate, None, workspace_id, window, cx) }); }) .ok(); @@ -380,8 +382,9 @@ impl TextFinder { let delegate = delegate_task.await; workspace .update_in(cx, |workspace, window, cx| { + let workspace_id = workspace.database_id(); workspace.toggle_modal(window, cx, |window, cx| { - Self::new(delegate, seed_query, window, cx) + Self::new(delegate, seed_query, workspace_id, window, cx) }); }) .ok(); @@ -391,6 +394,7 @@ impl TextFinder { fn new( delegate: Delegate, seed_query: Option, + workspace_id: Option, window: &mut Window, cx: &mut Context, ) -> Self { @@ -418,6 +422,7 @@ impl TextFinder { Self { picker, init_modifiers: window.modifiers().modified().then_some(window.modifiers()), + workspace_id, _subscription: subscription, } } @@ -450,11 +455,7 @@ impl ModalView for TextFinder { let query = picker.query(cx); if !query.is_empty() { let options = picker.delegate.search_options; - let workspace_id = self - .weak_workspace(cx) - .upgrade() - .and_then(|workspace| workspace.read(cx).database_id()); - store_last_search(workspace_id, query, options, cx); + store_last_search(self.workspace_id, query, options, cx); } DismissDecision::Dismiss(true) } @@ -478,3 +479,74 @@ pub struct SearchMatch { pub line_text: String, pub line_number: u32, } + +#[cfg(test)] +mod tests { + use gpui::{TestAppContext, VisualTestContext}; + use project::{FakeFs, Project}; + use serde_json::json; + use settings::SettingsStore; + use util::path; + use workspace::MultiWorkspace; + + use super::*; + + fn init_test(cx: &mut TestAppContext) { + cx.update(|cx| { + let settings = SettingsStore::test(cx); + cx.set_global(settings); + + theme_settings::init(theme::LoadThemes::JustBase, cx); + + editor::init(cx); + crate::init(cx); + }); + } + + /// Dismissal can be initiated from inside a workspace update: workspace-level + /// action handlers (e.g. buffer search's `SearchActionsRegistrar`) call + /// `Workspace::hide_modal` while the workspace entity is leased, which runs + /// `on_before_dismiss` synchronously under that lease. Reading the workspace + /// entity there panics with "cannot read workspace::Workspace while it is + /// already being updated", so this test dismisses the finder the same way. + #[gpui::test] + async fn test_dismiss_from_within_workspace_update(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.background_executor.clone()); + fs.insert_tree(path!("/dir"), json!({"one.rs": "const ONE: usize = 1;"})) + .await; + let project = Project::test(fs.clone(), [path!("/dir").as_ref()], cx).await; + let window = cx.add_window(|window, cx| MultiWorkspace::test_new(project, window, cx)); + let workspace = window + .read_with(cx, |multi_workspace, _| multi_workspace.workspace().clone()) + .unwrap(); + let cx = &mut VisualTestContext::from_window(window.into(), cx); + + // Seed a query: the last-search persistence in `on_before_dismiss` (the + // code path that read the workspace entity) only runs when the query is + // non-empty, which is the common case in practice since the finder seeds + // the previous query on open. + let seed_query = SearchSeed { + query: "ONE".to_string(), + options: None, + }; + workspace + .update_in(cx, |_, window, cx| { + TextFinder::open(Some(seed_query), window, cx) + }) + .await; + + workspace.update(cx, |workspace, cx| { + assert!(workspace.active_modal::(cx).is_some()); + }); + + workspace.update_in(cx, |workspace, window, cx| { + assert!(workspace.hide_modal(window, cx)); + }); + + workspace.update(cx, |workspace, cx| { + assert!(workspace.active_modal::(cx).is_none()); + }); + } +}