fix(renderer): clip terminal contents to expected grid size (#4523) (#5265)

Resolves #4523

More notably this fixes a memory corruption crash that can occur while
resizing the font under Metal while there's a lot of active changes
occurring (e.g. while running DOOM fire). The change where all
background colors are explicitly written exposed this issue, though it
was technically a problem the whole time I'm fairly sure, just that the
corruption it caused before was benign.

This significantly improves the robustness of the renderers since it
prevents synchronization issues from causing memory corruption due to
out of bounds read/writes while building the cells.

TODO: when viewport is narrower than renderer grid size, fill blank
margin with bg color- currently appears as black, this only affects
DECCOLM right now, and possibly could create single-frame artefacts
during poorly managed resizes, but it's not ideal regardless.
This commit is contained in:
Mitchell Hashimoto
2025-01-20 19:27:09 -08:00
committed by GitHub
2 changed files with 39 additions and 36 deletions

View File

@ -1046,19 +1046,6 @@ pub fn updateFrame(
} }
} }
// If our terminal screen size doesn't match our expected renderer
// size then we skip a frame. This can happen if the terminal state
// is resized between when the renderer mailbox is drained and when
// the state mutex is acquired inside this function.
//
// For some reason this doesn't seem to cause any significant issues
// with flickering while resizing. '\_('-')_/'
if (self.cells.size.rows != state.terminal.rows or
self.cells.size.columns != state.terminal.cols)
{
return;
}
// Get the viewport pin so that we can compare it to the current. // Get the viewport pin so that we can compare it to the current.
const viewport_pin = state.terminal.screen.pages.pin(.{ .viewport = .{} }).?; const viewport_pin = state.terminal.screen.pages.pin(.{ .viewport = .{} }).?;
@ -2437,12 +2424,22 @@ fn rebuildCells(
} }
} }
// Go row-by-row to build the cells. We go row by row because we do // We rebuild the cells row-by-row because we
// font shaping by row. In the future, we will also do dirty tracking // do font shaping and dirty tracking by row.
// by row.
var row_it = screen.pages.rowIterator(.left_up, .{ .viewport = .{} }, null); var row_it = screen.pages.rowIterator(.left_up, .{ .viewport = .{} }, null);
var y: terminal.size.CellCountInt = screen.pages.rows; // If our cell contents buffer is shorter than the screen viewport,
// we render the rows that fit, starting from the bottom. If instead
// the viewport is shorter than the cell contents buffer, we align
// the top of the viewport with the top of the contents buffer.
var y: terminal.size.CellCountInt = @min(
screen.pages.rows,
self.cells.size.rows,
);
while (row_it.next()) |row| { while (row_it.next()) |row| {
// The viewport may have more rows than our cell contents,
// so we need to break from the loop early if we hit y = 0.
if (y == 0) break;
y -= 1; y -= 1;
if (!rebuild) { if (!rebuild) {
@ -2501,7 +2498,11 @@ fn rebuildCells(
var shaper_cells: ?[]const font.shape.Cell = null; var shaper_cells: ?[]const font.shape.Cell = null;
var shaper_cells_i: usize = 0; var shaper_cells_i: usize = 0;
const row_cells = row.cells(.all); const row_cells_all = row.cells(.all);
// If our viewport is wider than our cell contents buffer,
// we still only process cells up to the width of the buffer.
const row_cells = row_cells_all[0..@min(row_cells_all.len, self.cells.size.columns)];
for (row_cells, 0..) |*cell, x| { for (row_cells, 0..) |*cell, x| {
// If this cell falls within our preedit range then we // If this cell falls within our preedit range then we

View File

@ -706,8 +706,6 @@ pub fn updateFrame(
// Update all our data as tightly as possible within the mutex. // Update all our data as tightly as possible within the mutex.
var critical: Critical = critical: { var critical: Critical = critical: {
const grid_size = self.size.grid();
state.mutex.lock(); state.mutex.lock();
defer state.mutex.unlock(); defer state.mutex.unlock();
@ -748,19 +746,6 @@ pub fn updateFrame(
} }
} }
// If our terminal screen size doesn't match our expected renderer
// size then we skip a frame. This can happen if the terminal state
// is resized between when the renderer mailbox is drained and when
// the state mutex is acquired inside this function.
//
// For some reason this doesn't seem to cause any significant issues
// with flickering while resizing. '\_('-')_/'
if (grid_size.rows != state.terminal.rows or
grid_size.columns != state.terminal.cols)
{
return;
}
// Get the viewport pin so that we can compare it to the current. // Get the viewport pin so that we can compare it to the current.
const viewport_pin = state.terminal.screen.pages.pin(.{ .viewport = .{} }).?; const viewport_pin = state.terminal.screen.pages.pin(.{ .viewport = .{} }).?;
@ -1276,10 +1261,23 @@ pub fn rebuildCells(
} }
} }
// Build each cell const grid_size = self.size.grid();
// We rebuild the cells row-by-row because we do font shaping by row.
var row_it = screen.pages.rowIterator(.left_up, .{ .viewport = .{} }, null); var row_it = screen.pages.rowIterator(.left_up, .{ .viewport = .{} }, null);
var y: terminal.size.CellCountInt = screen.pages.rows; // If our cell contents buffer is shorter than the screen viewport,
// we render the rows that fit, starting from the bottom. If instead
// the viewport is shorter than the cell contents buffer, we align
// the top of the viewport with the top of the contents buffer.
var y: terminal.size.CellCountInt = @min(
screen.pages.rows,
grid_size.rows,
);
while (row_it.next()) |row| { while (row_it.next()) |row| {
// The viewport may have more rows than our cell contents,
// so we need to break from the loop early if we hit y = 0.
if (y == 0) break;
y -= 1; y -= 1;
// True if we want to do font shaping around the cursor. We want to // True if we want to do font shaping around the cursor. We want to
@ -1356,7 +1354,11 @@ pub fn rebuildCells(
var shaper_cells: ?[]const font.shape.Cell = null; var shaper_cells: ?[]const font.shape.Cell = null;
var shaper_cells_i: usize = 0; var shaper_cells_i: usize = 0;
const row_cells = row.cells(.all); const row_cells_all = row.cells(.all);
// If our viewport is wider than our cell contents buffer,
// we still only process cells up to the width of the buffer.
const row_cells = row_cells_all[0..@min(row_cells_all.len, grid_size.columns)];
for (row_cells, 0..) |*cell, x| { for (row_cells, 0..) |*cell, x| {
// If this cell falls within our preedit range then we // If this cell falls within our preedit range then we