Guard against identical ID's in plugin tree view

[colin-grant-work]

What it does

Fixes #12337 mostly by bringing in VSCode's approach to the same problem.

How to test

1. Follow procedure in New PluginTree ID generation algorithm generates identical ID's #12337
2. All references should be rendered separately, and application should not crash.

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[tsmaeder]

@colin-grant-work I'm a bit confuse, because the API documentation of TreeItem clearly states:

Optional id for the tree item that has to be unique across tree. The id is used to preserve the selection and expansion state of the tree item.

Is the "references view" built in extension not compliant with the published API?

[tsmaeder]

Ah, I see: the trouble is that we calculate a "pseudo-id" based on the label which can be the same for multiple items.

[tsmaeder]

We still need the logic to construct paths from ID's for all the reasons in https://github.com/eclipse-theia/theia/pull/12120/files. While the incremental tree update is running, a node might be in more than one place at the same time. The drag/drop example helps from the PR helps test this.

[colin-grant-work]

This logic does use the parent ID as part of the ID if it's present:

const prefix: string = parentId ? parentId : TreeViewExtImpl.ID_COMPUTED;

[tsmaeder]

It also needs to be done for items that have an id set. Otherwise reparenting items does not work.

[colin-grant-work]

This code does seem to work with the example extension from your drag and drop PR. Do you have a concrete example where this would fail?

[tsmaeder]

I was able to reproduce the problem from #12111 eventually. The simple scenario from the bug does not reproduce the problem reliably, but randomly dragging elements around for a while eventually triggered the stack overflow. The problem seems to be timing-dependent.

[colin-grant-work]

I've pushed a change that includes the parent's ID for plugin-supplied ID's as well as computed ID's.

[tsmaeder]

I'm thinking this might be a problem because we're using the same separator for the path separator and the prefix separator. Let's think about an element that has the id /i/parent/child. The tree item id will be i/i/parent/child. Wouldn't that be the same id we get when we compute the id of an element with id child, whose parent is an element with id parent? Because the computed id of the parent is i/parent, so the expression becomes ${'i'}/${'i/parent'}/'child' = i/i/parent/child.

Come to think of it, we might have to escape the path separator in given id's in order to prevent this "attack". Sorry to make seemingly trivial change so complicated 🤦

[tsmaeder]

The current version fixes the problem with D&D reparenting.

[tsmaeder]

Fine to merge from my side: I still think we could construct an example where unescaped weird id values could wreak havoc, but that situation is no worse than before, so not an obstacle to merging the PR.