oak-team/oak-gpui
3
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Don't allow strong view handles to be read/updated with an AsyncAppContext

Browse Source 
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 <nathan@zed.dev>
This commit is contained in:
Antonio Scandurra
2023-04-26 13:36:13 +02:00
co-authored by Nathan Sobo
parent 689e878bd8
commit 6317e885c7
14 changed files with 129 additions and 163 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/workspace/src/dock.rs
-2
View File
@@ -811,8 +811,6 @@ mod tests {
}
impl BorrowWindowContext for DockTestContext<'_> {
type ReturnValue<T> = T;
fn read_with<T, F: FnOnce(&WindowContext) -> T>(&self, window_id: usize, f: F) -> T {
BorrowWindowContext::read_with(self.cx, window_id, f)
}
crates/workspace/src/pane.rs
+56 -68
View File
@@ -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<ViewHandle<Pane>>,
pane: Option<WeakViewHandle<Pane>>,
cx: &mut ViewContext<Workspace>,
) -> Task<Result<()>> {
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<ViewHandle<Pane>>,
pane: Option<WeakViewHandle<Pane>>,
cx: &mut ViewContext<Workspace>,
) -> Task<Result<()>> {
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<Result<()>> {
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>,
pane: WeakViewHandle<Pane>,
mode: NavigationMode,
cx: &mut ViewContext<Workspace>,
) -> Task<Result<()>> {
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<Project>,
pane: &ViewHandle<Pane>,
pane: &WeakViewHandle<Pane>,
item_ix: usize,
item: &dyn ItemHandle,
should_prompt_for_save: bool,
crates/workspace/src/persistence/model.rs
+19 -17
View File
@@ -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<Vec<PathBuf>>);
@@ -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<Project>,
pane_handle: &ViewHandle<Pane>,
pane_handle: &WeakViewHandle<Pane>,
workspace_id: WorkspaceId,
workspace: &WeakViewHandle<Workspace>,
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 {
crates/workspace/src/workspace.rs
+10 -9
View File
@@ -864,7 +864,7 @@ impl Workspace {
requesting_window_id: Option<usize>,
cx: &mut AppContext,
) -> Task<(
ViewHandle<Workspace>,
WeakViewHandle<Workspace>,
Vec<Option<Result<Box<dyn ItemHandle>, 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<Workspace>, cx: &mut AsyncAppContext) {
fn notify_if_database_failed(workspace: &WeakViewHandle<Workspace>, 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<Workspace>);
pub fn activate_workspace_for_project(
cx: &mut AppContext,
predicate: impl Fn(&mut Project, &mut ModelContext<Project>) -> bool,
) -> Option<ViewHandle<Workspace>> {
) -> Option<WeakViewHandle<Workspace>> {
for window_id in cx.window_ids().collect::<Vec<_>>() {
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<Workspace>,
WeakViewHandle<Workspace>,
Vec<Option<Result<Box<dyn ItemHandle>, 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();