Fix panic in remove_item (#21480)

Conrad Irwin created 1 year ago

In #20742 we added a call to remove_item that retain an item index over
an
await point. This led to a race condition that could panic if another
tab was
removed during that time. (cc @mgsloan)

This changes the API to make it harder to misuse.

Release Notes:

- Fixed a panic when closing tabs containing new unsaved files

Change summary

crates/workspace/src/pane.rs      | 30 ++++++++++++++----------------
crates/workspace/src/workspace.rs |  8 ++++----
2 files changed, 18 insertions(+), 20 deletions(-)

Detailed changes

crates/workspace/src/pane.rs

@@ -828,9 +828,10 @@ impl Pane {
 
     pub fn close_current_preview_item(&mut self, cx: &mut ViewContext<Self>) -> Option<usize> {
         let item_idx = self.preview_item_idx()?;
+        let id = self.preview_item_id()?;
 
         let prev_active_item_index = self.active_item_index;
-        self.remove_item(item_idx, false, false, cx);
+        self.remove_item(id, false, false, cx);
         self.active_item_index = prev_active_item_index;
 
         if item_idx < self.items.len() {
@@ -1403,13 +1404,7 @@ impl Pane {
 
                 // Remove the item from the pane.
                 pane.update(&mut cx, |pane, cx| {
-                    if let Some(item_ix) = pane
-                        .items
-                        .iter()
-                        .position(|i| i.item_id() == item_to_close.item_id())
-                    {
-                        pane.remove_item(item_ix, false, true, cx);
-                    }
+                    pane.remove_item(item_to_close.item_id(), false, true, cx);
                 })
                 .ok();
             }
@@ -1421,11 +1416,14 @@ impl Pane {
 
     pub fn remove_item(
         &mut self,
-        item_index: usize,
+        item_id: EntityId,
         activate_pane: bool,
         close_pane_if_empty: bool,
         cx: &mut ViewContext<Self>,
     ) {
+        let Some(item_index) = self.index_for_item_id(item_id) else {
+            return;
+        };
         self._remove_item(item_index, activate_pane, close_pane_if_empty, None, cx)
     }
 
@@ -1615,7 +1613,9 @@ impl Pane {
                             .await?
                     }
                     Ok(1) => {
-                        pane.update(cx, |pane, cx| pane.remove_item(item_ix, false, false, cx))?;
+                        pane.update(cx, |pane, cx| {
+                            pane.remove_item(item.item_id(), false, false, cx)
+                        })?;
                     }
                     _ => return Ok(false),
                 }
@@ -1709,9 +1709,7 @@ impl Pane {
                 if let Some(abs_path) = abs_path.await.ok().flatten() {
                     pane.update(cx, |pane, cx| {
                         if let Some(item) = pane.item_for_path(abs_path.clone(), cx) {
-                            if let Some(idx) = pane.index_for_item(&*item) {
-                                pane.remove_item(idx, false, false, cx);
-                            }
+                            pane.remove_item(item.item_id(), false, false, cx);
                         }
 
                         item.save_as(project, abs_path, cx)
@@ -1777,15 +1775,15 @@ impl Pane {
         entry_id: ProjectEntryId,
         cx: &mut ViewContext<Pane>,
     ) -> Option<()> {
-        let (item_index_to_delete, item_id) = self.items().enumerate().find_map(|(i, item)| {
+        let item_id = self.items().find_map(|item| {
             if item.is_singleton(cx) && item.project_entry_ids(cx).as_slice() == [entry_id] {
-                Some((i, item.item_id()))
+                Some(item.item_id())
             } else {
                 None
             }
         })?;
 
-        self.remove_item(item_index_to_delete, false, true, cx);
+        self.remove_item(item_id, false, true, cx);
         self.nav_history.remove_item(item_id);
 
         Some(())

crates/workspace/src/workspace.rs

@@ -3723,7 +3723,7 @@ impl Workspace {
 
             let mut new_item = task.await?;
             pane.update(cx, |pane, cx| {
-                let mut item_ix_to_remove = None;
+                let mut item_to_remove = None;
                 for (ix, item) in pane.items().enumerate() {
                     if let Some(item) = item.to_followable_item_handle(cx) {
                         match new_item.dedup(item.as_ref(), cx) {
@@ -3733,7 +3733,7 @@ impl Workspace {
                                 break;
                             }
                             Some(item::Dedup::ReplaceExisting) => {
-                                item_ix_to_remove = Some(ix);
+                                item_to_remove = Some((ix, item.item_id()));
                                 break;
                             }
                             None => {}
@@ -3741,8 +3741,8 @@ impl Workspace {
                     }
                 }
 
-                if let Some(ix) = item_ix_to_remove {
-                    pane.remove_item(ix, false, false, cx);
+                if let Some((ix, id)) = item_to_remove {
+                    pane.remove_item(id, false, false, cx);
                     pane.add_item(new_item.boxed_clone(), false, false, Some(ix), cx);
                 }
             })?;