Fix Project strong reference leaks (#22470)

Kirill Bulatov created

Closes https://github.com/zed-industries/zed/issues/21906

* After https://github.com/zed-industries/zed/pull/21238,
`TerminalPanel` and `Project` strong references were moved into
`Pane`-related closures, creating a cycle, that did not allow
registering project release and shutting down corresponding language
servers

* After https://github.com/zed-industries/zed/pull/22329, a special
`Editor` was created with a strong reference to the `Project` which
seemed to do nothing bad in general, but when a working Zed was running
a Zed Dev build, had the same issue with preventing language servers
from shutting down.

The latter is very odd, and seems quite dangerous, as any arbitrary
`Editor` with `Project` in it may do the same, yet it seems that we did
not store them before the way git panel does.

I have tried creating a test, yet seems that we need to initialize a lot
of Zed for it which I failed β€”Β all my attempts resulted in a single
language server being present in the `Project`'s statuses.

Release Notes:

- Fixed language servers not being released between project reopens

Change summary

crates/git_ui/src/git_panel.rs             | 83 ++++++++++++-----------
crates/terminal_view/src/terminal_panel.rs | 34 ++++++---
2 files changed, 66 insertions(+), 51 deletions(-)

Detailed changes

crates/git_ui/src/git_panel.rs πŸ”—

@@ -107,7 +107,7 @@ pub struct GitPanel {
     // not hidden by folding or such
     visible_entries: Vec<WorktreeEntries>,
     width: Option<Pixels>,
-    git_diff_editor: View<Editor>,
+    git_diff_editor: Option<View<Editor>>,
     git_diff_editor_updates: Task<()>,
     reveal_in_editor: Task<()>,
 }
@@ -149,11 +149,7 @@ impl GitPanel {
         workspace: WeakView<Workspace>,
         cx: AsyncWindowContext,
     ) -> Task<Result<View<Self>>> {
-        cx.spawn(|mut cx| async move {
-            // Clippy incorrectly classifies this as a redundant closure
-            #[allow(clippy::redundant_closure)]
-            workspace.update(&mut cx, |workspace, cx| Self::new(workspace, cx))
-        })
+        cx.spawn(|mut cx| async move { workspace.update(&mut cx, Self::new) })
     }
 
     pub fn new(workspace: &mut Workspace, cx: &mut ViewContext<Workspace>) -> View<Self> {
@@ -168,7 +164,7 @@ impl GitPanel {
                 this.hide_scrollbar(cx);
             })
             .detach();
-            cx.subscribe(&project, |this, project, event, cx| match event {
+            cx.subscribe(&project, |this, _, event, cx| match event {
                 project::Event::WorktreeRemoved(id) => {
                     this.expanded_dir_ids.remove(id);
                     this.update_visible_entries(None, None, cx);
@@ -186,9 +182,10 @@ impl GitPanel {
                 }
                 project::Event::Closed => {
                     this.git_diff_editor_updates = Task::ready(());
+                    this.reveal_in_editor = Task::ready(());
                     this.expanded_dir_ids.clear();
                     this.visible_entries.clear();
-                    this.git_diff_editor = diff_display_editor(project.clone(), cx);
+                    this.git_diff_editor = None;
                 }
                 _ => {}
             })
@@ -196,7 +193,7 @@ impl GitPanel {
 
             let scroll_handle = UniformListScrollHandle::new();
 
-            let mut this = Self {
+            let mut git_panel = Self {
                 workspace: weak_workspace,
                 focus_handle: cx.focus_handle(),
                 fs,
@@ -211,13 +208,13 @@ impl GitPanel {
                 selected_item: None,
                 show_scrollbar: !Self::should_autohide_scrollbar(cx),
                 hide_scrollbar_task: None,
-                git_diff_editor: diff_display_editor(project.clone(), cx),
+                git_diff_editor: Some(diff_display_editor(cx)),
                 git_diff_editor_updates: Task::ready(()),
                 reveal_in_editor: Task::ready(()),
                 project,
             };
-            this.update_visible_entries(None, None, cx);
-            this
+            git_panel.update_visible_entries(None, None, cx);
+            git_panel
         });
 
         git_panel
@@ -602,7 +599,7 @@ impl GitPanel {
             );
         }
 
-        let project = self.project.clone();
+        let project = self.project.downgrade();
         self.git_diff_editor_updates = cx.spawn(|git_panel, mut cx| async move {
             cx.background_executor()
                 .timer(UPDATE_DEBOUNCE)
@@ -610,7 +607,7 @@ impl GitPanel {
             let Some(project_buffers) = git_panel
                 .update(&mut cx, |git_panel, cx| {
                     futures::future::join_all(git_panel.visible_entries.iter_mut().flat_map(
-                        move |worktree_entries| {
+                        |worktree_entries| {
                             worktree_entries
                                 .visible_entries
                                 .iter()
@@ -694,7 +691,7 @@ impl GitPanel {
                                                     anyhow::Ok((buffer, unstaged_changes, hunks))
                                                 });
                                             Some((entry_path, unstaged_changes_task))
-                                        })?;
+                                        }).ok()??;
                                     Some((entry_path, unstaged_changes_task))
                                 })
                                 .map(|(entry_path, open_task)| async move {
@@ -716,7 +713,7 @@ impl GitPanel {
             let mut change_sets = Vec::with_capacity(project_buffers.len());
             if let Some(buffer_update_task) = git_panel
                 .update(&mut cx, |git_panel, cx| {
-                    let editor = git_panel.git_diff_editor.clone();
+                    let editor = git_panel.git_diff_editor.clone()?;
                     let multi_buffer = editor.read(cx).buffer().clone();
                     let mut buffers_with_ranges = Vec::with_capacity(project_buffers.len());
                     for (buffer_path, open_result) in project_buffers {
@@ -735,25 +732,27 @@ impl GitPanel {
                         }
                     }
 
-                    multi_buffer.update(cx, |multi_buffer, cx| {
+                    Some(multi_buffer.update(cx, |multi_buffer, cx| {
                         multi_buffer.clear(cx);
                         multi_buffer.push_multiple_excerpts_with_context_lines(
                             buffers_with_ranges,
                             DEFAULT_MULTIBUFFER_CONTEXT,
                             cx,
                         )
-                    })
+                    }))
                 })
-                .ok()
+                .ok().flatten()
             {
                 buffer_update_task.await;
                 git_panel
                     .update(&mut cx, |git_panel, cx| {
-                        git_panel.git_diff_editor.update(cx, |editor, cx| {
-                            for change_set in change_sets {
-                                editor.add_change_set(change_set, cx);
-                            }
-                        })
+                        if let Some(diff_editor) = git_panel.git_diff_editor.as_ref() {
+                            diff_editor.update(cx, |editor, cx| {
+                                for change_set in change_sets {
+                                    editor.add_change_set(change_set, cx);
+                                }
+                            });
+                        }
                     })
                     .ok();
             }
@@ -1001,7 +1000,7 @@ impl GitPanel {
         let id = id.to_proto() as usize;
         let checkbox_id = ElementId::Name(format!("checkbox_{}", id).into());
         let is_staged = ToggleState::Selected;
-        let handle = cx.view().clone();
+        let handle = cx.view().downgrade();
 
         h_flex()
             .id(id)
@@ -1024,16 +1023,18 @@ impl GitPanel {
                     .toggle_state(selected)
                     .child(h_flex().gap_1p5().child(details.display_name.clone()))
                     .on_click(move |e, cx| {
-                        handle.update(cx, |git_panel, cx| {
-                            git_panel.selected_item = Some(details.index);
-                            let change_focus = e.down.click_count > 1;
-                            git_panel.reveal_entry_in_git_editor(
-                                details.hunks.clone(),
-                                change_focus,
-                                None,
-                                cx,
-                            );
-                        });
+                        handle
+                            .update(cx, |git_panel, cx| {
+                                git_panel.selected_item = Some(details.index);
+                                let change_focus = e.down.click_count > 1;
+                                git_panel.reveal_entry_in_git_editor(
+                                    details.hunks.clone(),
+                                    change_focus,
+                                    None,
+                                    cx,
+                                );
+                            })
+                            .ok();
                     }),
             )
     }
@@ -1046,7 +1047,9 @@ impl GitPanel {
         cx: &mut ViewContext<'_, Self>,
     ) {
         let workspace = self.workspace.clone();
-        let diff_editor = self.git_diff_editor.clone();
+        let Some(diff_editor) = self.git_diff_editor.clone() else {
+            return;
+        };
         self.reveal_in_editor = cx.spawn(|_, mut cx| async move {
             if let Some(debounce) = debounce {
                 cx.background_executor().timer(debounce).await;
@@ -1236,12 +1239,12 @@ impl Panel for GitPanel {
     }
 }
 
-fn diff_display_editor(project: Model<Project>, cx: &mut WindowContext) -> View<Editor> {
+fn diff_display_editor(cx: &mut WindowContext) -> View<Editor> {
     cx.new_view(|cx| {
-        let multi_buffer = cx.new_model(|cx| {
-            MultiBuffer::new(project.read(cx).capability()).with_title("Project diff".to_string())
+        let multi_buffer = cx.new_model(|_| {
+            MultiBuffer::new(language::Capability::ReadWrite).with_title("Project diff".to_string())
         });
-        let mut editor = Editor::for_multibuffer(multi_buffer, Some(project), true, cx);
+        let mut editor = Editor::for_multibuffer(multi_buffer, None, true, cx);
         editor.set_expand_all_diff_hunks();
         editor
     })

crates/terminal_view/src/terminal_panel.rs πŸ”—

@@ -964,19 +964,23 @@ pub fn new_terminal_pane(
         pane.set_should_display_tab_bar(|_| true);
         pane.set_zoom_out_on_close(false);
 
-        let terminal_panel_for_split_check = terminal_panel.clone();
+        let split_closure_terminal_panel = terminal_panel.downgrade();
         pane.set_can_split(Some(Arc::new(move |pane, dragged_item, cx| {
             if let Some(tab) = dragged_item.downcast_ref::<DraggedTab>() {
-                let current_pane = cx.view().clone();
-                let can_drag_away =
-                    terminal_panel_for_split_check.update(cx, |terminal_panel, _| {
+                let is_current_pane = &tab.pane == cx.view();
+                let Some(can_drag_away) = split_closure_terminal_panel
+                    .update(cx, |terminal_panel, _| {
                         let current_panes = terminal_panel.center.panes();
                         !current_panes.contains(&&tab.pane)
                             || current_panes.len() > 1
-                            || (tab.pane != current_pane || pane.items_len() > 1)
-                    });
+                            || (!is_current_pane || pane.items_len() > 1)
+                    })
+                    .ok()
+                else {
+                    return false;
+                };
                 if can_drag_away {
-                    let item = if tab.pane == current_pane {
+                    let item = if is_current_pane {
                         pane.item_for_index(tab.ix)
                     } else {
                         tab.pane.read(cx).item_for_index(tab.ix)
@@ -996,7 +1000,12 @@ pub fn new_terminal_pane(
             toolbar.add_item(breadcrumbs, cx);
         });
 
+        let drop_closure_project = project.downgrade();
+        let drop_closure_terminal_panel = terminal_panel.downgrade();
         pane.set_custom_drop_handle(cx, move |pane, dropped_item, cx| {
+            let Some(project) = drop_closure_project.upgrade() else {
+                return ControlFlow::Break(());
+            };
             if let Some(tab) = dropped_item.downcast_ref::<DraggedTab>() {
                 let this_pane = cx.view().clone();
                 let item = if tab.pane == this_pane {
@@ -1009,10 +1018,10 @@ pub fn new_terminal_pane(
                         let source = tab.pane.clone();
                         let item_id_to_move = item.item_id();
 
-                        let new_split_pane = pane
+                        let Ok(new_split_pane) = pane
                             .drag_split_direction()
                             .map(|split_direction| {
-                                terminal_panel.update(cx, |terminal_panel, cx| {
+                                drop_closure_terminal_panel.update(cx, |terminal_panel, cx| {
                                     let is_zoomed = if terminal_panel.active_pane == this_pane {
                                         pane.is_zoomed()
                                     } else {
@@ -1033,9 +1042,12 @@ pub fn new_terminal_pane(
                                     anyhow::Ok(new_pane)
                                 })
                             })
-                            .transpose();
+                            .transpose()
+                        else {
+                            return ControlFlow::Break(());
+                        };
 
-                        match new_split_pane {
+                        match new_split_pane.transpose() {
                             // Source pane may be the one currently updated, so defer the move.
                             Ok(Some(new_pane)) => cx
                                 .spawn(|_, mut cx| async move {