From d784600fd6a6e409d791838d71cf6f120ef3cd28 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 29 Mar 2026 07:06:04 -0700 Subject: [PATCH] terminal: update page_serial_min in erasePage 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. --- src/terminal/PageList.zig | 82 ++++++++++++++++++++++++---------- src/terminal/Screen.zig | 30 +++++++------ src/terminal/Terminal.zig | 2 +- src/terminal/search/screen.zig | 44 ++++++++++++++++++ src/termio/Termio.zig | 5 +-- 5 files changed, 121 insertions(+), 42 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index bcdf2b6a3..398733745 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -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). diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index ea8ca1445..77e05b092 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -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 diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index c09eb9981..99536e7ab 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -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), } } diff --git a/src/terminal/search/screen.zig b/src/terminal/search/screen.zig index e98ecd958..3e7e316fa 100644 --- a/src/terminal/search/screen.zig +++ b/src/terminal/search/screen.zig @@ -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); +} diff --git a/src/termio/Termio.zig b/src/termio/Termio.zig index f05379077..75ccb94b5 100644 --- a/src/termio/Termio.zig +++ b/src/termio/Termio.zig @@ -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, ); }