search

xtermjs / xterm.js

A terminal for the web
https://xtermjs.org/
MIT License
17.53k stars 1.62k forks source link

A crashing character joiner brings down the whole terminal #3547

Closed Eugeny closed 2 years ago

Eugeny commented 2 years ago

In particular, opentype.js cannot parse the updated Monaco font file in macOS Monterey (No valid cmap sub-tables found.), rendering xterm unusable when the ligature addon is active.

Tyriar commented 2 years ago

Easy repro is probably to throw in the registerCharacterJoiner callback

jerch commented 2 years ago

On a sidenote - the particular issue might be fixed in opentype.js v0.9.0 (see https://github.com/opentypejs/opentype.js/issues/348), while font-ligatures still uses v0.8.0.

Eugeny commented 2 years ago

It's not unfortunately - I've tried forcibly upgrading ot.js to their latest release

jerch commented 2 years ago

Ah ok. Well even if it was fixed, the character joiner should not tear down the whole terminal in the first place.

LabhanshAgrawal commented 2 years ago

What if we create an anonymous function to try catch the handler (which returns empty array on catch) and use that instead of the actual handler at

https://github.com/xtermjs/xterm.js/blob/20141518d995b85a0a39e3e0352cf0a9db3e676c/src/browser/services/CharacterJoinerService.ts#L71-L74

Does this seem ok?

Tyriar commented 2 years ago

I think we just need to catch and handle exceptions here:

https://github.com/xtermjs/xterm.js/blob/20141518d995b85a0a39e3e0352cf0a9db3e676c/src/browser/services/CharacterJoinerService.ts#L179-L182

LabhanshAgrawal commented 2 years ago

Yeah, that's the only place it's called at right now, so we can update there. I was suggesting wrapping it, so that we don't have to worry about it later, in case it's called from other places. But that might not be necessary.

Should I go ahead and open a pr for this?

Tyriar commented 2 years ago

@LabhanshAgrawal I think that's the only place and probably won't change. That would be great!