Xterm.js: Browser crash related to fit addon returning geometry with "Infinity" sizes

[vincentwoo]

Appears to be caused by edge cases around offscreen xterms coming/going onscreen. With term.renderer.dimensions.actualCellWidth as null, fit returns Infinity. If you allow xterm to try to resize itself to Infinity, it consumes all available RAM and crashes the browser.

[vincentwoo]

Here's a CodePen that mostly reproduces: https://codepen.io/akanet/pen/zjBgXa

[vincentwoo]

@Tyriar sorry to nag, any chance you got the chance to look at this one?

[ioquatix]

I think I'm running into the same issue.

[ioquatix]

I added the following at the end of that function

    console.log("fit proposed geometry", geometry);
    if (geometry.cols == Infinity || geometry.rows == Infinity) {
      return null;
    }

but now fit doesn't work at all. I think it's broken.

[ioquatix]

I just stuffed this in at the top:

var cellWidth = term.renderer.dimensions.actualCellWidth || 10;
var cellHeight = term.renderer.dimensions.actualCellHeight || 15;

and the appropriate adjustment to the bottom:

    var geometry = {
        cols: Math.floor(availableWidth / cellWidth),
        rows: Math.floor(availableHeight / cellHeight)
    };

Things starting to work a bit better, but obviously the fit is wrong.

[ioquatix]

If you put this at the top of fit:

    if (!cellWidth || !cellHeight)
      return null;

and don't have defaults, it never renders correctly, and thus the renderer actualCell* fields never get a value.

[vincentwoo]

I am mitigating currently by defaulting to some sane (but wrong) value in the case of Infinity, and re-running fit on a timeout. It's not ideal.

[ioquatix]

Yeah, but I don't understand why it's necessary to default to the wrong value.... I'm just trying to figure it out.

[ioquatix]

Somehow, if I call terminal.resize(NaN, NaN), eventually it starts working. But if I call anything else, e.g. terminal.resize(10, 10), it never works.

[ioquatix]

        //this.terminal.fit();
        this.terminal.resize(NaN, NaN);

just seems so wrong.

[ioquatix]

It literally doesn't work at all if I don't have that. The only other time fit is called is when the parent element changes size.

[ioquatix]

Somehow, the renderer never computes values unless you do that.

[ioquatix]

This is what I ended up with, that works. Gah.

Terminal.prototype.proposeGeometry = function() {
        if (!this.element.parentElement) {
                return null;
        }

        var cellWidth = this.renderer.dimensions.actualCellWidth || 9;
        var cellHeight = this.renderer.dimensions.actualCellHeight || 17;
        var parentElementStyle = window.getComputedStyle(this.element.parentElement);
        var parentElementHeight = parseInt(parentElementStyle.getPropertyValue('height'));
        var parentElementWidth = Math.max(0, parseInt(parentElementStyle.getPropertyValue('width')));
        var elementStyle = window.getComputedStyle(this.element);
        var elementPadding = {
                top: parseInt(elementStyle.getPropertyValue('padding-top')),
                bottom: parseInt(elementStyle.getPropertyValue('padding-bottom')),
                right: parseInt(elementStyle.getPropertyValue('padding-right')),
                left: parseInt(elementStyle.getPropertyValue('padding-left'))
        };
        var elementPaddingVer = elementPadding.top + elementPadding.bottom;
        var elementPaddingHor = elementPadding.right + elementPadding.left;
        var availableHeight = parentElementHeight - elementPaddingVer;
        var availableWidth = parentElementWidth - elementPaddingHor - this.viewport.scrollBarWidth;
        var geometry = {
                cols: Math.floor(availableWidth / cellWidth),
                rows: Math.floor(availableHeight / cellHeight)
        };
                // This is still sometimes NaN, NaN !?
        return geometry;
}

Terminal.prototype.fit = function() {
    var geometry = this.proposeGeometry();

    if (geometry) {
        if (this.rows !== geometry.rows || this.cols !== geometry.cols) {
            this.renderer.clear();
            this.resize(geometry.cols, geometry.rows);
        }
    }
}

[vincentwoo]

I just noticed the requirement that xterm has that the parent be visible when open is called. We might be in the wrong, here, so I'll try to take a second look later.

[ioquatix]

I see. I'm not even sure how to guarantee this with etch and atom. I don't know if there is some kind of hook, like onParentVisible?

[ioquatix]

I figured out a solution based on what you said.

I moved terminal.open into the resize handler. It worked. So, the component gets created on a virtual dom, and then once it gets added to the real parent, the resize is invoked, and it sets up the actual terminal.

[Tyriar]

I just noticed the requirement that xterm has that the parent be visible when open is called. We might be in the wrong, here, so I'll try to take a second look later.

Yeah this, you should be able to work around it by checking if the item is attached before calling fit:

if (term.element) {
  term.fit();
}

So, the component gets created on a virtual dom

In this case you'll need some other way to handle this, maybe by keeping track of whether it's truly attached (why is it being attached to a virtual dom?)

---

To close off this issue maybe we should add a null check on some dimensions property inside fit so it doesn't attempt to size to Infinity?

[ioquatix]

Even if term.element exists, the parent element might not, so it still fails.

why is it being attached to a virtual dom?

Because it's being used inside Etch/atom.

https://atom.io/packages/script-runner

I don't know if there is a better way to do it.

[ww-k]

@Tyriar Just like, xterm publish a great lib, and the lib has an dangerous bug cause my browser crash, and xterm group just tell developer pay attention this.
Of course, I can add a checking. But I think most developer would not attention this bug, include xterm group. And we could easily fix this bug by add some protective code to prevent it (eg: set a default max rows and max cols). why we don't do it?

[ioquatix]

You should definitely check that it's only going to allocate a finite number of rows and columns... It doesn't need to be a maximum... just sanity check the arguments are not Infinity.

[ww-k]

@ioquatix yeah, may be we don't need maximum, add max rows just an solution of my proposal. I mean xterm should add protective code. Not just tell thousands of developers to add check code.
The root cause of this bug is this lines had't protective code.
https://github.com/xtermjs/xterm.js/blob/master/src/Terminal.ts#L1941
https://github.com/xtermjs/xterm.js/blob/master/src/Buffer.ts#L150

[ioquatix]

I think it's reasonable to have proactive checks. It shouldn't be possible for integers to be NaN, but here we are, it's JavaScript. So, it makes sense to ensure the numbers, aren't, say, negative, NaN, Infinity, etc.

[ww-k]

The purpose of add check code is to enhance the robustness of the program. I obviously xterm not be robust enough. Now if only normal errors, the browser will report errors, we can easily locate and solve. But this error crash the browser, it is very hard to debug.

[Tyriar]

@kevinwon1985 I said let's close this issue off with a PR that prevents invalid values:

To close off this issue maybe we should add a null check on some dimensions property inside fit so it doesn't attempt to size to Infinity?

I don't want to add a maximum value though as it complicates the API for little reason, I've never run into issues with setting the size to such a large number.

[ww-k]

@Tyriar No maximum value is ok. I think just add some check inside fit is not enough. Other developers, may be make the same mistake when call resize, just like fit add-on do. So should we add some check inside resize ?

[ioquatix]

1/ I think if the size is invalid, it should be an error.

2/ If the size is too big for the available computer memory, it should be an error too, not crash the browser.