Issue #821 - Prevent TypeError in resize() #823
Conversation
|
Thanks for this PR @jpmasters! Can you please revert the changes in all files in |
1 similar comment
|
Again, thanks for this PR @jpmasters! I was able to remove the checked out |
|
Awesome, thanks @parisk, I wasn't sure whether the /dist files needed to be updated or not. Does this mean that gulp needs to be run after xterm.js is added to a project via a bower install or is this done automatically? |
|
@jpmasters the This will go away thankfully though on 3.0: #406. |
| @@ -1941,6 +1941,9 @@ Terminal.prototype.resize = function(x, y) { | |||
| ch = [this.defAttr, ' ', 1]; // does xterm use the default attr? | |||
| i = this.buffer.lines.length; | |||
| while (i--) { | |||
| if (this.buffer.lines.get(i) === undefined) { | |||
There was a problem hiding this comment.
The resize rows section below is meant to handle this row resizing, if this is actually happening we should figure out the root cause. this.buffer.lines.get(i) should always return an array when i < this.buffer.lines.length which is guaranteed by line 1942.
There was a problem hiding this comment.
Underneath the CircularList is just a straight Javascript array so I'm assuming that somehow 'undefined' got pushed into the array. Could that not give the same symptoms? I'm wondering if perhaps there was some kind of problem parsing the data from the server that resulted in an undefined object getting into the circular list.
I'll try an experiment by looking for a push of an undefined into CircularList and see if that yields any clues.
There was a problem hiding this comment.
Looking at what is happening in the debugger, we're making a lot of use of the RI escape sequence to implement a 'scrolling' area of the terminal screen. This seems to result in calls to shiftElements that in turn calls set() in the circular buffer code which is bringing undefined data from the unused 1000 circular buffer array entries into the part being used by xterm.
I think the root cause might be line 2282 in xterm.js (Terminal.prototype.reverseIndex).
this.buffer.lines.shiftElements(this.buffer.y + this.buffer.ybase, this.rows - 1, 1);
When I look at the prototype for shiftElements the parameters are start, count and offset so instead of setting count to this.rows - 1 should it not also subtract (this.buffer.y + this.buffer.ybase)? Otherwise when this.set is called in shiftElements (line 174 in CirlculstList.ts), it could be bringing in undefined data from the unused array area in the circular list buffer.
Edit: Using this patch I was able to fix the problem but this was only with a ybase of 0. Not sure what happens when ybase !== 0.
There was a problem hiding this comment.
Nice investigation 😄 could you come up with a script or something that can reproduce the problem and put into #824? The best way I've found to do these is to open devtools console in the demo and type term.write('\x1b[blah...') containing the escape sequences that cause the issue.
Updated Terminal resize() to fill undefined rows before resizing them.
Closes #821.