From 3b889438aa40951755805f10665d96aeaeca3b55 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 22 Jul 2024 17:01:25 -0700 Subject: [PATCH] terminal: page clone handles case where hyperlink can't dupe Fixes #1991 To check if a hyperlink from another page is already present in our page's set, we need to dupe the hyperlink struct. If the hyperlink is already present in our page, this dupe is a waste and is freed. In the case where the hyperlink is present AND we don't have enough memory to dupe the hyperlink to check if its present, we'd previous simply crash out and fail rendering. Debug builds would crash with integrity errors. This commit resolves the issue by falling back to a slow path when our string allocation table is full and iterating over the hyperlink map to check one by one if we have the hyperlink. This O(N) is much slower than allocating (in this case) but N is usually low on top of this case being rare. A better solution would probably be to ensure we always have some % of available space free in our string allocation table. This would result in some wasteful page reallocs but would speed up the render loop. We can look into that later. --- src/terminal/hyperlink.zig | 23 ++++++++---- src/terminal/page.zig | 73 +++++++++++++++++++++++++++++++++----- 2 files changed, 81 insertions(+), 15 deletions(-) diff --git a/src/terminal/hyperlink.zig b/src/terminal/hyperlink.zig index f692f1f8d..53d776ad5 100644 --- a/src/terminal/hyperlink.zig +++ b/src/terminal/hyperlink.zig @@ -36,7 +36,11 @@ pub const Hyperlink = struct { }; /// Duplicate this hyperlink from one page to another. - pub fn dupe(self: *const Hyperlink, self_page: *const Page, dst_page: *Page) !Hyperlink { + pub fn dupe( + self: *const Hyperlink, + self_page: *const Page, + dst_page: *Page, + ) error{OutOfMemory}!Hyperlink { var copy = self.*; // If the pages are the same then we can return a shallow copy. @@ -100,13 +104,18 @@ pub const Hyperlink = struct { return hasher.final(); } - pub fn eql(self: *const Hyperlink, base: anytype, other: *const Hyperlink) bool { + pub fn eql( + self: *const Hyperlink, + self_base: anytype, + other: *const Hyperlink, + other_base: anytype, + ) bool { if (std.meta.activeTag(self.id) != std.meta.activeTag(other.id)) return false; switch (self.id) { .implicit => if (self.id.implicit != other.id.implicit) return false, .explicit => { - const self_ptr = self.id.explicit.offset.ptr(base); - const other_ptr = other.id.explicit.offset.ptr(base); + const self_ptr = self.id.explicit.offset.ptr(self_base); + const other_ptr = other.id.explicit.offset.ptr(other_base); if (!std.mem.eql( u8, self_ptr[0..self.id.explicit.len], @@ -117,8 +126,8 @@ pub const Hyperlink = struct { return std.mem.eql( u8, - self.uri.offset.ptr(base)[0..self.uri.len], - other.uri.offset.ptr(base)[0..other.uri.len], + self.uri.offset.ptr(self_base)[0..self.uri.len], + other.uri.offset.ptr(other_base)[0..other.uri.len], ); } }; @@ -137,7 +146,7 @@ pub const Set = RefCountedSet( } pub fn eql(self: *const @This(), a: Hyperlink, b: Hyperlink) bool { - return a.eql(self.page.?.memory, &b); + return a.eql(self.page.?.memory, &b, self.page.?.memory); } pub fn deleted(self: *const @This(), link: Hyperlink) void { diff --git a/src/terminal/page.zig b/src/terminal/page.zig index bc7d3b740..ec8bbc370 100644 --- a/src/terminal/page.zig +++ b/src/terminal/page.zig @@ -312,7 +312,7 @@ pub const Page = struct { /// A helper that can be used to assert the integrity of the page /// when runtime safety is enabled. This is a no-op when runtime /// safety is disabled. This uses the libc allocator. - pub fn assertIntegrity(self: *Page) void { + pub fn assertIntegrity(self: *const Page) void { if (comptime std.debug.runtime_safety) { self.verifyIntegrity(std.heap.c_allocator) catch unreachable; } @@ -324,7 +324,7 @@ pub const Page = struct { /// is freed before this function returns. /// /// Integrity errors are also logged as warnings. - pub fn verifyIntegrity(self: *Page, alloc_gpa: Allocator) !void { + pub fn verifyIntegrity(self: *const Page, alloc_gpa: Allocator) !void { // Some things that seem like we should check but do not: // // - We do not check that the style ref count is exact, only that @@ -657,6 +657,17 @@ pub const Page = struct { // If we have no managed memory in the source, then we can just // copy it directly. if (!src_row.managedMemory()) { + // This is an integrity check: if the row claims it doesn't + // have managed memory then all cells must also not have + // managed memory. + if (comptime std.debug.runtime_safety) { + for (other_cells) |cell| { + assert(!cell.hasGrapheme()); + assert(!cell.hyperlink); + assert(cell.style_id == style.default_id); + } + } + fastmem.copy(Cell, cells, other_cells); } else { // We have managed memory, so we have to do a slower copy to @@ -664,8 +675,11 @@ pub const Page = struct { for (cells, other_cells) |*dst_cell, *src_cell| { dst_cell.* = src_cell.*; if (src_cell.hasGrapheme()) { - // To prevent integrity checks flipping - if (comptime std.debug.runtime_safety) dst_cell.style_id = style.default_id; + // To prevent integrity checks flipping. This will + // get fixed up when we check the style id below. + if (comptime std.debug.runtime_safety) { + dst_cell.style_id = style.default_id; + } dst_cell.content_tag = .codepoint; // required for appendGrapheme const cps = other.lookupGrapheme(src_cell).?; @@ -683,16 +697,59 @@ pub const Page = struct { // Slow-path: get the hyperlink from the other page, // add it, and migrate. const id = other.lookupHyperlink(src_cell).?; - const other_link = other.hyperlink_set.get(other.memory, id); + + const dst_link = dst_link: { + // Fast path is we just dupe the hyperlink because + // it doesn't require traversal through the hyperlink + // map. If the add below already contains it then it'll + // call the deleted context callback and we'll free + // this back. + const other_link = other.hyperlink_set.get(other.memory, id); + if (other_link.dupe(other, self)) |dst_link| { + break :dst_link dst_link; + } else |err| switch (err) { + // If this happens, the only possible valid outcome is + // that it is because this link is already in our set + // and our memory is full because we already have it. + // Any other outcome is an integrity violation. + error.OutOfMemory => {}, + } + + // Slow, the only way to really find our link is to + // traverse over the map, which includes dupes... + const dst_map = self.hyperlink_map.map(self.memory); + var it = dst_map.valueIterator(); + while (it.next()) |existing_id| { + const existing_link = self.hyperlink_set.get( + self.memory, + existing_id.*, + ); + + if (existing_link.eql( + self.memory, + other_link, + other.memory, + )) { + break :dst_link existing_link.*; + } + } + + // There is no other valid scenario where we don't + // have the memory to dupe a hyperlink since we allocate + // cloned pages with enough capacity to contain their + // contents. + unreachable; + }; + const dst_id = try self.hyperlink_set.addWithIdContext( self.memory, - try other_link.dupe(other, self), + dst_link, id, .{ .page = self }, ) orelse id; try self.setHyperlink(dst_row, dst_cell, dst_id); } - if (src_cell.style_id != style.default_id) { + if (src_cell.style_id != style.default_id) style: { dst_row.styled = true; if (other == self) { @@ -700,7 +757,7 @@ pub const Page = struct { // copying the style, we can use the style ID directly. dst_cell.style_id = src_cell.style_id; self.styles.use(self.memory, dst_cell.style_id); - continue; + break :style; } // Slow path: Get the style from the other