Skip to content

Commit b4df742

Browse files
committed
fix(buffer): use counted grapheme tracking to prevent WrongGeneration panics
The panic came from a model mismatch: GraphemeTracker tracked only ID presence, while interned IDs can appear in multiple cells. During set(), remove->add could briefly drop an ID to refcount 0, allowing slot reuse and generation bump.
1 parent 5e57b93 commit b4df742

File tree

3 files changed

+54
-17
lines changed

3 files changed

+54
-17
lines changed

packages/core/src/zig/buffer.zig

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -440,6 +440,7 @@ pub const OptimizedBuffer = struct {
440440
const prev_char = self.buffer.char[index];
441441
const prev_attr = self.buffer.attributes[index];
442442
const prev_link_id = ansi.TextAttributes.getLinkId(prev_attr);
443+
var tracker_replaced = false;
443444

444445
// If overwriting a grapheme span (start or continuation) with a different char, clear that span first
445446
if ((gp.isGraphemeChar(prev_char) or gp.isContinuationChar(prev_char)) and prev_char != cell.char) {
@@ -449,15 +450,14 @@ pub const OptimizedBuffer = struct {
449450
const right = gp.charRightExtent(prev_char);
450451
const id = gp.graphemeIdFromChar(prev_char);
451452

452-
// Skip tracker remove when the new cell reuses the same grapheme ID
453-
// (different extent bits but same pool slot). Removing would decref
454-
// to 0, freeing the slot; a concurrent alloc could then reuse it and
455-
// bump the generation, making the later tracker.add() hit WrongGeneration.
456-
const new_is_grapheme = gp.isGraphemeChar(cell.char);
457-
const new_id = if (new_is_grapheme) gp.graphemeIdFromChar(cell.char) else 0;
458-
if (!(new_is_grapheme and new_id == id)) {
459-
self.grapheme_tracker.remove(id);
460-
}
453+
const new_grapheme_id: ?u32 = blk: {
454+
if (!gp.isGraphemeChar(cell.char)) break :blk null;
455+
const new_width = gp.charRightExtent(cell.char) + 1;
456+
if (x + new_width > self.width) break :blk null;
457+
break :blk gp.graphemeIdFromChar(cell.char);
458+
};
459+
self.grapheme_tracker.replace(id, new_grapheme_id);
460+
tracker_replaced = true;
461461

462462
const span_start = index - @min(left, index - row_start);
463463
const span_end = index + @min(right, row_end - index);
@@ -512,7 +512,10 @@ pub const OptimizedBuffer = struct {
512512
self.buffer.attributes[index] = cell.attributes;
513513

514514
const id: u32 = gp.graphemeIdFromChar(cell.char);
515-
self.grapheme_tracker.add(id);
515+
const is_same_grapheme_start = gp.isGraphemeChar(prev_char) and prev_char == cell.char;
516+
if (!tracker_replaced and !is_same_grapheme_start) {
517+
self.grapheme_tracker.add(id);
518+
}
516519

517520
const new_link_id = ansi.TextAttributes.getLinkId(cell.attributes);
518521
if (prev_link_id != 0 and prev_link_id != new_link_id) {
@@ -598,7 +601,7 @@ pub const OptimizedBuffer = struct {
598601
/// Calculate the real byte size of the character buffer including grapheme pool data
599602
pub fn getRealCharSize(self: *const OptimizedBuffer) u32 {
600603
const total_chars = self.width * self.height;
601-
const grapheme_count = self.grapheme_tracker.getGraphemeCount();
604+
const grapheme_count = self.grapheme_tracker.getGraphemeCellCount();
602605
const total_grapheme_bytes = self.grapheme_tracker.getTotalGraphemeBytes();
603606

604607
const regular_char_bytes = (total_chars - grapheme_count) * @sizeOf(u32);

packages/core/src/zig/grapheme.zig

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -499,18 +499,20 @@ pub fn deinitGlobalPool() void {
499499

500500
pub const GraphemeTracker = struct {
501501
pool: *GraphemePool,
502-
used_ids: std.AutoHashMap(u32, void),
502+
used_ids: std.AutoHashMap(u32, u32), // id -> number of cells in this buffer
503503

504504
pub fn init(allocator: std.mem.Allocator, pool: *GraphemePool) GraphemeTracker {
505505
return .{
506506
.pool = pool,
507-
.used_ids = std.AutoHashMap(u32, void).init(allocator),
507+
.used_ids = std.AutoHashMap(u32, u32).init(allocator),
508508
};
509509
}
510510

511511
fn decRefAll(self: *GraphemeTracker) void {
512512
var it = self.used_ids.keyIterator();
513513
while (it.next()) |idp| {
514+
// Pool refs are tracked per ID (first/last cell transition), so clear
515+
// decrefs once per tracked ID, not once per per-buffer cell count.
514516
self.pool.decref(idp.*) catch {};
515517
}
516518
}
@@ -530,18 +532,34 @@ pub const GraphemeTracker = struct {
530532
std.debug.panic("GraphemeTracker.add failed: {}\n", .{err});
531533
};
532534
if (!res.found_existing) {
535+
res.value_ptr.* = 1;
533536
self.pool.incref(id) catch |err| {
534537
std.debug.panic("GraphemeTracker.add incref failed: {}\n", .{err});
535538
};
539+
} else {
540+
res.value_ptr.* += 1;
536541
}
537542
}
538543

539544
pub fn remove(self: *GraphemeTracker, id: u32) void {
545+
const count_ptr = self.used_ids.getPtr(id) orelse return;
546+
if (count_ptr.* > 1) {
547+
count_ptr.* -= 1;
548+
return;
549+
}
550+
540551
if (self.used_ids.remove(id)) {
541552
self.pool.decref(id) catch {};
542553
}
543554
}
544555

556+
pub fn replace(self: *GraphemeTracker, old_id: ?u32, new_id: ?u32) void {
557+
if (old_id != null and new_id != null and old_id.? == new_id.?) return;
558+
559+
if (new_id) |id| self.add(id);
560+
if (old_id) |id| self.remove(id);
561+
}
562+
545563
pub fn contains(self: *const GraphemeTracker, id: u32) bool {
546564
return self.used_ids.contains(id);
547565
}
@@ -554,13 +572,23 @@ pub const GraphemeTracker = struct {
554572
return @intCast(self.used_ids.count());
555573
}
556574

575+
pub fn getGraphemeCellCount(self: *const GraphemeTracker) u32 {
576+
var total: u32 = 0;
577+
var it = self.used_ids.valueIterator();
578+
while (it.next()) |count_ptr| {
579+
total += count_ptr.*;
580+
}
581+
return total;
582+
}
583+
557584
pub fn getTotalGraphemeBytes(self: *const GraphemeTracker) u32 {
558585
var total_bytes: u32 = 0;
559-
var it = self.used_ids.keyIterator();
560-
while (it.next()) |idp| {
561-
const id = idp.*;
586+
var it = self.used_ids.iterator();
587+
while (it.next()) |entry| {
588+
const id = entry.key_ptr.*;
589+
const count = entry.value_ptr.*;
562590
if (self.pool.get(id)) |bytes| {
563-
total_bytes += @intCast(bytes.len);
591+
total_bytes += @as(u32, @intCast(bytes.len)) * count;
564592
} else |_| {
565593
// If we can't get the bytes, this shouldn't happen but handle gracefully
566594
continue;

packages/core/src/zig/tests/grapheme_test.zig

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -647,6 +647,12 @@ test "GraphemeTracker - add same grapheme twice increfs once" {
647647
tracker.add(id); // Should not incref again
648648

649649
try std.testing.expectEqual(@as(u32, 1), tracker.getGraphemeCount());
650+
try std.testing.expectEqual(@as(u32, 2), tracker.getGraphemeCellCount());
651+
try std.testing.expectEqual(@as(u32, 2), tracker.getTotalGraphemeBytes());
652+
653+
tracker.remove(id);
654+
try std.testing.expect(tracker.contains(id));
655+
try std.testing.expectEqual(@as(u32, 1), tracker.getGraphemeCellCount());
650656

651657
// After deinit (via defer), tracker decrefs once, bringing refcount to 0
652658
}

0 commit comments

Comments
 (0)