Uncaught TypeError: Cannot read property 'size' of undefined

[vscodeerrors]

Issue Id: 6297e0aa-6693-c1dc-1a92-9880326445ab

Versions
- 1.20.0
- 5190885
- f2b2109
Stack
TypeError: Cannot read property 'size' of undefined
/vs/base/browser/ui/list/listView.ts#L228:27 (size)
/vs/base/browser/ui/list/listWidget.ts#L967:34 (elementHeight)
/vs/base/browser/ui/selectBox/selectBoxCustom.ts#L413:19 (reveal)
/vs/base/browser/ui/selectBox/selectBoxCustom.ts#L377:7 (layoutSelectDropDown)
/vs/base/browser/ui/selectBox/selectBoxCustom.ts#L353:53 (renderSelectDropDown)
/vs/base/browser/ui/contextview/contextview.ts#L156:35 (render)
/vs/platform/contextview/browser/contextViewService.ts#L36:19 (show)
/vs/base/browser/ui/selectBox/selectBoxCustom.ts#L351:27 (showContextView)
/vs/base/browser/ui/selectBox/selectBoxCustom.ts#L160:9 (showSelectDropDown)

[octref]

Actually seems to be caused by 9a5b6eb so giving to @bpasero

[bpasero]

@cleidigh fyi this came in as error telemetry

[cleidigh]

@bpasero
this looks like getElementHeight might be called on an uninitialized select. the telemetry Stack trace doesn't give me a lot to go on and I cannot really reproduce since I cannot see telemetry.

is there any way to get more information on this?

[bpasero]

@cleidigh no, not more than the stack is what we have

[isidorn]

We might fix it with #44368
Though let's wait and see if @joaomoreno want to fix it on that level.

[joaomoreno]

#44368 doesn't fix it, it just silently returns 0, which is very often not the right thing to do.

db616b3 throws a more specific error message. List users should not pass in anything other than valid indexes to the list.

[joaomoreno]

The bug is in here. What happens if getFocus() returns []?

[cleidigh]

@joaomoreno
@bpasero
@isidorn
@Tyriar

As I mentioned in the other issue #45078 (same case and some were description) the root cause it's not the Focus, it's the fact that we somehow end up with a misconfigured list. I don't want to fix this with just a null check since it would still leave us with an error and an empty select box.

I can now reproduce this thanks to @IlyaBiryukov .
Somehow when terminal.dispose it's called from the extension , the select list gets destroyed .

I'm looking at the sequence now to see how the other terminals get destroyed incorrectly.
It looks like it's associated with changing the options (eg deleting the terminal) and not knowing what should become the current selection.

[cleidigh]

@Tyriar
I've done some tracing on this and determined that when the terminal created with an extension is disposed the current selected index passed to selectbox.setOptions can be out of range by one
IF a user has created a terminal and then subsequently removes the terminal created by the extension
with terminal.dispose

The terminal index remains correct if the terminal created by the extension has focus when disposed.

Some of the behavior is in _removeTab but I don't think this is where it should be fixed. I don't want to make any assumptions on this so I wanted your insight/determination on where to be fixed.

Simplest reproduction:

• manually create one terminal
• create terminal through the extension interface
• manually create a second terminal
• now there are three total terminals, terminal three - manually created has focus in the list and tab
• have the extension do terminal.dispose on the extension created terminal
• Drop-down header will become blank and an error thrown
• with the tracing I can see the call to setOptions has the correct options and length but the selected option is off by one (2) when it should be (1).

[cleidigh]

@bpasero
@Tyriar
I'm overdue to fix this for the February recovery but, I have a couple issues:

• there are at least two problems:

1. improved boundary checking needed in new and old select box code
2. there seem to be at least one probably two scenarios where the terminal makes a
   setOptions call with the selected option out of bounds

I did a fair amount of trace and debugging (see above) and I am confident
with the fact that when a terminal is deleted from an extension this happens.
I can add boundary checking, question is what to do with an out of bound option

I wanted to add a new notification but I cannot call from the widget due to layering
I cannot return an error without changing all clients to deal with the error and perhaps notify the user.

it's not clear to me yet if #45300 @Tyriar references, but I bet he is correct and this is a related issue.

a force fix bounds checking selected and setting to zero can take care of the issue of the extension removing the terminal. This ends up making the active terminal somewhat arbitrary and it is not the root cause of the problem.

I think I need some guidance on your best practices for error handling (telemetry, console and/or notifications) understanding the way you guys like to do this is key for future submissions.

I also need to know how you want to handle the clients, @Tyriar I don't think I know enough to fundamentally change your terminal instance handling.

[Tyriar]

@cleidigh I'm a bit swamped at the moment but I'll try get to this for March

[cleidigh]

@Tyriar
no problem. I'll continue to try and investigate, hopefully it won't be a problem if I have a couple of simple questions. I think if I use balance checking on the select box side and force a terminal selection upon an extension 's disposal we will end up with a situation where the terminal has one perspective on selection in the select box has a different one so I don't think that really works. The end requirement is both sides need the correct perspective

do you agree?

regardless I'll keep pecking away at it /-;)

[cleidigh]

Fixed With #47445