From 6317e885c7667d9d488c32d8d81737708dc0556a Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 26 Apr 2023 13:36:13 +0200 Subject: [PATCH] Don't allow strong view handles to be read/updated with an AsyncAppContext This avoids an invitation to hold strong view handles across async await points, which is a common source of leaks. Co-Authored-By: Nathan Sobo --- crates/collab_ui/src/collab_ui.rs | 4 +- crates/editor/src/editor.rs | 12 ++- crates/editor/src/items.rs | 8 +- crates/feedback/src/feedback_editor.rs | 3 +- crates/gpui/src/app.rs | 44 +------- crates/gpui/src/app/test_app_context.rs | 2 - crates/gpui/src/app/window.rs | 2 - crates/journal/src/journal.rs | 2 +- crates/workspace/src/dock.rs | 2 - crates/workspace/src/pane.rs | 124 ++++++++++------------ crates/workspace/src/persistence/model.rs | 36 ++++--- crates/workspace/src/workspace.rs | 19 ++-- crates/zed/src/main.rs | 16 +-- crates/zed/src/zed.rs | 18 +++- 14 files changed, 129 insertions(+), 163 deletions(-) diff --git a/crates/collab_ui/src/collab_ui.rs b/crates/collab_ui/src/collab_ui.rs index 2902172684..557f9b58d9 100644 --- a/crates/collab_ui/src/collab_ui.rs +++ b/crates/collab_ui/src/collab_ui.rs @@ -61,7 +61,7 @@ fn join_project(action: &JoinProject, app_state: Arc, cx: &mut AppCont }); let workspace = if let Some(existing_workspace) = existing_workspace { - existing_workspace + existing_workspace.downgrade() } else { let active_call = cx.read(ActiveCall::global); let room = active_call @@ -93,7 +93,7 @@ fn join_project(action: &JoinProject, app_state: Arc, cx: &mut AppCont workspace }, ); - workspace + workspace.downgrade() }; cx.activate_window(workspace.window_id()); diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 59d96fc83e..24a0b15a8f 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -2715,14 +2715,15 @@ impl Editor { let apply_code_actions = workspace.project().clone().update(cx, |project, cx| { project.apply_code_action(buffer, action, true, cx) }); + let editor = editor.downgrade(); Some(cx.spawn(|workspace, cx| async move { let project_transaction = apply_code_actions.await?; - Self::open_project_transaction(editor, workspace, project_transaction, title, cx).await + Self::open_project_transaction(&editor, workspace, project_transaction, title, cx).await })) } async fn open_project_transaction( - this: ViewHandle, + this: &WeakViewHandle, workspace: WeakViewHandle, transaction: ProjectTransaction, title: String, @@ -5943,10 +5944,11 @@ impl Editor { project.perform_rename(buffer.clone(), range.start, new_name.clone(), true, cx) }); + let editor = editor.downgrade(); Some(cx.spawn(|workspace, mut cx| async move { let project_transaction = rename.await?; Self::open_project_transaction( - editor.clone(), + &editor, workspace, project_transaction, format!("Rename: {} → {}", old_name, new_name), @@ -6761,7 +6763,8 @@ impl Editor { let editor = editor .await? .downcast::() - .ok_or_else(|| anyhow!("opened item was not an editor"))?; + .ok_or_else(|| anyhow!("opened item was not an editor"))? + .downgrade(); editor.update(&mut cx, |editor, cx| { let buffer = editor .buffer() @@ -6783,6 +6786,7 @@ impl Editor { anyhow::Ok(()) })??; + anyhow::Ok(()) }) .detach_and_log_err(cx); diff --git a/crates/editor/src/items.rs b/crates/editor/src/items.rs index 058829d1b9..2eaf7b568e 100644 --- a/crates/editor/src/items.rs +++ b/crates/editor/src/items.rs @@ -67,6 +67,7 @@ impl FollowableItem for Editor { .collect::>() }); + let pane = pane.downgrade(); Some(cx.spawn(|mut cx| async move { let mut buffers = futures::future::try_join_all(buffers).await?; let editor = pane.read_with(&cx, |pane, cx| { @@ -127,7 +128,7 @@ impl FollowableItem for Editor { }; update_editor_from_message( - editor.clone(), + editor.downgrade(), project, proto::update_view::Editor { selections: state.selections, @@ -286,9 +287,6 @@ impl FollowableItem for Editor { let update_view::Variant::Editor(message) = message; let project = project.clone(); cx.spawn(|this, mut cx| async move { - let this = this - .upgrade(&cx) - .ok_or_else(|| anyhow!("editor was dropped"))?; update_editor_from_message(this, project, message, &mut cx).await }) } @@ -304,7 +302,7 @@ impl FollowableItem for Editor { } async fn update_editor_from_message( - this: ViewHandle, + this: WeakViewHandle, project: ModelHandle, message: proto::update_view::Editor, cx: &mut AsyncAppContext, diff --git a/crates/feedback/src/feedback_editor.rs b/crates/feedback/src/feedback_editor.rs index e5a406bc73..7bf5328048 100644 --- a/crates/feedback/src/feedback_editor.rs +++ b/crates/feedback/src/feedback_editor.rs @@ -124,11 +124,10 @@ impl FeedbackEditor { &["Yes, Submit!", "No"], ); - let this = cx.handle(); let client = cx.global::>().clone(); let specs = self.system_specs.clone(); - cx.spawn(|_, mut cx| async move { + cx.spawn(|this, mut cx| async move { let answer = answer.recv().await; if answer == Some(0) { diff --git a/crates/gpui/src/app.rs b/crates/gpui/src/app.rs index 3739620e3b..c201febed5 100644 --- a/crates/gpui/src/app.rs +++ b/crates/gpui/src/app.rs @@ -127,18 +127,8 @@ pub trait BorrowAppContext { } pub trait BorrowWindowContext { - type ReturnValue; - - fn read_with T>( - &self, - window_id: usize, - f: F, - ) -> Self::ReturnValue; - fn update T>( - &mut self, - window_id: usize, - f: F, - ) -> Self::ReturnValue; + fn read_with T>(&self, window_id: usize, f: F) -> T; + fn update T>(&mut self, window_id: usize, f: F) -> T; } #[derive(Clone)] @@ -381,28 +371,6 @@ impl BorrowAppContext for AsyncAppContext { } } -impl BorrowWindowContext for AsyncAppContext { - type ReturnValue = Result; - - fn read_with T>(&self, window_id: usize, f: F) -> Result { - self.0 - .borrow() - .read_window(window_id, f) - .ok_or_else(|| anyhow!("window was closed")) - } - - fn update T>( - &mut self, - window_id: usize, - f: F, - ) -> Result { - self.0 - .borrow_mut() - .update_window(window_id, f) - .ok_or_else(|| anyhow!("window was closed")) - } -} - type ActionCallback = dyn FnMut(&mut dyn AnyView, &dyn Action, &mut WindowContext, usize); type GlobalActionCallback = dyn FnMut(&dyn Action, &mut AppContext); @@ -3330,8 +3298,6 @@ impl BorrowAppContext for ViewContext<'_, '_, V> { } impl BorrowWindowContext for ViewContext<'_, '_, V> { - type ReturnValue = T; - fn read_with T>(&self, window_id: usize, f: F) -> T { BorrowWindowContext::read_with(&*self.window_context, window_id, f) } @@ -3384,8 +3350,6 @@ impl BorrowAppContext for EventContext<'_, '_, '_, V> { } impl BorrowWindowContext for EventContext<'_, '_, '_, V> { - type ReturnValue = T; - fn read_with T>(&self, window_id: usize, f: F) -> T { BorrowWindowContext::read_with(&*self.view_context, window_id, f) } @@ -3722,7 +3686,7 @@ impl ViewHandle { cx.read_view(self) } - pub fn read_with(&self, cx: &C, read: F) -> C::ReturnValue + pub fn read_with(&self, cx: &C, read: F) -> S where C: BorrowWindowContext, F: FnOnce(&T, &ViewContext) -> S, @@ -3733,7 +3697,7 @@ impl ViewHandle { }) } - pub fn update(&self, cx: &mut C, update: F) -> C::ReturnValue + pub fn update(&self, cx: &mut C, update: F) -> S where C: BorrowWindowContext, F: FnOnce(&mut T, &mut ViewContext) -> S, diff --git a/crates/gpui/src/app/test_app_context.rs b/crates/gpui/src/app/test_app_context.rs index 9aa6eb3393..3a03a81c47 100644 --- a/crates/gpui/src/app/test_app_context.rs +++ b/crates/gpui/src/app/test_app_context.rs @@ -390,8 +390,6 @@ impl BorrowAppContext for TestAppContext { } impl BorrowWindowContext for TestAppContext { - type ReturnValue = T; - fn read_with T>(&self, window_id: usize, f: F) -> T { self.cx .borrow() diff --git a/crates/gpui/src/app/window.rs b/crates/gpui/src/app/window.rs index 908f58e33e..f54c18c755 100644 --- a/crates/gpui/src/app/window.rs +++ b/crates/gpui/src/app/window.rs @@ -142,8 +142,6 @@ impl BorrowAppContext for WindowContext<'_> { } impl BorrowWindowContext for WindowContext<'_> { - type ReturnValue = T; - fn read_with T>(&self, window_id: usize, f: F) -> T { if self.window_id == window_id { f(self) diff --git a/crates/journal/src/journal.rs b/crates/journal/src/journal.rs index c439edd87d..4b9622ece9 100644 --- a/crates/journal/src/journal.rs +++ b/crates/journal/src/journal.rs @@ -56,7 +56,7 @@ pub fn new_journal_entry(app_state: Arc, cx: &mut AppContext) { .await; if let Some(Some(Ok(item))) = opened.first() { - if let Some(editor) = item.downcast::() { + if let Some(editor) = item.downcast::().map(|editor| editor.downgrade()) { editor.update(&mut cx, |editor, cx| { let len = editor.buffer().read(cx).len(cx); editor.change_selections(Some(Autoscroll::center()), cx, |s| { diff --git a/crates/workspace/src/dock.rs b/crates/workspace/src/dock.rs index c80f71c422..67cfea3d9a 100644 --- a/crates/workspace/src/dock.rs +++ b/crates/workspace/src/dock.rs @@ -811,8 +811,6 @@ mod tests { } impl BorrowWindowContext for DockTestContext<'_> { - type ReturnValue = T; - fn read_with T>(&self, window_id: usize, f: F) -> T { BorrowWindowContext::read_with(self.cx, window_id, f) } diff --git a/crates/workspace/src/pane.rs b/crates/workspace/src/pane.rs index b1214f4233..aa913986de 100644 --- a/crates/workspace/src/pane.rs +++ b/crates/workspace/src/pane.rs @@ -214,26 +214,10 @@ pub fn init(cx: &mut AppContext) { Pane::reopen_closed_item(workspace, cx).detach(); }); cx.add_action(|workspace: &mut Workspace, action: &GoBack, cx| { - Pane::go_back( - workspace, - action - .pane - .as_ref() - .and_then(|weak_handle| weak_handle.upgrade(cx)), - cx, - ) - .detach(); + Pane::go_back(workspace, action.pane.clone(), cx).detach(); }); cx.add_action(|workspace: &mut Workspace, action: &GoForward, cx| { - Pane::go_forward( - workspace, - action - .pane - .as_ref() - .and_then(|weak_handle| weak_handle.upgrade(cx)), - cx, - ) - .detach(); + Pane::go_forward(workspace, action.pane.clone(), cx).detach(); }); } @@ -392,12 +376,12 @@ impl Pane { pub fn go_back( workspace: &mut Workspace, - pane: Option>, + pane: Option>, cx: &mut ViewContext, ) -> Task> { Self::navigate_history( workspace, - pane.unwrap_or_else(|| workspace.active_pane().clone()), + pane.unwrap_or_else(|| workspace.active_pane().downgrade()), NavigationMode::GoingBack, cx, ) @@ -405,12 +389,12 @@ impl Pane { pub fn go_forward( workspace: &mut Workspace, - pane: Option>, + pane: Option>, cx: &mut ViewContext, ) -> Task> { Self::navigate_history( workspace, - pane.unwrap_or_else(|| workspace.active_pane().clone()), + pane.unwrap_or_else(|| workspace.active_pane().downgrade()), NavigationMode::GoingForward, cx, ) @@ -422,7 +406,7 @@ impl Pane { ) -> Task> { Self::navigate_history( workspace, - workspace.active_pane().clone(), + workspace.active_pane().downgrade(), NavigationMode::ReopeningClosedItem, cx, ) @@ -450,62 +434,62 @@ impl Pane { fn navigate_history( workspace: &mut Workspace, - pane: ViewHandle, + pane: WeakViewHandle, mode: NavigationMode, cx: &mut ViewContext, ) -> Task> { - cx.focus(&pane); + let to_load = if let Some(pane) = pane.upgrade(cx) { + cx.focus(&pane); - let to_load = pane.update(cx, |pane, cx| { - loop { - // Retrieve the weak item handle from the history. - let entry = pane.nav_history.borrow_mut().pop(mode, cx)?; + pane.update(cx, |pane, cx| { + loop { + // Retrieve the weak item handle from the history. + let entry = pane.nav_history.borrow_mut().pop(mode, cx)?; - // If the item is still present in this pane, then activate it. - if let Some(index) = entry - .item - .upgrade(cx) - .and_then(|v| pane.index_for_item(v.as_ref())) - { - let prev_active_item_index = pane.active_item_index; - pane.nav_history.borrow_mut().set_mode(mode); - pane.activate_item(index, true, true, cx); - pane.nav_history - .borrow_mut() - .set_mode(NavigationMode::Normal); + // If the item is still present in this pane, then activate it. + if let Some(index) = entry + .item + .upgrade(cx) + .and_then(|v| pane.index_for_item(v.as_ref())) + { + let prev_active_item_index = pane.active_item_index; + pane.nav_history.borrow_mut().set_mode(mode); + pane.activate_item(index, true, true, cx); + pane.nav_history + .borrow_mut() + .set_mode(NavigationMode::Normal); - let mut navigated = prev_active_item_index != pane.active_item_index; - if let Some(data) = entry.data { - navigated |= pane.active_item()?.navigate(data, cx); + let mut navigated = prev_active_item_index != pane.active_item_index; + if let Some(data) = entry.data { + navigated |= pane.active_item()?.navigate(data, cx); + } + + if navigated { + break None; + } } - - if navigated { - break None; + // If the item is no longer present in this pane, then retrieve its + // project path in order to reopen it. + else { + break pane + .nav_history + .borrow() + .paths_by_item + .get(&entry.item.id()) + .cloned() + .map(|project_path| (project_path, entry)); } } - // If the item is no longer present in this pane, then retrieve its - // project path in order to reopen it. - else { - break pane - .nav_history - .borrow() - .paths_by_item - .get(&entry.item.id()) - .cloned() - .map(|project_path| (project_path, entry)); - } - } - }); + }) + } else { + None + }; if let Some((project_path, entry)) = to_load { // If the item was no longer present, then load it again from its previous path. - let pane = pane.downgrade(); let task = workspace.load_path(project_path, cx); cx.spawn(|workspace, mut cx| async move { let task = task.await; - let pane = pane - .upgrade(&cx) - .ok_or_else(|| anyhow!("pane was dropped"))?; let mut navigated = false; if let Some((project_entry_id, build_item)) = task.log_err() { let prev_active_item_id = pane.update(&mut cx, |pane, _| { @@ -514,15 +498,18 @@ impl Pane { })?; let item = workspace.update(&mut cx, |workspace, cx| { - Self::open_item( + let pane = pane + .upgrade(cx) + .ok_or_else(|| anyhow!("pane was dropped"))?; + anyhow::Ok(Self::open_item( workspace, pane.clone(), project_entry_id, true, cx, build_item, - ) - })?; + )) + })??; pane.update(&mut cx, |pane, cx| { navigated |= Some(item.id()) != prev_active_item_id; @@ -973,6 +960,7 @@ impl Pane { // of what content they would be saving. items_to_close.sort_by_key(|item| !item.is_singleton(cx)); + let pane = pane.downgrade(); cx.spawn(|workspace, mut cx| async move { let mut saved_project_items_ids = HashSet::default(); for item in items_to_close.clone() { @@ -1084,7 +1072,7 @@ impl Pane { pub async fn save_item( project: ModelHandle, - pane: &ViewHandle, + pane: &WeakViewHandle, item_ix: usize, item: &dyn ItemHandle, should_prompt_for_save: bool, diff --git a/crates/workspace/src/persistence/model.rs b/crates/workspace/src/persistence/model.rs index 03d7f2235b..08da41d7e8 100644 --- a/crates/workspace/src/persistence/model.rs +++ b/crates/workspace/src/persistence/model.rs @@ -1,28 +1,24 @@ -use std::{ - path::{Path, PathBuf}, - sync::Arc, +use crate::{ + dock::DockPosition, ItemDeserializers, Member, Pane, PaneAxis, Workspace, WorkspaceId, }; - -use anyhow::{Context, Result}; - +use anyhow::{anyhow, Context, Result}; use async_recursion::async_recursion; -use gpui::{ - platform::WindowBounds, AsyncAppContext, Axis, ModelHandle, Task, ViewHandle, WeakViewHandle, -}; - use db::sqlez::{ bindable::{Bind, Column, StaticColumnCount}, statement::Statement, }; +use gpui::{ + platform::WindowBounds, AsyncAppContext, Axis, ModelHandle, Task, ViewHandle, WeakViewHandle, +}; use project::Project; use settings::DockAnchor; +use std::{ + path::{Path, PathBuf}, + sync::Arc, +}; use util::ResultExt; use uuid::Uuid; -use crate::{ - dock::DockPosition, ItemDeserializers, Member, Pane, PaneAxis, Workspace, WorkspaceId, -}; - #[derive(Debug, Clone, PartialEq, Eq)] pub struct WorkspaceLocation(Arc>); @@ -134,7 +130,7 @@ impl SerializedPaneGroup { } SerializedPaneGroup::Pane(serialized_pane) => { let pane = workspace - .update(cx, |workspace, cx| workspace.add_pane(cx)) + .update(cx, |workspace, cx| workspace.add_pane(cx).downgrade()) .log_err()?; let active = serialized_pane.active; serialized_pane @@ -146,8 +142,10 @@ impl SerializedPaneGroup { .read_with(cx, |pane, _| pane.items_len() != 0) .log_err()? { + let pane = pane.upgrade(cx)?; Some((Member::Pane(pane.clone()), active.then(|| pane))) } else { + let pane = pane.upgrade(cx)?; workspace .update(cx, |workspace, cx| workspace.remove_pane(pane, cx)) .log_err()?; @@ -172,7 +170,7 @@ impl SerializedPane { pub async fn deserialize_to( &self, project: &ModelHandle, - pane_handle: &ViewHandle, + pane_handle: &WeakViewHandle, workspace_id: WorkspaceId, workspace: &WeakViewHandle, cx: &mut AsyncAppContext, @@ -196,8 +194,12 @@ impl SerializedPane { if let Some(item_handle) = item_handle { workspace.update(cx, |workspace, cx| { + let pane_handle = pane_handle + .upgrade(cx) + .ok_or_else(|| anyhow!("pane was dropped"))?; Pane::add_item(workspace, &pane_handle, item_handle, false, false, None, cx); - })?; + anyhow::Ok(()) + })??; } if item.active { diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index 0796c7c283..7524b84bf8 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -864,7 +864,7 @@ impl Workspace { requesting_window_id: Option, cx: &mut AppContext, ) -> Task<( - ViewHandle, + WeakViewHandle, Vec, anyhow::Error>>>, )> { let project_handle = Project::local( @@ -982,6 +982,7 @@ impl Workspace { .1 }); + let workspace = workspace.downgrade(); notify_if_database_failed(&workspace, &mut cx); // Call open path for each of the project paths @@ -1206,7 +1207,7 @@ impl Workspace { .flat_map(|pane| { pane.read(cx).items().filter_map(|item| { if item.is_dirty(cx) { - Some((pane.clone(), item.boxed_clone())) + Some((pane.downgrade(), item.boxed_clone())) } else { None } @@ -2688,7 +2689,7 @@ impl Workspace { workspace.read_with(&cx, |workspace, _| { ( workspace.project().clone(), - workspace.dock_pane().clone(), + workspace.dock_pane().downgrade(), workspace.last_active_center_pane.clone(), ) })?; @@ -2769,7 +2770,7 @@ impl Workspace { } } -fn notify_if_database_failed(workspace: &ViewHandle, cx: &mut AsyncAppContext) { +fn notify_if_database_failed(workspace: &WeakViewHandle, cx: &mut AsyncAppContext) { workspace.update(cx, |workspace, cx| { if (*db::ALL_FILE_DB_FAILED).load(std::sync::atomic::Ordering::Acquire) { workspace.show_notification_once(0, cx, |cx| { @@ -2980,7 +2981,7 @@ pub struct WorkspaceCreated(WeakViewHandle); pub fn activate_workspace_for_project( cx: &mut AppContext, predicate: impl Fn(&mut Project, &mut ModelContext) -> bool, -) -> Option> { +) -> Option> { for window_id in cx.window_ids().collect::>() { let handle = cx .update_window(window_id, |cx| { @@ -2995,8 +2996,8 @@ pub fn activate_workspace_for_project( }) .flatten(); - if handle.is_some() { - return handle; + if let Some(handle) = handle { + return Some(handle.downgrade()); } } None @@ -3014,7 +3015,7 @@ pub fn open_paths( cx: &mut AppContext, ) -> Task< Result<( - ViewHandle, + WeakViewHandle, Vec, anyhow::Error>>>, )>, > { @@ -3700,7 +3701,7 @@ mod tests { workspace .update(cx, |workspace, cx| { - Pane::go_back(workspace, Some(pane.clone()), cx) + Pane::go_back(workspace, Some(pane.downgrade()), cx) }) .await .unwrap(); diff --git a/crates/zed/src/main.rs b/crates/zed/src/main.rs index 8162eb5a09..081938788f 100644 --- a/crates/zed/src/main.rs +++ b/crates/zed/src/main.rs @@ -674,13 +674,15 @@ async fn handle_cli_connection( let wait = async move { if paths.is_empty() { let (done_tx, done_rx) = oneshot::channel(); - let _subscription = cx.update(|cx| { - cx.observe_release(&workspace, move |_, _| { - let _ = done_tx.send(()); - }) - }); - drop(workspace); - let _ = done_rx.await; + if let Some(workspace) = workspace.upgrade(&cx) { + let _subscription = cx.update(|cx| { + cx.observe_release(&workspace, move |_, _| { + let _ = done_tx.send(()); + }) + }); + drop(workspace); + let _ = done_rx.await; + } } else { let _ = futures::future::try_join_all(item_release_futures).await; diff --git a/crates/zed/src/zed.rs b/crates/zed/src/zed.rs index 48bf68a433..4325835cdb 100644 --- a/crates/zed/src/zed.rs +++ b/crates/zed/src/zed.rs @@ -374,7 +374,14 @@ pub fn build_window_options( fn restart(_: &Restart, cx: &mut gpui::AppContext) { let mut workspaces = cx .window_ids() - .filter_map(|window_id| cx.root_view(window_id)?.clone().downcast::()) + .filter_map(|window_id| { + Some( + cx.root_view(window_id)? + .clone() + .downcast::()? + .downgrade(), + ) + }) .collect::>(); // If multiple windows have unsaved changes, and need a save prompt, @@ -419,7 +426,14 @@ fn restart(_: &Restart, cx: &mut gpui::AppContext) { fn quit(_: &Quit, cx: &mut gpui::AppContext) { let mut workspaces = cx .window_ids() - .filter_map(|window_id| cx.root_view(window_id)?.clone().downcast::()) + .filter_map(|window_id| { + Some( + cx.root_view(window_id)? + .clone() + .downcast::()? + .downgrade(), + ) + }) .collect::>(); // If multiple windows have unsaved changes, and need a save prompt,