diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 1ba939f8f..79aa96f18 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -365,23 +365,31 @@ pool: MemoryPool, /// The list of pages in the screen. pages: List, -/// A monotonically increasing serial number that is incremented each -/// time a page is allocated or reused as new. The serial is assigned to -/// the Node. +/// The next globally unique reference generation for this PageList. A +/// generation is assigned whenever a page is allocated, reused as new, or +/// changed in place such that existing page coordinates are no longer stable. /// /// The serial number can be used to detect whether the page is identical /// to the page that was originally referenced by a pointer. Since we reuse /// and pool memory, pointer stability is not guaranteed, but the serial -/// will always be different for different allocations. +/// will always be different for different page generations. /// /// Developer note: we never do overflow checking on this. If we created /// a new page every second it'd take 584 billion years to overflow. We're /// going to risk it. page_serial: u64, -/// The lowest still valid serial number that could exist. This allows -/// for quick comparisons to find invalid pages in references. -page_serial_min: u64, +/// The first serial in the current whole-list validity epoch. Only `reset` +/// advances this value, immediately before rebuilding every page. It is not +/// the generation of the first page or the exact minimum live generation. +/// Page generations are not monotonic in list order: replacement and split +/// operations can put a fresh generation before older live pages. +/// +/// A generation below this epoch is definitely invalid, allowing O(1) +/// rejection in `nodeIsValid` and bulk removal of pre-reset search results. +/// A generation at or above it is only potentially valid and must still be +/// checked against the live list before its coordinates are used. +page_serial_epoch: u64, /// Byte size of the raw backing mappings owned by active page nodes. This is /// logical scrollback accounting and does not change while a mapping is @@ -653,7 +661,7 @@ pub fn init( .pool = pool, .pages = page_list, .page_serial = page_serial, - .page_serial_min = 0, + .page_serial_epoch = 0, .page_size = page_size, .explicit_max_size = max_size orelse std.math.maxInt(usize), .min_max_size = min_max_size, @@ -810,12 +818,11 @@ fn verifyIntegrity(self: *const PageList) IntegrityError!void { actual_total += node.rows(); node_ = node.next; - // While doing this traversal, verify no node has a serial - // number lower than our min. - if (node.serial < self.page_serial_min) { + // Every live node must belong to the current validity epoch. + if (node.serial < self.page_serial_epoch) { log.warn( - "PageList integrity violation: page serial too low serial={} min={}", - .{ node.serial, self.page_serial_min }, + "PageList integrity violation: page serial predates epoch serial={} epoch={}", + .{ node.serial, self.page_serial_epoch }, ); return IntegrityError.PageSerialInvalid; } @@ -916,10 +923,10 @@ pub fn reset(self: *PageList) void { // Reset discards all scrollback, so there is nothing left to compress. self.page_compression.reset(); - // Invalidate all external page refs to the previous list. The reset below - // rebuilds the page list from the pools, so old untracked refs must be - // rejected before any validation attempts to inspect their node pointers. - self.page_serial_min = self.page_serial; + // Begin a new whole-list validity epoch before rebuilding from the pools. + // Every old reference now has a serial below the epoch and can be rejected + // in O(1), even if the node pool later reuses its pointer address. + self.page_serial_epoch = self.page_serial; // We need enough pages/nodes to keep our active area. This should // never fail since we by definition have allocated a page already @@ -1149,7 +1156,7 @@ pub fn clone( .pool = pool, .pages = page_list, .page_serial = page_serial, - .page_serial_min = 0, + .page_serial_epoch = 0, .page_size = page_size, .explicit_max_size = self.explicit_max_size, .min_max_size = self.min_max_size, @@ -2722,6 +2729,7 @@ fn trimTrailingBlankRows( max: size.CellCountInt, ) size.CellCountInt { var trimmed: size.CellCountInt = 0; + var invalidated_node: ?*List.Node = null; const bl_pin = self.getBottomRight(.screen).?; var it = bl_pin.rowIterator(.left_up, null); while (it.next()) |row_pin| { @@ -2743,6 +2751,11 @@ fn trimTrailingBlankRows( // No text, we can trim this row. Because it has // no text we can also be sure it has no styling // so we don't need to worry about memory. + if (row_pin.node.rows() > 1 and invalidated_node != row_pin.node) { + // Shrinking a retained page changes its valid row-coordinate range. + self.invalidateNodeLayout(row_pin.node); + invalidated_node = row_pin.node; + } row_pin.node.page().size.rows -= 1; if (row_pin.node.page().size.rows == 0) { self.erasePage(row_pin.node); @@ -3093,6 +3106,49 @@ pub fn scrollClear(self: *PageList) Allocator.Error!void { for (0..non_empty) |_| _ = try self.grow(); } +/// Give a live node a new generation before changing its coordinate layout in +/// place. +/// +/// This must be called when a node remains at the same address and stays in the +/// list, but a mutation changes which logical row a `(node, y)` coordinate +/// identifies or whether that coordinate is still in range. Examples include +/// rotating rows after an erase, truncating a page's row range, reinitializing +/// the sole remaining page, or shortening the source page of a split. Without +/// a new generation, a cached pointer, serial, and coordinate could still pass +/// `nodeIsValid` while referring to a different row than it originally did. +/// Screen and Terminal fast paths which manipulate Page rows directly must use +/// this because they bypass PageList's own row-mutation helpers. +/// +/// The caller must pass a node which is currently live in this PageList. Call +/// this at the point the operation commits to changing the layout and before +/// the first such change. When an operation is intended to be atomic, finish +/// its failable preparation first so a failed operation does not needlessly +/// invalidate references. One call per affected node is sufficient when an +/// operation performs several layout changes without exposing intermediate +/// references. +/// +/// This helper is only needed for in-place changes. Removing or replacing a +/// node already invalidates old references through the live-list check in +/// `nodeIsValid`, and newly allocated or reused nodes receive a fresh serial as +/// part of their initialization. Ordinary cell or style changes which preserve +/// the meaning of page coordinates do not require a new generation solely for +/// that reason. +/// +/// The only state changed here is `node.serial` and the next-generation counter +/// `page_serial`. Consequently, every previously captured pointer-plus-serial +/// pair for this node becomes invalid while new references can capture the new +/// generation. This does not advance `page_serial_epoch`: only `reset` starts a +/// new whole-list validity epoch. +/// +/// This also does not mutate the page, update tracked pins or viewport state, +/// adjust row or memory accounting, mark cells dirty, or notify incremental +/// compression. The surrounding operation remains responsible for all such +/// bookkeeping required by its layout change. +pub fn invalidateNodeLayout(self: *PageList, node: *List.Node) void { + node.serial = self.page_serial; + self.page_serial += 1; +} + /// Compact a page to use the minimum required memory for the contents /// it stores. Returns the new node pointer if compaction occurred, or null /// if the page was already compact or compaction would not provide any @@ -3164,6 +3220,7 @@ pub fn compact(self: *PageList, node: *List.Node) Allocator.Error!?*List.Node { self.pages.insertBefore(node, new_node); self.pages.remove(node); self.destroyNode(node); + self.page_compression.markActivity(); new_page.assertIntegrity(); return new_node; @@ -3237,6 +3294,10 @@ pub fn split( // error handling. It is possible but we haven't written it. errdefer comptime unreachable; + // Failable split work is complete; shortening the source changes its row range. + self.invalidateNodeLayout(original_node); + self.page_compression.markActivity(); + // Move any tracked pins from the copied rows for (self.tracked_pins.keys()) |tracked| { if (tracked.node.page() != page or @@ -3537,11 +3598,10 @@ pub fn grow(self: *PageList) Allocator.Error!?*List.Node { self.pages.insertAfter(last, first); self.total_rows += 1; - // We also need to reset the serial number. Since this is the only - // place we ever reuse a serial number, we also can safely set - // page_serial_min to be one more than the old serial because we - // only ever prune the oldest pages. - self.page_serial_min = first.serial + 1; + // Reusing the node gives it a fresh generation. Do not begin a new + // page_serial_epoch here: generations are not monotonic in list order, + // so older live successors may have lower generations. The epoch only + // advances when reset invalidates the entire list. first.serial = self.page_serial; self.page_serial += 1; @@ -3703,6 +3763,7 @@ pub fn increaseCapacity( self.pages.insertBefore(node, new_node); self.pages.remove(node); self.destroyNode(node); + self.page_compression.markActivity(); new_page.assertIntegrity(); return new_node; @@ -4272,9 +4333,16 @@ pub fn eraseRow( var page = node.page(); var rows = page.rows.ptr(page.memory.ptr); + // Erasing history may restore compressed pages. Mark unconditionally + // because incrementing the activity token is cheaper than locating the + // active boundary. + self.page_compression.markActivity(); + // In order to move the following rows up we rotate the rows array by 1. // The rotate operation turns e.g. [ 0 1 2 3 ] in to [ 1 2 3 0 ], which // works perfectly to move all of our elements where they belong. + // Rotating rows changes which logical row cached coordinates identify. + self.invalidateNodeLayout(node); fastmem.rotateOnce(Row, rows[pn.y..node.rows()]); // We adjust the tracked pins in this page, moving up any that were below @@ -4326,6 +4394,8 @@ pub fn eraseRow( page = next_page; rows = next_rows; + // Rotating this page moves every cached row coordinate up by one. + self.invalidateNodeLayout(node); fastmem.rotateOnce(Row, rows[0..node.rows()]); // Mark the whole page as dirty. @@ -4374,10 +4444,17 @@ pub fn eraseRowBounded( var page = node.page(); var rows = page.rows.ptr(page.memory.ptr); + // Erasing history may restore compressed pages. Mark unconditionally + // because incrementing the activity token is cheaper than locating the + // active boundary. + self.page_compression.markActivity(); + // If the row limit is less than the remaining rows before the end of the // page, then we clear the row, rotate it to the end of the boundary limit // and update our pins. if (node.rows() - pn.y > limit) { + // Rotating this bounded region changes its cached row coordinates. + self.invalidateNodeLayout(node); page.clearCells(&rows[pn.y], 0, node.cols()); fastmem.rotateOnce(Row, rows[pn.y..][0 .. limit + 1]); @@ -4420,6 +4497,8 @@ pub fn eraseRowBounded( return; } + // Rotating this suffix changes which logical row its coordinates identify. + self.invalidateNodeLayout(node); fastmem.rotateOnce(Row, rows[pn.y..node.rows()]); // Mark the whole page as dirty. @@ -4480,6 +4559,8 @@ pub fn eraseRowBounded( // The logic here is very similar to the one before the loop. const shifted_limit = limit - shifted; if (node.rows() > shifted_limit) { + // Rotating this bounded prefix changes its cached row coordinates. + self.invalidateNodeLayout(node); page.clearCells(&rows[0], 0, node.cols()); fastmem.rotateOnce(Row, rows[0 .. shifted_limit + 1]); @@ -4515,6 +4596,8 @@ pub fn eraseRowBounded( return; } + // Rotating the whole page moves every cached row coordinate up by one. + self.invalidateNodeLayout(node); fastmem.rotateOnce(Row, rows[0..node.rows()]); // Mark the whole page as dirty. @@ -4576,9 +4659,8 @@ pub fn eraseActive( /// 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. +/// the front or back of the linked list, never the middle. The pin and row +/// accounting in this operation is only defined for those boundary ranges. /// Use the public eraseHistory/eraseActive wrappers which enforce this. fn eraseRows( self: *PageList, @@ -4586,6 +4668,7 @@ fn eraseRows( bl_pt: ?point.Point, ) void { defer self.assertIntegrity(); + self.page_compression.markActivity(); // The count of rows that was erased. var erased: usize = 0; @@ -4603,6 +4686,8 @@ fn eraseRows( // page so to handle this we reinit this page, set it to zero // size which will let us grow our active area back. if (chunk.node.next == null and chunk.node.prev == null) { + // Reinitializing the sole page invalidates every coordinate in it. + self.invalidateNodeLayout(chunk.node); const page = chunk.node.page(); erased += page.size.rows; page.reinit(); @@ -4615,6 +4700,9 @@ fn eraseRows( continue; } + // Moving the retained suffix changes every captured row coordinate. + self.invalidateNodeLayout(chunk.node); + // We are modifying our chunk so make sure it is in a good state. const page = chunk.node.page(); defer page.assertIntegrity(); @@ -4697,17 +4785,10 @@ 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. + // We only support erasing from the front or back, never the middle. The + // public erase operations maintain this contract by construction. 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| { @@ -4809,7 +4890,9 @@ pub fn nodeIsValid( target: *List.Node, serial: u64, ) bool { - if (serial < self.page_serial_min) return false; + // Reset invalidates the whole prior epoch, so reject those generations + // without scanning the live list. + if (serial < self.page_serial_epoch) return false; var it = self.pages.first; while (it) |node| : (it = node.next) { @@ -6554,6 +6637,27 @@ fn growColdPagesForTest(self: *PageList, count: usize) !void { } } +/// Fill the current tail page to capacity without allocating a successor. +/// Capturing the tail before the loop makes this stop at the allocation +/// boundary needed by bounded-pruning tests. +fn fillLastPageForTest(self: *PageList) !void { + const last = self.pages.last.?; + while (last.rows() < last.capacity().rows) _ = try self.grow(); +} + +/// Verify every live page belongs to the current validity epoch, has an +/// allocated generation below the next serial, and validates through the same +/// pointer-plus-generation lookup used by external references. +fn expectLivePageSerialsValidForTest(self: *const PageList) !void { + const testing = std.testing; + var node = self.pages.first; + while (node) |live| : (node = live.next) { + try testing.expect(live.serial >= self.page_serial_epoch); + try testing.expect(live.serial < self.page_serial); + try testing.expect(self.nodeIsValid(live, live.serial)); + } +} + test "PageList Pin rightWrap exact row multiple" { const testing = std.testing; @@ -6733,6 +6837,34 @@ test "PageList owns incremental compression state" { ); } +test "PageList replacements preserve compression continuation and mark activity" { + const testing = std.testing; + + var s = try init(testing.allocator, 80, 24, null); + defer s.deinit(); + + const state: IncrementalCompressionState = .{ + .flags = .{ .verifying = true }, + .activity_serial = 42, + .last_serial = 7, + .next_serial = 8, + }; + const expected: IncrementalCompressionState = .{ + .flags = .{ .verifying = true }, + .activity_serial = 43, + .last_serial = 7, + .next_serial = 8, + }; + + s.page_compression = state; + const replacement = try s.increaseCapacity(s.pages.first.?, null); + try testing.expectEqual(expected, s.page_compression); + + s.page_compression = state; + _ = (try s.compact(replacement)).?; + try testing.expectEqual(expected, s.page_compression); +} + test "PageList incremental compression bounds inspected pages" { const testing = std.testing; @@ -6968,6 +7100,180 @@ test "PageList incremental compression restarts after prune reuse" { try testing.expectEqual(@as(usize, 1), s.memoryStats().compressed_pages); } +test "PageList bounded pruning after partial erase preserves live serials" { + const testing = std.testing; + + var s = try init( + testing.allocator, + 80, + 24, + 2 * PagePool.item_size, + ); + defer s.deinit(); + + while (s.totalPages() < 2) _ = try s.grow(); + const first = s.pages.first.?; + const old_serial = first.serial; + const old_rows = first.rows(); + + s.eraseHistory(.{ .history = .{ .y = 0 } }); + try testing.expectEqual(first, s.pages.first.?); + try testing.expectEqual(old_rows - 1, first.rows()); + try testing.expect(!s.nodeIsValid(first, old_serial)); + + try s.fillLastPageForTest(); + _ = try s.grow(); + try s.expectLivePageSerialsValidForTest(); +} + +test "PageList partial erase restarts compression before continuation" { + const testing = std.testing; + + var s = try init(testing.allocator, 80, 24, null); + defer s.deinit(); + try s.growColdPagesForTest(incremental_compression_max_inspected + 1); + _ = s.compress(.full); + + const first = s.pages.first.?; + try testing.expect(first.isCompressed()); + + var marker = first; + for (1..incremental_compression_max_inspected) |_| marker = marker.next.?; + s.page_compression = .{ + .flags = .{ .verifying = true }, + .last_serial = marker.serial, + .next_serial = s.page_serial, + }; + + const activity = s.page_compression.activity_serial; + s.eraseHistory(.{ .history = .{ .y = 0 } }); + try testing.expect(!first.isCompressed()); + try testing.expect(activity != s.page_compression.activity_serial); + + // The changed generation is before the saved marker, so continuation must + // restart and recompress it instead of reporting verification complete. + try testing.expectEqual( + IncrementalCompressionResult.pending, + s.compress(.incremental), + ); + try testing.expect(first.isCompressed()); +} + +test "PageList bounded pruning after split invalidation preserves live serials" { + const testing = std.testing; + + var s = try init( + testing.allocator, + 80, + 24, + 2 * PagePool.item_size, + ); + defer s.deinit(); + + while (s.totalPages() < 2) _ = try s.grow(); + const first = s.pages.first.?; + const old_serial = first.serial; + const activity = s.page_compression.activity_serial; + + try s.split(.{ + .node = first, + .y = first.rows() / 2, + .x = 0, + }); + try testing.expect(!s.nodeIsValid(first, old_serial)); + try testing.expect(activity != s.page_compression.activity_serial); + + try s.fillLastPageForTest(); + _ = try s.grow(); + try s.expectLivePageSerialsValidForTest(); +} + +test "PageList repeated bounded pruning after split preserves live serials" { + const testing = std.testing; + + var s = try init( + testing.allocator, + 80, + 24, + 3 * PagePool.item_size, + ); + defer s.deinit(); + + const epoch = s.page_serial_epoch; + while (s.totalPages() < 3) _ = try s.grow(); + const first = s.pages.first.?; + try s.split(.{ + .node = first, + .y = first.rows() / 2, + .x = 0, + }); + + // The split target has a fresh serial but precedes older successor pages. + // Prune both the old source and then that target while verifying ordinary + // list mutation does not advance the whole-list validity epoch. + for (0..2) |_| { + while (s.pages.last.?.rows() < s.pages.last.?.capacity().rows) { + _ = try s.grow(); + } + _ = try s.grow(); + + // Ordinary pruning invalidates one generation at a time through live + // list validation; only reset may begin a new whole-list epoch. + try testing.expectEqual(epoch, s.page_serial_epoch); + try s.expectLivePageSerialsValidForTest(); + } +} + +test "PageList bounded pruning after front replacement preserves live serials" { + const testing = std.testing; + + var s = try init( + testing.allocator, + 80, + 24, + 2 * PagePool.item_size, + ); + defer s.deinit(); + + while (s.totalPages() < 2) _ = try s.grow(); + const old = s.pages.first.?; + const old_serial = old.serial; + const replacement = try s.increaseCapacity(old, null); + try testing.expect(replacement != old); + try testing.expect(!s.nodeIsValid(old, old_serial)); + + try s.fillLastPageForTest(); + _ = try s.grow(); + + try s.expectLivePageSerialsValidForTest(); +} + +test "PageList bounded pruning after middle replacement preserves live serials" { + const testing = std.testing; + + var s = try init( + testing.allocator, + 80, + 24, + 3 * PagePool.item_size, + ); + defer s.deinit(); + + while (s.totalPages() < 3) _ = try s.grow(); + const old = s.pages.first.?.next.?; + const old_serial = old.serial; + const replacement = try s.increaseCapacity(old, null); + try testing.expect(replacement != old); + try testing.expect(!s.nodeIsValid(old, old_serial)); + + // Prune the original first page and then the fresh middle replacement. + for (0..2) |_| { + try s.fillLastPageForTest(); + _ = try s.grow(); + try s.expectLivePageSerialsValidForTest(); + } +} + test "PageList incremental compression restarts after earlier replacement" { const testing = std.testing; @@ -9439,6 +9745,53 @@ test "PageList eraseRowBounded invalidates viewport offset cache" { }, s.scrollbar()); } +test "PageList row erasure renews affected page generations" { + const testing = std.testing; + + var s = try init(testing.allocator, 80, 24, null); + defer s.deinit(); + while (s.totalPages() < 2) _ = try s.grow(); + + const first = s.pages.first.?; + const second = first.next.?; + var first_serial = first.serial; + var second_serial = second.serial; + var activity = s.page_compression.activity_serial; + + try s.eraseRow(.{ .history = .{ .y = 0 } }); + try testing.expect(!s.nodeIsValid(first, first_serial)); + try testing.expect(!s.nodeIsValid(second, second_serial)); + try testing.expect(activity != s.page_compression.activity_serial); + + first_serial = first.serial; + second_serial = second.serial; + activity = s.page_compression.activity_serial; + try s.eraseRowBounded( + .{ .history = .{ .y = 0 } }, + first.rows() + 1, + ); + try testing.expect(!s.nodeIsValid(first, first_serial)); + try testing.expect(!s.nodeIsValid(second, second_serial)); + try testing.expect(activity != s.page_compression.activity_serial); +} + +test "PageList trailing row truncation renews page generation" { + const testing = std.testing; + + var s = try init(testing.allocator, 80, 24, null); + defer s.deinit(); + + const node = s.pages.last.?; + const old_serial = node.serial; + const trimmed = s.trimTrailingBlankRows(1); + s.total_rows -= trimmed; + try testing.expectEqual(@as(size.CellCountInt, 1), trimmed); + try testing.expect(!s.nodeIsValid(node, old_serial)); + + _ = try s.grow(); + try s.expectLivePageSerialsValidForTest(); +} + test "PageList eraseRowBounded multi-page invalidates viewport offset cache" { const testing = std.testing; const alloc = testing.allocator; @@ -15841,20 +16194,44 @@ test "PageList reset invalidates stale untracked refs even if node memory is reu var s = try init(alloc, 80, 24, null); defer s.deinit(); - const old_serial = s.pages.first.?.serial; - try testing.expect(old_serial >= s.page_serial_min); - try testing.expect(old_serial < s.page_serial); + var stale_nodes: [page_preheat * 4]*List.Node = undefined; + var stale_serials: [stale_nodes.len]u64 = undefined; + var stale_len: usize = 0; + var reused: ?struct { *List.Node, u64 } = null; - s.reset(); + while (stale_len < stale_nodes.len and reused == null) { + const old_node = s.pages.first.?; + const old_serial = old_node.serial; + try testing.expect(old_serial >= s.page_serial_epoch); + try testing.expect(old_serial < s.page_serial); + stale_nodes[stale_len] = old_node; + stale_serials[stale_len] = old_serial; + stale_len += 1; - // The important safety property is that stale serials are rejected before - // the node pointer is inspected. Reset rebuilds the page list from the - // pools, so old untracked refs may contain node pointers that are no - // longer safe to dereference. - try testing.expect(old_serial < s.page_serial_min); + s.reset(); - const new_serial = s.pages.first.?.serial; - try testing.expect(new_serial >= s.page_serial_min); + const new_node = s.pages.first.?; + for (stale_nodes[0..stale_len], stale_serials[0..stale_len]) |node, serial| { + if (node == new_node) { + reused = .{ node, serial }; + break; + } + } + } + + try testing.expect(reused != null); + const old_node, const old_serial = reused.?; + const new_node = s.pages.first.?; + const new_serial = new_node.serial; + + // Reset advances the epoch before rebuilding from the node pool. Reject + // the stale generation before inspecting its pointer, even when that exact + // address now belongs to a new live generation. + try testing.expectEqual(old_node, new_node); + try testing.expect(old_serial < s.page_serial_epoch); + try testing.expect(!s.nodeIsValid(old_node, old_serial)); + try testing.expect(s.nodeIsValid(new_node, new_serial)); + try testing.expect(new_serial >= s.page_serial_epoch); try testing.expect(new_serial < s.page_serial); } diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 8798411b0..5ca5d957c 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -960,8 +960,8 @@ pub fn cursorScrollAbove(self: *Screen) !void { // lot cheaper in 99% of cases. const old_pin = self.cursor.page_pin.*; - if (try self.pages.grow()) |_| { - try self.cursorScrollAboveRotate(); + if (try self.pages.grow()) |new_node| { + try self.cursorScrollAboveRotate(new_node); } else { // In this case, it means grow() didn't allocate a new page. @@ -981,6 +981,8 @@ pub fn cursorScrollAbove(self: *Screen) !void { // Rotate the rows so that the newly created empty row is at the // beginning. e.g. [ 0 1 2 3 ] in to [ 3 0 1 2 ]. var rows = page.rows.ptr(page.memory.ptr); + // Rotating this suffix changes which logical row its coordinates identify. + self.pages.invalidateNodeLayout(pin.node); fastmem.rotateOnceR(Row, rows[pin.y..page.size.rows]); // Mark the whole page as dirty. @@ -1014,7 +1016,7 @@ pub fn cursorScrollAbove(self: *Screen) !void { // 1 |5E00000000| | 4 // +----------+ : // +-------------+ - try self.cursorScrollAboveRotate(); + try self.cursorScrollAboveRotate(null); } } @@ -1029,7 +1031,10 @@ pub fn cursorScrollAbove(self: *Screen) !void { } } -fn cursorScrollAboveRotate(self: *Screen) !void { +fn cursorScrollAboveRotate( + self: *Screen, + fresh_node: ?*PageList.List.Node, +) !void { self.cursorChangePin(self.cursor.page_pin.down(1).?); // Go through each of the pages following our pin, shift all rows @@ -1042,6 +1047,12 @@ fn cursorScrollAboveRotate(self: *Screen) !void { const prev_rows = prev_page.rows.ptr(prev_page.memory.ptr); const cur_rows = cur_page.rows.ptr(cur_page.memory.ptr); + // A newly allocated tail has no earlier references to invalidate. + if (fresh_node == null or current != fresh_node.?) { + // Rotating this page moves every cached row coordinate down by one. + self.pages.invalidateNodeLayout(current); + } + // Rotate the pages down: [ 0 1 2 3 ] => [ 3 0 1 2 ] fastmem.rotateOnceR(Row, cur_rows[0..cur_page.size.rows]); @@ -1061,6 +1072,8 @@ fn cursorScrollAboveRotate(self: *Screen) !void { assert(current == self.cursor.page_pin.node); const cur_page = current.page(); const cur_rows = cur_page.rows.ptr(cur_page.memory.ptr); + // Rotating the cursor-page suffix changes its cached row coordinates. + self.pages.invalidateNodeLayout(current); fastmem.rotateOnceR(Row, cur_rows[self.cursor.page_pin.y..cur_page.size.rows]); self.clearCells( cur_page, @@ -1145,6 +1158,8 @@ pub fn cursorScrollRegionUp(self: *Screen, limit: usize) !void { // Rotate the region rows so the now-blank top row moves to the // bottom (the cursor row) and everything else shifts up by one. + // Rotating the region changes which logical row its coordinates identify. + self.pages.invalidateNodeLayout(pin.node); fastmem.rotateOnce(Row, rows); // Mark the whole page as dirty. @@ -4919,6 +4934,22 @@ test "Screen: cursorScrollRegionUp simple" { } } +test "Screen: cursorScrollRegionUp renews page generation" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, .{ .cols = 5, .rows = 5, .max_scrollback = 0 }); + defer s.deinit(); + try s.testWriteString("1ABCD\n2EFGH\n3IJKL\n4MNOP\n5QRST"); + s.cursorAbsolute(0, 2); + + const node = s.cursor.page_pin.node; + const serial = node.serial; + try s.cursorScrollRegionUp(2); + + try testing.expect(!s.pages.nodeIsValid(node, serial)); +} + test "Screen: cursorScrollRegionUp moves selection" { const testing = std.testing; const alloc = testing.allocator; @@ -5281,7 +5312,10 @@ test "Screen: scroll above same page" { // +----------+ : // +-------------+ + const node = s.cursor.page_pin.node; + const serial = node.serial; try s.cursorScrollAbove(); + try testing.expect(!s.pages.nodeIsValid(node, serial)); // +----------+ = PAGE 0 // 0 |1ABCD00000| @@ -5356,7 +5390,13 @@ test "Screen: scroll above same page but cursor on previous page" { // +----------+ : // +-------------+ + const first_node = s.pages.pages.first.?; + const second_node = first_node.next.?; + const first_serial = first_node.serial; + const second_serial = second_node.serial; try s.cursorScrollAbove(); + try testing.expect(!s.pages.nodeIsValid(first_node, first_serial)); + try testing.expect(!s.pages.nodeIsValid(second_node, second_serial)); // +----------+ = PAGE 0 // ... : : @@ -5521,7 +5561,10 @@ test "Screen: scroll above creates new page" { // 4307 |3IJKL00000| | 2 // +----------+ : // +-------------+ + const node = s.pages.pages.first.?; + const serial = node.serial; try s.cursorScrollAbove(); + try testing.expect(!s.pages.nodeIsValid(node, serial)); // +----------+ = PAGE 0 // ... : : diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index e5bfd93b7..089201276 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -2429,6 +2429,21 @@ fn rowWillBeShifted( } } +/// Renew every live page generation in an inclusive range before a full-width +/// line operation moves logical rows between their coordinates. +fn invalidateFullWidthRowRange( + self: *Terminal, + first: *PageList.List.Node, + last: *PageList.List.Node, +) void { + var node = first; + while (true) : (node = node.next.?) { + // Full-width line movement remaps cached row coordinates in this page. + self.screens.active.pages.invalidateNodeLayout(node); + if (node == last) break; + } +} + // TODO(qwerasd): `insertLines` and `deleteLines` are 99% identical, // the majority of their logic can (and should) be abstracted in to // a single shared helper function, probably on `Screen` not here. @@ -2504,6 +2519,12 @@ pub fn insertLines(self: *Terminal, count: usize) void { }; defer self.screens.active.pages.untrackPin(cur_p); + // Partial-width margins edit cells in stable rows; full-width moves rows. + if (!left_right) self.invalidateFullWidthRowRange( + self.screens.active.cursor.page_pin.node, + cur_p.node, + ); + // Our current y position relative to the cursor var y: usize = rem; @@ -2699,6 +2720,12 @@ pub fn deleteLines(self: *Terminal, count: usize) void { }; defer self.screens.active.pages.untrackPin(cur_p); + // Partial-width margins edit cells in stable rows; full-width moves rows. + if (!left_right) self.invalidateFullWidthRowRange( + cur_p.node, + cur_p.down(rem - 1).?.node, + ); + // Our current y position relative to the cursor var y: usize = 0; @@ -6867,8 +6894,11 @@ test "Terminal: insertLines simple" { try t.printString("GHI"); t.setCursorPos(2, 2); + const node = t.screens.active.cursor.page_pin.node; + const serial = node.serial; t.clearDirty(); t.insertLines(1); + try testing.expect(!t.screens.active.pages.nodeIsValid(node, serial)); try testing.expect(!t.isDirty(.{ .active = .{ .x = 0, .y = 0 } })); try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 1 } })); @@ -7046,11 +7076,15 @@ test "Terminal: insertLines across page boundary marks all shifted rows dirty" { try t.printString("5EEEE"); // Verify we now have a second page - try testing.expect(first_page.next != null); + const second_page = first_page.next.?; + const first_serial = first_page.serial; + const second_serial = second_page.serial; t.setCursorPos(1, 1); t.clearDirty(); t.insertLines(1); + try testing.expect(!t.screens.active.pages.nodeIsValid(first_page, first_serial)); + try testing.expect(!t.screens.active.pages.nodeIsValid(second_page, second_serial)); try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 0 } })); try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 1 } })); @@ -9754,8 +9788,11 @@ test "Terminal: deleteLines simple" { try t.printString("GHI"); t.setCursorPos(2, 2); + const node = t.screens.active.cursor.page_pin.node; + const serial = node.serial; t.clearDirty(); t.deleteLines(1); + try testing.expect(!t.screens.active.pages.nodeIsValid(node, serial)); try testing.expect(!t.isDirty(.{ .active = .{ .x = 0, .y = 0 } })); try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 1 } })); @@ -9837,11 +9874,15 @@ test "Terminal: deleteLines across page boundary marks all shifted rows dirty" { try t.printString("5EEEE"); // Verify we now have a second page - try testing.expect(first_page.next != null); + const second_page = first_page.next.?; + const first_serial = first_page.serial; + const second_serial = second_page.serial; t.setCursorPos(1, 1); t.clearDirty(); t.deleteLines(1); + try testing.expect(!t.screens.active.pages.nodeIsValid(first_page, first_serial)); + try testing.expect(!t.screens.active.pages.nodeIsValid(second_page, second_serial)); try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 0 } })); try testing.expect(t.isDirty(.{ .active = .{ .x = 0, .y = 1 } })); diff --git a/src/terminal/render.zig b/src/terminal/render.zig index 01ded1d6b..04f0c9679 100644 --- a/src/terminal/render.zig +++ b/src/terminal/render.zig @@ -209,10 +209,16 @@ pub const RenderState = struct { /// change often. arena: ArenaAllocator.State, - /// The page pin. This is not safe to read unless you can guarantee - /// the terminal state hasn't changed since the last `update` call. + /// The page pin. Its copied values may be compared, but its node must + /// not be dereferenced unless the terminal state is protected from + /// changes since the last `update` call. pin: PageList.Pin, + /// The page node generation captured alongside `pin`. This lets + /// consumers validate the pin without dereferencing its node after + /// the terminal lock has been released. + serial: u64, + /// Raw row data. raw: page.Row, @@ -452,6 +458,7 @@ pub const RenderState = struct { row_data.set(y, .{ .arena = .{}, .pin = undefined, + .serial = undefined, .raw = undefined, .cells = .empty, .dirty = true, @@ -477,6 +484,7 @@ pub const RenderState = struct { const row_data = self.row_data.slice(); const row_arenas = row_data.items(.arena); const row_pins = row_data.items(.pin); + const row_serials = row_data.items(.serial); const row_rows = row_data.items(.raw); const row_cells = row_data.items(.cells); const row_sels = row_data.items(.selection); @@ -511,6 +519,7 @@ pub const RenderState = struct { while (y < self.rows) { const chunk = page_it.next() orelse break; const node = chunk.node; + const node_serial = node.serial; const p: *page.Page = node.page(); // The number of rows we consume from this chunk. The chunk @@ -552,25 +561,34 @@ pub const RenderState = struct { const page_rows: []page.Row = p.rows.ptr(p.memory)[chunk.start..][0..take]; assert(p.size.cols == self.cols); - // Store our pins. We have to store these even for rows that - // aren't dirty because dirty is only a renderer optimization; - // it doesn't apply to memory movement. This lets us remap any - // cell pins back to an exact entry in our RenderState. + // Store our pins and their node generations. We have to store + // these even for rows that aren't dirty because dirty is only a + // renderer optimization; it doesn't apply to memory movement. + // This lets us remap any cell pins back to an exact entry in our + // RenderState and validate them later without dereferencing a + // potentially stale node. // - // We can skip the writes when the pins are unchanged: if we're - // not redrawing, every pin was stored by a prior update (row - // count changes force a redraw). Within a single update a node - // appears at most once and its stored pins have consecutive y - // values, so if the first and last pins of this chunk's range - // already match then every pin in between matches too. + // We can skip the writes when the pins and serials are unchanged: + // if we're not redrawing, every value was stored by a prior update + // (row count changes force a redraw). Within a single update a + // node appears at most once and its stored pins have consecutive + // y values, so if the first and last entries of this chunk's range + // already match then every entry in between matches too. if (redraw or row_pins[y].node != node or row_pins[y].y != chunk.start or + row_serials[y] != node_serial or row_pins[y + take - 1].node != node or - row_pins[y + take - 1].y != chunk.start + take - 1) + row_pins[y + take - 1].y != chunk.start + take - 1 or + row_serials[y + take - 1] != node_serial) { - for (row_pins[y..][0..take], chunk.start..) |*pin, py| { + for ( + row_pins[y..][0..take], + row_serials[y..][0..take], + chunk.start.., + ) |*pin, *serial, py| { pin.* = .{ .node = node, .y = @intCast(py) }; + serial.* = node_serial; } } @@ -763,22 +781,28 @@ pub const RenderState = struct { const row_arenas = row_data.items(.arena); const row_dirties = row_data.items(.dirty); const row_pins = row_data.items(.pin); + const row_serials = row_data.items(.serial); const row_highlights_slice = row_data.items(.highlights); for ( row_arenas, row_pins, + row_serials, row_highlights_slice, row_dirties, - ) |*row_arena, row_pin, *row_highlights, *dirty| { + ) |*row_arena, row_pin, row_serial, *row_highlights, *dirty| { for (hls) |hl| { const chunks_slice = hl.chunks.slice(); const nodes = chunks_slice.items(.node); + const serials = chunks_slice.items(.serial); const starts = chunks_slice.items(.start); const ends = chunks_slice.items(.end); for (0.., nodes) |i, node| { - // If this node doesn't match or we're not within - // the row range, skip it. + // If this node generation doesn't match or we're not + // within the row range, skip it. Both serials are copied + // values, so this never dereferences a node outside the + // terminal lock. if (node != row_pin.node or + serials[i] != row_serial or row_pin.y < starts[i] or row_pin.y >= ends[i]) continue; @@ -2040,6 +2064,73 @@ test "linkCells with invalid viewport point" { } } +test "flattened highlights require matching page serial" { + const testing = std.testing; + const alloc = testing.allocator; + + var t = try Terminal.init(alloc, .{ + .cols = 10, + .rows = 3, + }); + defer t.deinit(alloc); + + // Capture the live generation while terminal-owned state is in scope so + // we can also verify beginUpdate copies it into the render row. + const live_pin = t.screens.active.pages.getTopLeft(.viewport); + const live_serial = live_pin.node.serial; + + var state: RenderState = .empty; + defer state.deinit(alloc); + try state.update(alloc, &t); + + const pin: PageList.Pin = pin: { + const row_data = state.row_data.slice(); + @memset(row_data.items(.dirty), false); + state.dirty = .false; + break :pin row_data.items(.pin)[0]; + }; + const row_serial = state.row_data.items(.serial)[0]; + try testing.expectEqual(live_pin.node, pin.node); + try testing.expectEqual(live_serial, row_serial); + + // Use the exact node pointer and row captured by the render state, but a + // different generation. A reused node address must not make this stale + // flattened highlight match. + var hl: highlight.Flattened = .{ + .chunks = .empty, + .top_x = 2, + .bot_x = 4, + }; + defer hl.deinit(alloc); + try hl.chunks.append(alloc, .{ + .node = pin.node, + .serial = live_serial ^ 1, + .start = pin.y, + .end = pin.y + 1, + }); + + try state.updateHighlightsFlattened(alloc, 42, &.{hl}); + { + const row_data = state.row_data.slice(); + try testing.expectEqual(0, row_data.items(.highlights)[0].items.len); + try testing.expect(!row_data.items(.dirty)[0]); + try testing.expectEqual(.false, state.dirty); + } + + // The same chunk is accepted once its copied serial also matches. + hl.chunks.items(.serial)[0] = live_serial; + try state.updateHighlightsFlattened(alloc, 42, &.{hl}); + { + const row_data = state.row_data.slice(); + const row_highlights = row_data.items(.highlights)[0].items; + try testing.expectEqual(1, row_highlights.len); + try testing.expectEqual(42, row_highlights[0].tag); + try testing.expectEqual([2]size.CellCountInt{ 2, 4 }, row_highlights[0].range); + try testing.expect(row_data.items(.dirty)[0]); + try testing.expectEqual(.partial, state.dirty); + } +} + test "dirty row resets highlights" { const testing = std.testing; const alloc = testing.allocator; diff --git a/src/terminal/search/screen.zig b/src/terminal/search/screen.zig index 448bb98c3..e0cd99110 100644 --- a/src/terminal/search/screen.zig +++ b/src/terminal/search/screen.zig @@ -344,6 +344,39 @@ pub const ScreenSearch = struct { } } + fn historyResultIsValid( + self: *const ScreenSearch, + hl: *const FlattenedHighlight, + ) bool { + const chunks = hl.chunks.slice(); + const nodes = chunks.items(.node); + const serials = chunks.items(.serial); + for (nodes, serials) |node, serial| { + if (!self.screen.pages.nodeIsValid(node, serial)) return false; + } + + return true; + } + + /// Clear a selected history match if its flattened page coordinates are + /// stale. reloadActive only inspects the selected history result, so this + /// avoids validating every cached result on each active-area refresh. + fn validateSelectedHistory(self: *ScreenSearch) void { + const m = if (self.selected) |*m| m else return; + const active_len = self.active_results.items.len; + if (m.idx < active_len) return; + + const history_idx = m.idx - active_len; + if (history_idx < self.history_results.items.len and + self.historyResultIsValid(&self.history_results.items[history_idx])) + { + return; + } + + m.deinit(self.screen); + self.selected = null; + } + fn pruneHistory(self: *ScreenSearch) void { // Go through our history results in order (newest to oldest) to find // any result that contains an invalid serial. @@ -352,10 +385,12 @@ pub const ScreenSearch = struct { const hl = &self.history_results.items[i]; const chunks = hl.chunks.slice(); const serials = chunks.items(.serial); - const lowest = serials[0]; - if (lowest < self.screen.pages.page_serial_min) { - // Everything from here forward we assume is invalid because - // our history results only get older. + const first_serial = serials[0]; + if (first_serial < self.screen.pages.page_serial_epoch) { + // Only a whole-list reset advances the epoch. Results are + // newest to oldest, so this result and the entire remaining + // suffix predate that reset. Drop them without scanning the + // live page list for every captured chunk. const alloc = self.allocator(); if (self.selected) |*m| { const first_pruned = self.active_results.items.len + i; @@ -369,13 +404,10 @@ pub const ScreenSearch = struct { return; } - // A page can also be replaced in place, such as by compaction. - // In that case its old serial remains above page_serial_min, so - // validate the captured pointer and serial against the live list. - const nodes = chunks.items(.node); - for (nodes, serials) |node, serial| { - if (!self.screen.pages.nodeIsValid(node, serial)) break; - } else { + // Ordinary pruning, layout changes, and replacements do not + // advance the epoch. Validate their pointer-plus-generation pairs + // against the live list. + if (self.historyResultIsValid(hl)) { i += 1; continue; } @@ -457,6 +489,10 @@ pub const ScreenSearch = struct { // searchers: column reflow may have freed every node they reference. if (try self.resetIfDimensionsChanged()) return; + // reloadActive only inspects the selected history result, so validate + // that result without scanning every cached match on this hot path. + self.validateSelectedHistory(); + const tw = reloadActive_tw; // If our selection pin became garbage it means we scrolled off @@ -766,9 +802,9 @@ pub const ScreenSearch = struct { // selected result so no pre-resize pointer is ever dereferenced. _ = try self.resetIfDimensionsChanged(); - // All selection requires valid pins so we prune history and - // reload our active area immediately. This ensures all search - // results point to valid nodes. + // Reload validates the selected history result before inspecting its + // cached coordinates. Prune the remaining results afterward so every + // candidate is valid before selection tracks it. try self.reloadActive(); self.pruneHistory(); @@ -1616,6 +1652,144 @@ test "select after history compaction ignores replaced results" { try testing.expect(search.selected == null); } +test "select after partial history page erase ignores shifted results" { + 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 stream = t.vtStream(); + defer stream.deinit(); + + const first = list.pages.first.?; + while (first.rows() < first.capacity().rows) stream.nextSlice("\r\n"); + stream.nextSlice("error"); + for (0..list.rows + 1) |_| stream.nextSlice("\r\n"); + try testing.expect(first != list.pages.last.?); + + var search: ScreenSearch = try .init(alloc, t.screens.active, "error"); + defer search.deinit(); + try search.searchAll(); + try testing.expectEqual(0, search.active_results.items.len); + try testing.expectEqual(1, search.history_results.items.len); + + const old_rows = first.rows(); + list.eraseHistory(.{ .history = .{ .y = 0 } }); + try testing.expectEqual(old_rows - 1, first.rows()); + + try testing.expect(!try search.select(.next)); + try testing.expect(search.selected == null); +} + +test "reload defers pruning unselected history results" { + 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 stream = t.vtStream(); + defer stream.deinit(); + + const first = list.pages.first.?; + while (first.rows() < first.capacity().rows) stream.nextSlice("\r\n"); + stream.nextSlice("error"); + for (0..list.rows + 1) |_| stream.nextSlice("\r\n"); + + var search: ScreenSearch = try .init(alloc, t.screens.active, "error"); + defer search.deinit(); + try search.searchAll(); + try testing.expectEqual(1, search.history_results.items.len); + + list.eraseHistory(.{ .history = .{ .y = 0 } }); + + // Routine active refreshes don't inspect unselected history results. + try search.reloadActive(); + try testing.expectEqual(1, search.history_results.items.len); + + // Selection prunes every candidate before attempting to track it. + try testing.expect(!try search.select(.next)); + try testing.expectEqual(0, search.history_results.items.len); +} + +test "reload after partial history page erase drops shifted selection first" { + 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 stream = t.vtStream(); + defer stream.deinit(); + + const first = list.pages.first.?; + while (first.rows() < first.capacity().rows) stream.nextSlice("\r\n"); + stream.nextSlice("error"); + for (0..list.rows + 1) |_| stream.nextSlice("\r\n"); + + var search: ScreenSearch = try .init(alloc, t.screens.active, "error"); + defer search.deinit(); + try search.searchAll(); + try testing.expect(try search.select(.next)); + try testing.expect(search.selected != null); + + list.eraseHistory(.{ .history = .{ .y = 0 } }); + + // Advance the active boundary into another node and add matches so + // reloadActive updates the history result offsets. It must discard the + // stale selected result before comparing any of its captured coordinates. + const active_node = list.getTopLeft(.active).node; + while (list.getTopLeft(.active).node == active_node) { + stream.nextSlice("error\r\n"); + } + + try search.reloadActive(); + try testing.expect(search.selected == null); +} + +test "select after history page split ignores moved results" { + 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 stream = t.vtStream(); + defer stream.deinit(); + + const first = list.pages.first.?; + while (first.rows() < first.capacity().rows) stream.nextSlice("\r\n"); + stream.nextSlice("error"); + for (0..list.rows + 1) |_| stream.nextSlice("\r\n"); + + var search: ScreenSearch = try .init(alloc, t.screens.active, "error"); + defer search.deinit(); + try search.searchAll(); + try testing.expectEqual(1, search.history_results.items.len); + + try list.split(.{ + .node = first, + .y = first.rows() / 2, + .x = 0, + }); + + try testing.expect(!try search.select(.next)); + try testing.expect(search.selected == null); +} + test "screen search no scrollback has no history" { const alloc = testing.allocator; var t: Terminal = try .init(alloc, .{ diff --git a/src/terminal/search/sliding_window.zig b/src/terminal/search/sliding_window.zig index 5c29ce03b..2f858ec85 100644 --- a/src/terminal/search/sliding_window.zig +++ b/src/terminal/search/sliding_window.zig @@ -489,12 +489,14 @@ pub const SlidingWindow = struct { .reverse => { const slice = self.chunk_buf.slice(); const nodes = slice.items(.node); + const serials = slice.items(.serial); const starts = slice.items(.start); const ends = slice.items(.end); if (self.chunk_buf.len > 1) { // Reverse all our chunks. This should be pretty obvious why. std.mem.reverse(*PageList.List.Node, nodes); + std.mem.reverse(u64, serials); std.mem.reverse(size.CellCountInt, starts); std.mem.reverse(size.CellCountInt, ends); @@ -1447,6 +1449,15 @@ test "SlidingWindow two pages match across boundary reversed" { // Search should find a match { const h = w.next().?; + const chunks = h.chunks.slice(); + const nodes = chunks.items(.node); + const serials = chunks.items(.serial); + try testing.expectEqual(2, chunks.len); + try testing.expectEqual(node, nodes[0]); + try testing.expectEqual(node.serial, serials[0]); + try testing.expectEqual(node.next.?, nodes[1]); + try testing.expectEqual(node.next.?.serial, serials[1]); + const sel = h.untracked(); try testing.expectEqual(point.Point{ .active = .{ .x = 76, diff --git a/src/terminal/search/viewport.zig b/src/terminal/search/viewport.zig index d8dbb9429..0493c0d6f 100644 --- a/src/terminal/search/viewport.zig +++ b/src/terminal/search/viewport.zig @@ -103,9 +103,9 @@ pub const ViewportSearch = struct { // If our viewport contains the start or end of the active area, // we are in the active area. We purposely do this first // because our viewport is always larger than the active area. - for (old.nodes) |node| { - if (node == active_tl.node) break :match; - if (node == active_br.node) break :match; + for (old.entries) |entry| { + if (entry.node == active_tl.node) break :match; + if (entry.node == active_br.node) break :match; } } @@ -135,7 +135,7 @@ pub const ViewportSearch = struct { // exists) so we can cover the overlap. We only do this for the // soft-wrapped prior pages. const overlap_len = self.window.needle.len -| 1; - var node_ = fingerprint.nodes[0].prev; + var node_ = fingerprint.entries[0].node.prev; var added: usize = 0; while (node_) |node| : (node_ = node.prev) { // We could be more accurate here and count bytes since the @@ -149,13 +149,13 @@ pub const ViewportSearch = struct { // We can use our fingerprint nodes to initialize our sliding // window, since we already traversed the viewport once. var end_append: SlidingWindow.AppendResult = undefined; - for (fingerprint.nodes) |node| { - end_append = try self.window.append(node); + for (fingerprint.entries) |entry| { + end_append = try self.window.append(entry.node); } // Add any trailing overlap as well. trailing: { - const end: *PageList.List.Node = fingerprint.nodes[fingerprint.nodes.len - 1]; + const end: *PageList.List.Node = fingerprint.entries[fingerprint.entries.len - 1].node; if (!end_append.last_row_wrapped) break :trailing; node_ = end.next; @@ -181,14 +181,18 @@ pub const ViewportSearch = struct { /// Viewport fingerprint so we can detect when the viewport moves. const Fingerprint = struct { - /// The nodes that make up the viewport. We need to flatten this - /// to a single list because we can't safely traverse the cached values - /// because the page nodes may be invalid. All that is safe is comparing - /// the actual pointer values. - nodes: []const *PageList.List.Node, + /// The node addresses and generations that make up the viewport. We + /// can't safely dereference cached node pointers because the nodes may + /// be invalid, so equality only compares these captured values. + entries: []const Entry, + + const Entry = struct { + node: *PageList.List.Node, + serial: u64, + }; pub fn init(alloc: Allocator, pages: *PageList) Allocator.Error!Fingerprint { - var list: std.ArrayList(*PageList.List.Node) = .empty; + var list: std.ArrayList(Entry) = .empty; defer list.deinit(alloc); // Get our viewport area. Bottom right of a viewport can never @@ -197,18 +201,22 @@ pub const ViewportSearch = struct { const br = pages.getBottomRight(.viewport).?; var it = tl.pageIterator(.right_down, br); - while (it.next()) |chunk| try list.append(alloc, chunk.node); - return .{ .nodes = try list.toOwnedSlice(alloc) }; + while (it.next()) |chunk| try list.append(alloc, .{ + .node = chunk.node, + .serial = chunk.node.serial, + }); + return .{ .entries = try list.toOwnedSlice(alloc) }; } pub fn deinit(self: *Fingerprint, alloc: Allocator) void { - alloc.free(self.nodes); + alloc.free(self.entries); } pub fn eql(self: Fingerprint, other: Fingerprint) bool { - if (self.nodes.len != other.nodes.len) return false; - for (self.nodes, other.nodes) |a, b| { - if (a != b) return false; + if (self.entries.len != other.entries.len) return false; + for (self.entries, other.entries) |a, b| { + if (a.node != b.node) return false; + if (a.serial != b.serial) return false; } return true; } @@ -258,6 +266,33 @@ test "simple search" { try testing.expect(search.next() == null); } +test "fingerprint detects node generation changes" { + const alloc = testing.allocator; + var t: Terminal = try .init(alloc, .{ .cols = 10, .rows = 10 }); + defer t.deinit(alloc); + + var search: ViewportSearch = try .init(alloc, "needle"); + defer search.deinit(); + search.active_dirty = false; + + try testing.expect(try search.update(&t.screens.active.pages)); + try testing.expect(!try search.update(&t.screens.active.pages)); + + const old_entry = search.fingerprint.?.entries[0]; + const node = t.screens.active.pages.pages.first.?; + try testing.expectEqual(node, old_entry.node); + const original_serial = node.serial; + defer node.serial = original_serial; + node.serial += 1; + + // The viewport still contains the same pointer, but its generation has + // changed, so the cached window must be rebuilt. + try testing.expect(try search.update(&t.screens.active.pages)); + const new_entry = search.fingerprint.?.entries[0]; + try testing.expectEqual(old_entry.node, new_entry.node); + try testing.expect(old_entry.serial != new_entry.serial); +} + test "clear screen and search" { const alloc = testing.allocator; var t: Terminal = try .init(alloc, .{ .cols = 10, .rows = 10 });