Skip to content

Guard against undefined values in text mode#1587

Open
blixt wants to merge 1 commit into
copy:masterfrom
blixt:fix/text-mode
Open

Guard against undefined values in text mode#1587
blixt wants to merge 1 commit into
copy:masterfrom
blixt:fix/text-mode

Conversation

@blixt

@blixt blixt commented Jun 11, 2026

Copy link
Copy Markdown

I found that the whole render path could crash due to an undefined value in text mode colors. This is a quick fix for that. (Using more TypeScript would help a lot for this sort of bug!)

@chschnell

Copy link
Copy Markdown
Contributor

The value of cursor_rgba cannot be undefined, its value comes directly from Int32Array text_mode_data a couple of lines above.

fg_rgba and bg_rgba can only be undefined if either text_mode_width (number of text columns) or font_width (font width in pixel) is 0, which I have difficulties to understand. The only hint I could find is a comment in your PR:

(fg_rgba is undefined when the row had no cells, e.g. mid resize)

Do you mean a resize to 0 text columns?

Is there a way for me to reproduce this scenario?

@chschnell

Copy link
Copy Markdown
Contributor

Maybe I'm overlooking something, but from what I can find the number of text screen rows or columns should never be 0, see here, which makes sense and would explain why we never guarded the code against this scenario.

@blixt

blixt commented Jun 11, 2026

Copy link
Copy Markdown
Author

It was very rare for me, so I'm not sure I will have the ability to reproduce this again. I just had a definitive crash, and this is ostensibly the fix. I found the crash in my clipboard history, see below.

It's possible I was too defensive checking all the variables for undefined if this is the only time it could be undefined (though with proper TypeScript typing we wouldn't need to follow the lineage of the variables manually):


The crash:

  Uncaught TypeError: Cannot read properties of undefined (reading 'toString')
      at c (libv86.mjs:12:219)
      at xa.update_graphical_text (libv86.mjs:20:371)
      at xa.update_screen (libv86.mjs:18:196)
      at libv86.mjs:18:83

I could also extract this from libv86.mjs as the surrounding code within update_graphical_text and that c function:

Line 12 character ~219

function va(a,b){function c(w){w=w.toString(16);return"#"+"0".repeat(6-w.length)+w}function d(w){var t=256*ja,// ...

And I think the call site is here (note in this case that void 0 is the undefined check that was added):

Line 20 character ~371

… void 0!==K&&(H.fillStyle=c(K) …

@chschnell

Copy link
Copy Markdown
Contributor

Thank you for your quick responses (also in your other PR earlier).

As a suggestion, here's a simpler approach to fix this at an earlier point in the code (outside the for-loop). In function ScreenAdapter.render_changed_rows(), at src/browser/screen.js:210, just insert:

if(gfx_width === 0)
{
    return 0; // rows have 0px width: nothing visible to render
}

This is the only modification needed, and I think it makes sense to guard against this corner-case (even though its circumstances are not fully understood).

By the way, your edit ("The crash:...") makes a lot of sense, as this would first crash in function number_as_color(n) where toString() is called, which matches your Exception.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants