mirrors/ghostty
1
0
Fork 0
mirror of https://github.com/ghostty-org/ghostty.git synced 2026-04-06 07:38:21 +00:00
Code Issues Packages Projects Releases Wiki Activity

terminal: update page_serial_min in erasePage

Browse Source 
Fixes #11957

erasePage now updates page_serial_min when the first page is erased,
and asserts that only front or back pages are erased since
page_serial_min cannot represent serial gaps from middle erasure.

To enforce this invariant at the API level, PageList.eraseRows is
now private. Two public wrappers replace it: eraseHistory always
starts from the beginning of history, and eraseActive takes a y
coordinate (with bounds assertion) and always starts from the top
of the active area. This makes middle-page erasure impossible by
construction.
This commit is contained in:
Mitchell Hashimoto
2026-03-29 07:06:04 -07:00
parent 1c14b9615b
commit d784600fd6
5 changed files with 121 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

82
src/terminal/PageList.zig
View File

@@ -3734,12 +3734,36 @@ pub fn eraseRowBounded(
node.data.clearCells(&rows[node.data.size.rows - 1], 0, node.data.size.cols);
}
/// Erase the rows from the given top to bottom (inclusive). Erasing
/// the rows doesn't clear them but actually physically REMOVES the rows.
/// If the top or bottom point is in the middle of a page, the other
/// contents in the page will be preserved but the page itself will be
/// underutilized (size < capacity).
pub fn eraseRows(
/// Erase all history rows, optionally up to a bottom-left bound.
/// This always starts from the beginning of the history area.
pub fn eraseHistory(
self: *PageList,
bl_pt: ?point.Point,
) void {
self.eraseRows(.{ .history = .{} }, bl_pt);
}
/// Erase active area rows, from the top of the active area to the
/// given row (inclusive).
pub fn eraseActive(
self: *PageList,
y: size.CellCountInt,
) void {
assert(y < self.rows);
self.eraseRows(.{ .active = .{} }, .{ .active = .{ .y = y } });
}
/// Erase rows from tl_pt to bl_pt (inclusive), physically removing
/// them rather than just clearing their contents. If a point falls
/// in the middle of a page, remaining rows in that page are shifted
/// and the page becomes underutilized (size < capacity).
///
/// Callers must ensure that the erased range only removes pages from
/// the front or back of the linked list, never the middle. Middle-page
/// erasure would create serial gaps that page_serial_min cannot
/// represent, leaving dangling references in consumers such as search.
/// Use the public eraseHistory/eraseActive wrappers which enforce this.
fn eraseRows(
self: *PageList,
tl_pt: point.Point,
bl_pt: ?point.Point,
@@ -3843,16 +3867,29 @@ pub fn eraseRows(
self.fixupViewport(erased);
}
/// Erase a single page, freeing all its resources. The page can be
/// anywhere in the linked list but must NOT be the final page in the
/// entire list (i.e. must not make the list empty).
/// Erase a single page, freeing all its resources. The page must be
/// at the front or back of the linked list (not the middle) and must
/// NOT be the final page in the entire list (i.e. must not make the
/// list empty).
///
/// IMPORTANT: This function does NOT update `total_rows`. The caller is
/// responsible for accounting for the removed rows before or after calling
/// this function.
fn erasePage(self: *PageList, node: *List.Node) void {
// Must not be the final page.
assert(node.next != null or node.prev != null);
// We only support erasing from the front or back, never the middle.
// Middle erasure would create serial gaps that page_serial_min can't
// represent. If this ever needs to change, we'll need a more
// sophisticated invalidation mechanism.
assert(node.prev == null or node.next == null);
// If we're erasing the first page, update page_serial_min so that
// any external references holding this page's serial will know it
// has been invalidated.
if (node.prev == null) self.page_serial_min = node.next.?.serial;
// Update any tracked pins to move to the previous or next page.
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
@@ -7140,10 +7177,7 @@ test "PageList eraseRows invalidates viewport offset cache" {
// This removes rows from before our pin, which changes its absolute
// offset from the top, but the cache is not invalidated.
const rows_to_erase = page.capacity.rows / 2;
s.eraseRows(
.{ .history = .{} },
.{ .history = .{ .y = rows_to_erase - 1 } },
);
s.eraseHistory(.{ .history = .{ .y = rows_to_erase - 1 } });
try testing.expectEqual(Scrollbar{
.total = s.total_rows,
@@ -9318,7 +9352,7 @@ test "PageList erase" {
try testing.expect(s.total_rows > s.rows);
// Erase the entire history, we should be back to just our active set.
s.eraseRows(.{ .history = .{} }, null);
s.eraseHistory(null);
try testing.expectEqual(s.rows, s.total_rows);
// We should be back to just one page
@@ -9349,7 +9383,7 @@ test "PageList erase reaccounts page size" {
try testing.expect(s.page_size > start_size);
// Erase the entire history, we should be back to just our active set.
s.eraseRows(.{ .history = .{} }, null);
s.eraseHistory(null);
try testing.expectEqual(start_size, s.page_size);
}
@@ -9381,7 +9415,7 @@ test "PageList erase row with tracked pin resets to top-left" {
defer s.untrackPin(p);
// Erase the entire history, we should be back to just our active set.
s.eraseRows(.{ .history = .{} }, null);
s.eraseHistory(null);
try testing.expectEqual(s.rows, s.total_rows);
// Our pin should move to the first page
@@ -9402,7 +9436,7 @@ test "PageList erase row with tracked pin shifts" {
defer s.untrackPin(p);
// Erase only a few rows in our active
s.eraseRows(.{ .active = .{} }, .{ .active = .{ .y = 3 } });
s.eraseActive(3);
try testing.expectEqual(s.rows, s.total_rows);
// Our pin should move to the first page
@@ -9423,7 +9457,7 @@ test "PageList erase row with tracked pin is erased" {
defer s.untrackPin(p);
// Erase the entire history, we should be back to just our active set.
s.eraseRows(.{ .active = .{} }, .{ .active = .{ .y = 3 } });
s.eraseActive(3);
try testing.expectEqual(s.rows, s.total_rows);
// Our pin should move to the first page
@@ -9457,7 +9491,7 @@ test "PageList erase resets viewport to active if moves within active" {
try testing.expect(s.viewport == .top);
// Erase the entire history, we should be back to just our active set.
s.eraseRows(.{ .history = .{} }, null);
s.eraseHistory(null);
try testing.expect(s.viewport == .active);
}
@@ -9486,7 +9520,7 @@ test "PageList erase resets viewport if inside erased page but not active" {
try testing.expect(s.viewport == .top);
// Erase the entire history, we should be back to just our active set.
s.eraseRows(.{ .history = .{} }, .{ .history = .{ .y = 2 } });
s.eraseHistory(.{ .history = .{ .y = 2 } });
try testing.expect(s.viewport == .top);
}
@@ -9514,7 +9548,7 @@ test "PageList erase resets viewport to active if top is inside active" {
s.scroll(.{ .top = {} });
// Erase the entire history, we should be back to just our active set.
s.eraseRows(.{ .history = .{} }, null);
s.eraseHistory(null);
try testing.expect(s.viewport == .active);
}
@@ -9525,7 +9559,7 @@ test "PageList erase active regrows automatically" {
var s = try init(alloc, 80, 24, null);
defer s.deinit();
try testing.expect(s.totalRows() == s.rows);
s.eraseRows(.{ .active = .{} }, .{ .active = .{ .y = 10 } });
s.eraseActive(10);
try testing.expect(s.totalRows() == s.rows);
}
@@ -9547,7 +9581,7 @@ test "PageList erase a one-row active" {
};
}
s.eraseRows(.{ .active = .{} }, .{ .active = .{} });
s.eraseActive(0);
try testing.expectEqual(s.rows, s.total_rows);
// The row should be empty
@@ -13842,7 +13876,7 @@ test "PageList resize (no reflow) more cols remaps pins in backfill path" {
// Trim a history row so the first page has spare capacity.
// This triggers the backfill path in resizeWithoutReflowGrowCols.
s.eraseRows(.{ .history = .{} }, .{ .history = .{ .y = 0 } });
s.eraseHistory(.{ .history = .{ .y = 0 } });
try testing.expect(first_page.data.size.rows < first_page.data.capacity.rows);
// Ensure the resize takes the slow path (new capacity > current capacity).

30
src/terminal/Screen.zig
View File

@@ -1302,19 +1302,21 @@ pub inline fn viewportIsBottom(self: Screen) bool {
/// Erase the region specified by tl and br, inclusive. This will physically
/// erase the rows meaning the memory will be reclaimed (if the underlying
/// page is empty) and other rows will be shifted up.
pub inline fn eraseRows(
pub inline fn eraseHistory(
self: *Screen,
tl: point.Point,
bl: ?point.Point,
) void {
defer self.assertIntegrity();
self.pages.eraseHistory(bl);
self.cursorReload();
}
// Erase the rows
self.pages.eraseRows(tl, bl);
// Just to be safe, reset our cursor since it is possible depending
// on the points that our active area shifted so our pointers are
// invalid.
pub inline fn eraseActive(
self: *Screen,
y: size.CellCountInt,
) void {
defer self.assertIntegrity();
self.pages.eraseActive(y);
self.cursorReload();
}
@@ -1761,7 +1763,7 @@ pub inline fn resize(
// erase our history. This is because PageList always keeps at least
// a page size of history.
if (self.no_scrollback) {
self.pages.eraseRows(.{ .history = .{} }, null);
self.pages.eraseHistory(null);
}
// If our cursor was updated, we do a full reload so all our cursor
@@ -3880,7 +3882,7 @@ test "Screen eraseRows history" {
try testing.expectEqualStrings("1\n2\n3\n4\n5\n6", str);
}
s.eraseRows(.{ .history = .{} }, null);
s.eraseHistory(null);
{
const str = try s.dumpStringAlloc(alloc, .{ .active = .{} });
@@ -3914,7 +3916,7 @@ test "Screen eraseRows history with more lines" {
try testing.expectEqualStrings("A\nB\nC\n1\n2\n3\n4\n5\n6", str);
}
s.eraseRows(.{ .history = .{} }, null);
s.eraseHistory(null);
{
const str = try s.dumpStringAlloc(alloc, .{ .active = .{} });
@@ -3943,7 +3945,7 @@ test "Screen eraseRows active partial" {
try testing.expectEqualStrings("1\n2\n3", str);
}
s.eraseRows(.{ .active = .{} }, .{ .active = .{ .y = 1 } });
s.eraseActive(1);
{
const str = try s.dumpStringAlloc(alloc, .{ .active = .{} });
@@ -5655,7 +5657,7 @@ test "Screen: clear history with no history" {
defer s.deinit();
try s.testWriteString("4ABCD\n5EFGH\n6IJKL");
try testing.expect(s.pages.viewport == .active);
s.eraseRows(.{ .history = .{} }, null);
s.eraseHistory(null);
try testing.expect(s.pages.viewport == .active);
{
// Test our contents rotated
@@ -5689,7 +5691,7 @@ test "Screen: clear history" {
try testing.expectEqualStrings("1ABCD\n2EFGH\n3IJKL", contents);
}
s.eraseRows(.{ .history = .{} }, null);
s.eraseHistory(null);
try testing.expect(s.pages.viewport == .active);
{
// Test our contents rotated

2
src/terminal/Terminal.zig
View File

@@ -2601,7 +2601,7 @@ pub fn eraseDisplay(
assert(!self.screens.active.cursor.pending_wrap);
},
.scrollback => self.screens.active.eraseRows(.{ .history = .{} }, null),
.scrollback => self.screens.active.eraseHistory(null),
}
}

44
src/terminal/search/screen.zig
View File

@@ -1506,3 +1506,47 @@ test "reloadActive partial history cleanup on loop append error" {
// because FlattenedHighlight items weren't cleaned up.
try testing.expectError(error.OutOfMemory, search.select(.next));
}
test "select after clearing scrollback" {
// Regression test for: https://github.com/ghostty-org/ghostty/issues/11957
// After clearing scrollback (CSI 3J), selecting next/prev should not crash.
const alloc = testing.allocator;
var t: Terminal = try .init(alloc, .{
.cols = 10,
.rows = 2,
.max_scrollback = std.math.maxInt(usize),
});
defer t.deinit(alloc);
const list: *PageList = &t.screens.active.pages;
var s = t.vtStream();
defer s.deinit();
// Write enough content to push matches into scrollback history.
s.nextSlice("error\r\n");
while (list.totalPages() < 3) s.nextSlice("error\r\n");
for (0..list.rows) |_| s.nextSlice("\r\n");
s.nextSlice("error.");
// Start search and find all matches.
var search: ScreenSearch = try .init(alloc, t.screens.active, "error");
defer search.deinit();
try search.searchAll();
// Should have matches in both history and active areas.
try testing.expect(search.history_results.items.len > 0);
try testing.expect(search.active_results.items.len > 0);
// Select a match first (so we have a selection).
_ = try search.select(.next);
try testing.expect(search.selected != null);
// Clear scrollback (equivalent to CSI 3J / Cmd+K erasing scrollback).
t.eraseDisplay(.scrollback, false);
// Selecting next/prev after clearing scrollback should not crash.
// Before the fix, this would hit an assertion in trackPin because
// the FlattenedHighlight contained dangling node pointers.
_ = try search.select(.next);
_ = try search.select(.prev);
}

5
src/termio/Termio.zig
View File

@@ -577,9 +577,8 @@ pub fn clearScreen(self: *Termio, td: *ThreadData, history: bool) !void {
// If we're not at a prompt, we just delete above the cursor.
if (!self.terminal.cursorIsAtPrompt()) {
if (self.terminal.screens.active.cursor.y > 0) {
self.terminal.screens.active.eraseRows(
.{ .active = .{ .y = 0 } },
.{ .active = .{ .y = self.terminal.screens.active.cursor.y - 1 } },
self.terminal.screens.active.eraseActive(
self.terminal.screens.active.cursor.y - 1,
);
}