diff --git a/vendor/libghostty-vt.patches.md b/vendor/libghostty-vt.patches.md index 351b9560f..6bb973a1b 100644 --- a/vendor/libghostty-vt.patches.md +++ b/vendor/libghostty-vt.patches.md @@ -228,3 +228,41 @@ just test-one kitty_file_image_survives (cd vendor/libghostty-vt && zig build test-lib-vt -Dtest-filter='experimental PNG') just check ``` + +## 0008 keep cursor hyperlink across page replacement + +status: active + +patch: `vendor/patches/libghostty-vt/0008-cursor-hyperlink-page-replace.patch` + +herdr issue: https://github.com/herdrdev/herdr/issues/5296 + +upstream discussion: not opened + +upstream pr: not opened + +vendored base: `44f2a44df7e8c4a0c6df3f7d872ef3d7ead88e51` + +local files: + +- `vendor/libghostty-vt/src/terminal/Screen.zig` + +reason: Moving the cursor onto another page while a hyperlink and a +non-default style are active can grow that page. Growth replaces the page, +re-adds the same heap hyperlink, and frees it. The outer move then clears +the pointer without clearing the id that replacement just installed, so +`endHyperlink` dereferences null and the server dies. The same sequence is +still present on upstream main (`cursorChangePin` around line 1606 and +`cursorPageReplaced` around line 706 at 22fa134a0). This is not the one-row +scrollback prune fixed by 65ff54c67. + +remove when: the vendored source keeps the cursor hyperlink alive across a +page replacement during a pin change, and the Screen test below passes +without this patch. + +verification: + +```sh +(cd vendor/libghostty-vt && zig build test-lib-vt -Dtest-filter='cursorAbsolute across a full page keeps the hyperlink') +just maintenance-test +``` diff --git a/vendor/libghostty-vt/src/terminal/Screen.zig b/vendor/libghostty-vt/src/terminal/Screen.zig index d0d2b929f..b395c28ca 100644 --- a/vendor/libghostty-vt/src/terminal/Screen.zig +++ b/vendor/libghostty-vt/src/terminal/Screen.zig @@ -1530,6 +1530,10 @@ inline fn cursorChangePin(self: *Screen, new: Pin) void { // Update our pin to the new page self.cursor.page_pin.* = new; + // Remember the heap link before style migration. Growing the + // destination page replaces it and frees this object. + const pre_style_link = self.cursor.hyperlink; + // On the new page, we need to migrate our style if (old_style_) |old_style| { self.cursor.style = old_style; @@ -1546,10 +1550,14 @@ inline fn cursorChangePin(self: *Screen, new: Pin) void { // On the new page, we need to migrate our hyperlink if (self.cursor.hyperlink) |link| { - // startHyperlink will try to free old hyperlinks, so set this - // to null. We free it ourselves later since we're doing some - // ref-counting shenanigans in this function. + // Page replacement during style migration already inserted this + // link on the new page and freed the pre-move object. + if (link != pre_style_link) return; + + // Clear both fields so startHyperlink doesn't try to end a link + // through a null pointer. We free the old link ourselves below. self.cursor.hyperlink = null; + self.cursor.hyperlink_id = 0; // Re-add self.startHyperlink(link.uri, switch (link.id) { @@ -5021,6 +5029,46 @@ test "Screen: cursorAbsolute to page with insufficient capacity" { new_page.assertIntegrity(); } +test "Screen: cursorAbsolute across a full page keeps the hyperlink" { + const testing = std.testing; + const alloc = testing.allocator; + const io = testing.io; + + var s = try init(io, alloc, .{ .cols = 10, .rows = 3, .max_scrollback_bytes = 1 }); + defer s.deinit(); + + const start_page = s.pages.pages.last.?.page(); + const rem = start_page.capacity.rows; + start_page.pauseIntegrityChecks(true); + for (0..rem) |_| try s.cursorDownOrScroll(); + start_page.pauseIntegrityChecks(false); + try testing.expect(start_page != s.cursor.page_pin.node.page()); + + { + start_page.pauseIntegrityChecks(true); + defer start_page.pauseIntegrityChecks(false); + var n: u24 = 1; + while (start_page.styles.add( + start_page.memory, + .{ .bg_color = .{ .rgb = @bitCast(n) } }, + )) |_| n += 1 else |_| {} + } + + try s.setAttribute(.{ .bold = {} }); + try s.startHyperlink("https://example.com/5296", "keep"); + + // The destination style set is full, so this pin change grows the page + // while the cursor still holds the pre-move hyperlink. + s.cursorAbsolute(1, 1); + + try testing.expect(s.cursor.hyperlink_id != 0); + try testing.expectEqualStrings("https://example.com/5296", s.cursor.hyperlink.?.uri); + try testing.expectEqualStrings("keep", s.cursor.hyperlink.?.id.explicit); + s.endHyperlink(); + try testing.expect(s.cursor.hyperlink == null); + try testing.expect(s.cursor.hyperlink_id == 0); +} + test "Screen: scrolling" { const testing = std.testing; const alloc = testing.allocator; diff --git a/vendor/patches/libghostty-vt/0008-cursor-hyperlink-page-replace.patch b/vendor/patches/libghostty-vt/0008-cursor-hyperlink-page-replace.patch new file mode 100644 index 000000000..aa13f47a7 --- /dev/null +++ b/vendor/patches/libghostty-vt/0008-cursor-hyperlink-page-replace.patch @@ -0,0 +1,80 @@ +diff --git a/vendor/libghostty-vt/src/terminal/Screen.zig b/vendor/libghostty-vt/src/terminal/Screen.zig +index d0d2b929..b395c28c 100644 +--- a/vendor/libghostty-vt/src/terminal/Screen.zig ++++ b/vendor/libghostty-vt/src/terminal/Screen.zig +@@ -1530,6 +1530,10 @@ inline fn cursorChangePin(self: *Screen, new: Pin) void { + // Update our pin to the new page + self.cursor.page_pin.* = new; + ++ // Remember the heap link before style migration. Growing the ++ // destination page replaces it and frees this object. ++ const pre_style_link = self.cursor.hyperlink; ++ + // On the new page, we need to migrate our style + if (old_style_) |old_style| { + self.cursor.style = old_style; +@@ -1546,10 +1550,14 @@ inline fn cursorChangePin(self: *Screen, new: Pin) void { + + // On the new page, we need to migrate our hyperlink + if (self.cursor.hyperlink) |link| { +- // startHyperlink will try to free old hyperlinks, so set this +- // to null. We free it ourselves later since we're doing some +- // ref-counting shenanigans in this function. ++ // Page replacement during style migration already inserted this ++ // link on the new page and freed the pre-move object. ++ if (link != pre_style_link) return; ++ ++ // Clear both fields so startHyperlink doesn't try to end a link ++ // through a null pointer. We free the old link ourselves below. + self.cursor.hyperlink = null; ++ self.cursor.hyperlink_id = 0; + + // Re-add + self.startHyperlink(link.uri, switch (link.id) { +@@ -5021,6 +5029,46 @@ test "Screen: cursorAbsolute to page with insufficient capacity" { + new_page.assertIntegrity(); + } + ++test "Screen: cursorAbsolute across a full page keeps the hyperlink" { ++ const testing = std.testing; ++ const alloc = testing.allocator; ++ const io = testing.io; ++ ++ var s = try init(io, alloc, .{ .cols = 10, .rows = 3, .max_scrollback_bytes = 1 }); ++ defer s.deinit(); ++ ++ const start_page = s.pages.pages.last.?.page(); ++ const rem = start_page.capacity.rows; ++ start_page.pauseIntegrityChecks(true); ++ for (0..rem) |_| try s.cursorDownOrScroll(); ++ start_page.pauseIntegrityChecks(false); ++ try testing.expect(start_page != s.cursor.page_pin.node.page()); ++ ++ { ++ start_page.pauseIntegrityChecks(true); ++ defer start_page.pauseIntegrityChecks(false); ++ var n: u24 = 1; ++ while (start_page.styles.add( ++ start_page.memory, ++ .{ .bg_color = .{ .rgb = @bitCast(n) } }, ++ )) |_| n += 1 else |_| {} ++ } ++ ++ try s.setAttribute(.{ .bold = {} }); ++ try s.startHyperlink("https://example.com/5296", "keep"); ++ ++ // The destination style set is full, so this pin change grows the page ++ // while the cursor still holds the pre-move hyperlink. ++ s.cursorAbsolute(1, 1); ++ ++ try testing.expect(s.cursor.hyperlink_id != 0); ++ try testing.expectEqualStrings("https://example.com/5296", s.cursor.hyperlink.?.uri); ++ try testing.expectEqualStrings("keep", s.cursor.hyperlink.?.id.explicit); ++ s.endHyperlink(); ++ try testing.expect(s.cursor.hyperlink == null); ++ try testing.expect(s.cursor.hyperlink_id == 0); ++} ++ + test "Screen: scrolling" { + const testing = std.testing; + const alloc = testing.allocator;