Fix split diff stale scroll anchor crash (#58688)
This crash occurred when the left hand side (LHS) editor in the split diff retained a stale shared scroll handler after the split editor unsplits. I added an invariant check to our randomized companion display map tests to prevent regressions here and explicitly make LHS editor set a native scroll anchor when unsplitting. Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - Fix crash that could occur when unsplitting a side by side diff view
This commit is contained in:
1 parent
6942dc2faa
commit
34c47afbc1
2 files changed
+157
-18
No files matched your search
@@ -86,8 +86,13 @@ impl SharedScrollAnchor {
|
||||
let snapshot = if let Some(display_map_id) = self.display_map_id
|
||||
&& display_map_id != snapshot.display_map_id
|
||||
{
|
||||
let companion_snapshot = snapshot.companion_snapshot().unwrap();
|
||||
assert_eq!(companion_snapshot.display_map_id, display_map_id);
|
||||
let companion_snapshot = snapshot
|
||||
.companion_snapshot()
|
||||
.expect("shared scroll anchor references a non native display map, but snapshot has no companion");
|
||||
assert_eq!(
|
||||
companion_snapshot.display_map_id, display_map_id,
|
||||
"shared scroll anchor display map should match the snapshot's split companion"
|
||||
);
|
||||
companion_snapshot
|
||||
} else {
|
||||
snapshot
|
||||
@@ -100,8 +105,13 @@ impl SharedScrollAnchor {
|
||||
let snapshot = if let Some(display_map_id) = self.display_map_id
|
||||
&& display_map_id != snapshot.display_map_id
|
||||
{
|
||||
let companion_snapshot = snapshot.companion_snapshot().unwrap();
|
||||
assert_eq!(companion_snapshot.display_map_id, display_map_id);
|
||||
let companion_snapshot = snapshot
|
||||
.companion_snapshot()
|
||||
.expect("shared scroll anchor references a non native display map, but snapshot has no companion");
|
||||
assert_eq!(
|
||||
companion_snapshot.display_map_id, display_map_id,
|
||||
"shared scroll anchor display map should match the snapshot's split companion"
|
||||
);
|
||||
companion_snapshot
|
||||
} else {
|
||||
snapshot
|
||||
@@ -294,8 +304,14 @@ impl ScrollManager {
|
||||
let mut result = if let Some(display_map_id) = shared.display_map_id
|
||||
&& display_map_id != snapshot.display_map_id
|
||||
{
|
||||
let companion_snapshot = snapshot.companion_snapshot().unwrap();
|
||||
assert_eq!(companion_snapshot.display_map_id, display_map_id);
|
||||
let companion_snapshot = snapshot
|
||||
.companion_snapshot()
|
||||
.expect("shared scroll anchor references a non native display map, but the snapshot has no companion");
|
||||
assert_eq!(
|
||||
companion_snapshot.display_map_id, display_map_id,
|
||||
"shared scroll anchor display map should match the companion used for native anchor conversion"
|
||||
);
|
||||
|
||||
let mut display_point = shared
|
||||
.scroll_anchor
|
||||
.anchor
|
||||
@@ -337,6 +353,14 @@ impl ScrollManager {
|
||||
self.anchor = entity;
|
||||
}
|
||||
|
||||
pub fn unshare_scroll_anchor(&mut self, snapshot: &DisplaySnapshot, cx: &mut Context<Editor>) {
|
||||
let scroll_anchor = self.native_anchor(snapshot, cx);
|
||||
self.anchor = cx.new(|_| SharedScrollAnchor {
|
||||
scroll_anchor,
|
||||
display_map_id: Some(snapshot.display_map_id),
|
||||
});
|
||||
}
|
||||
|
||||
pub fn ongoing_scroll(&self) -> OngoingScroll {
|
||||
self.ongoing
|
||||
}
|
||||
|
||||
+127
-12
@@ -1043,6 +1043,17 @@ impl SplittableEditor {
|
||||
let Some(lhs) = self.lhs.take() else {
|
||||
return;
|
||||
};
|
||||
|
||||
// Detach the stale lhs editor from the shared scroll anchor while the split companion still exists,
|
||||
// so its anchor can be converted to lhs native before rhs tears down split specific state.
|
||||
lhs.editor.update(cx, |editor, cx| {
|
||||
let lhs_snapshot = editor.display_map.update(cx, |dm, cx| dm.snapshot(cx));
|
||||
editor
|
||||
.scroll_manager
|
||||
.unshare_scroll_anchor(&lhs_snapshot, cx);
|
||||
editor.set_on_local_selections_changed(None);
|
||||
});
|
||||
|
||||
self.rhs_editor.update(cx, |rhs, cx| {
|
||||
let rhs_snapshot = rhs.display_map.update(cx, |dm, cx| dm.snapshot(cx));
|
||||
let native_anchor = rhs.scroll_manager.native_anchor(&rhs_snapshot, cx);
|
||||
@@ -1064,9 +1075,6 @@ impl SplittableEditor {
|
||||
dm.set_companion(None, cx);
|
||||
});
|
||||
});
|
||||
lhs.editor.update(cx, |editor, _cx| {
|
||||
editor.set_on_local_selections_changed(None);
|
||||
});
|
||||
cx.notify();
|
||||
}
|
||||
|
||||
@@ -1319,19 +1327,108 @@ impl SplittableEditor {
|
||||
use crate::display_map::Block;
|
||||
use crate::display_map::DisplayRow;
|
||||
|
||||
let rhs_snapshot = self
|
||||
.rhs_editor
|
||||
.update(cx, |editor, cx| editor.display_snapshot(cx));
|
||||
|
||||
let Some(lhs) = self.lhs.as_ref() else {
|
||||
assert!(
|
||||
rhs_snapshot.companion_snapshot().is_none(),
|
||||
"rhs display snapshot should not have a companion when unsplit"
|
||||
);
|
||||
|
||||
let shared_scroll_anchor = self
|
||||
.rhs_editor
|
||||
.read(cx)
|
||||
.scroll_manager
|
||||
.shared_scroll_anchor(cx);
|
||||
if let Some(display_map_id) = shared_scroll_anchor.display_map_id {
|
||||
assert_eq!(
|
||||
display_map_id, rhs_snapshot.display_map_id,
|
||||
"unsplit editor should not retain a scroll anchor native to a torn-down split companion"
|
||||
);
|
||||
}
|
||||
|
||||
let _ = self
|
||||
.rhs_editor
|
||||
.read(cx)
|
||||
.scroll_manager
|
||||
.native_anchor(&rhs_snapshot, cx);
|
||||
return;
|
||||
};
|
||||
|
||||
self.debug_print(cx);
|
||||
self.check_excerpt_invariants(quiesced, cx);
|
||||
|
||||
let lhs = self.lhs.as_ref().unwrap();
|
||||
let lhs_snapshot = lhs
|
||||
.editor
|
||||
.update(cx, |editor, cx| editor.display_snapshot(cx));
|
||||
|
||||
let lhs_companion = lhs_snapshot
|
||||
.companion_snapshot()
|
||||
.expect("lhs display snapshot should have rhs companion while split");
|
||||
assert_eq!(
|
||||
lhs_companion.display_map_id, rhs_snapshot.display_map_id,
|
||||
"lhs display snapshot companion should point to rhs display map"
|
||||
);
|
||||
assert!(
|
||||
lhs_companion.companion_snapshot().is_none(),
|
||||
"embedded companion snapshot should not recursively contain another companion"
|
||||
);
|
||||
|
||||
let rhs_companion = rhs_snapshot
|
||||
.companion_snapshot()
|
||||
.expect("rhs display snapshot should have lhs companion while split");
|
||||
assert_eq!(
|
||||
rhs_companion.display_map_id, lhs_snapshot.display_map_id,
|
||||
"rhs display snapshot companion should point to lhs display map"
|
||||
);
|
||||
assert!(
|
||||
rhs_companion.companion_snapshot().is_none(),
|
||||
"embedded companion snapshot should not recursively contain another companion"
|
||||
);
|
||||
|
||||
let lhs_scroll_anchor_entity_id = lhs
|
||||
.editor
|
||||
.read(cx)
|
||||
.scroll_manager
|
||||
.scroll_anchor_entity()
|
||||
.entity_id();
|
||||
let rhs_scroll_anchor_entity_id = self
|
||||
.rhs_editor
|
||||
.read(cx)
|
||||
.scroll_manager
|
||||
.scroll_anchor_entity()
|
||||
.entity_id();
|
||||
assert_eq!(
|
||||
lhs_scroll_anchor_entity_id, rhs_scroll_anchor_entity_id,
|
||||
"split editors should share a scroll anchor entity"
|
||||
);
|
||||
|
||||
let shared_scroll_anchor = self
|
||||
.rhs_editor
|
||||
.read(cx)
|
||||
.scroll_manager
|
||||
.shared_scroll_anchor(cx);
|
||||
if let Some(display_map_id) = shared_scroll_anchor.display_map_id {
|
||||
assert!(
|
||||
display_map_id == lhs_snapshot.display_map_id
|
||||
|| display_map_id == rhs_snapshot.display_map_id,
|
||||
"shared scroll anchor should be native to one side of the split"
|
||||
);
|
||||
}
|
||||
let _ = lhs
|
||||
.editor
|
||||
.read(cx)
|
||||
.scroll_manager
|
||||
.native_anchor(&lhs_snapshot, cx);
|
||||
let _ = self
|
||||
.rhs_editor
|
||||
.read(cx)
|
||||
.scroll_manager
|
||||
.native_anchor(&rhs_snapshot, cx);
|
||||
|
||||
if quiesced {
|
||||
let lhs_snapshot = lhs
|
||||
.editor
|
||||
.update(cx, |editor, cx| editor.display_snapshot(cx));
|
||||
let rhs_snapshot = self
|
||||
.rhs_editor
|
||||
.update(cx, |editor, cx| editor.display_snapshot(cx));
|
||||
|
||||
let lhs_max_row = lhs_snapshot.max_point().row();
|
||||
let rhs_max_row = rhs_snapshot.max_point().row();
|
||||
assert_eq!(lhs_max_row, rhs_max_row, "mismatch in display row count");
|
||||
@@ -2406,11 +2503,29 @@ mod tests {
|
||||
}
|
||||
}
|
||||
75..=79 => {
|
||||
log::info!("unsplit and resplit");
|
||||
log::info!("unsplit, scroll stale lhs, and resplit");
|
||||
let Some(lhs_editor) = editor.update(cx, |editor, _cx| {
|
||||
editor.lhs.as_ref().map(|lhs| lhs.editor.clone())
|
||||
}) else {
|
||||
continue;
|
||||
};
|
||||
let lhs_max_row = lhs_editor.update(cx, |editor, cx| {
|
||||
editor.display_snapshot(cx).max_point().row().0
|
||||
});
|
||||
editor.update_in(cx, |editor, window, cx| {
|
||||
editor.unsplit(window, cx);
|
||||
});
|
||||
cx.run_until_parked();
|
||||
|
||||
if lhs_max_row > 0 {
|
||||
lhs_editor.update_in(cx, |editor, window, cx| {
|
||||
editor.set_scroll_position(gpui::Point::new(0., 1.), window, cx);
|
||||
});
|
||||
editor.update(cx, |editor, cx| {
|
||||
editor.check_invariants(false, cx);
|
||||
});
|
||||
}
|
||||
|
||||
editor.update_in(cx, |editor, window, cx| {
|
||||
editor.split(window, cx);
|
||||
});
|
||||
|
||||
Reference in new issue
Block a user