Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guard against identical ID's in plugin tree view #12338

Merged
merged 1 commit into from
Apr 4, 2023
Merged

Guard against identical ID's in plugin tree view #12338

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 28 additions & 15 deletions packages/plugin-ext/src/plugin/tree/tree-views.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
// TODO: extract `@theia/util` for event, disposable, cancellation and common types
// don't use @theia/core directly from plugin host
import { Emitter } from '@theia/core/lib/common/event';
import { basename } from '@theia/core/lib/common/paths';
import { Disposable, DisposableCollection } from '@theia/core/lib/common/disposable';
import { DataTransfer, DataTransferItem, Disposable as PluginDisposable, ThemeIcon } from '../types-impl';
import { Plugin, PLUGIN_RPC_CONTEXT, TreeViewsExt, TreeViewsMain, TreeViewItem, TreeViewRevealOptions, DataTransferFileDTO } from '../../common/plugin-api-rpc';
Expand Down Expand Up @@ -194,8 +195,6 @@ class TreeViewExtImpl<T> implements Disposable {
private static readonly ID_COMPUTED = 'c';
private static readonly ID_ITEM = 'i';

private nextItemId: 0;

private readonly onDidExpandElementEmitter = new Emitter<TreeViewExpansionEvent<T>>();
readonly onDidExpandElement = this.onDidExpandElementEmitter.event;

Expand Down Expand Up @@ -330,7 +329,7 @@ class TreeViewExtImpl<T> implements Disposable {
// parent is inconsistent
return undefined;
}
const candidateId = this.buildTreeItemId(parentId, treeItem);
const candidateId = this.buildTreeItemId(parentId, treeItem, false);
if (this.nodes.has(candidateId)) {
return chain.concat(candidateId);
}
Expand Down Expand Up @@ -361,18 +360,32 @@ class TreeViewExtImpl<T> implements Disposable {
return idLabel;
}

private buildTreeItemId(parentId: string, item: TreeItem): string {
// build tree id according to https://code.visualstudio.com/api/references/vscode-api#TreeItem
// note: the front end tree implementation cannot handle reparenting items, hence the id is set to the "path" of individual ids
let id = typeof item.id === 'string' ? item.id : this.getItemLabel(item);
if (id) {
// we use '' as the id of the root, we don't consider that a valid id
// since '' is falsy, we'll never get '' in this branch
id = TreeViewExtImpl.ID_ITEM + id;
} else {
id = TreeViewExtImpl.ID_COMPUTED + this.nextItemId++;
// Modeled on https://github.com/microsoft/vscode/blob/main/src/vs/workbench/api/common/extHostTreeViews.ts#L822
private buildTreeItemId(parentId: string, item: TreeItem, mustReturnNew: boolean): string {
tsmaeder marked this conversation as resolved.
Show resolved Hide resolved
if (item.id) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@colin-grant-work colin-grant-work Mar 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤦

return `${TreeViewExtImpl.ID_ITEM}-@-${parentId}-@-${item.id}`;
}
return `${parentId}/${id}`;

const treeItemLabel = this.getItemLabel(item);
const prefix: string = `${TreeViewExtImpl.ID_COMPUTED}-@-${parentId || ''}-@-`;
let elementId = treeItemLabel ? treeItemLabel : item.resourceUri ? basename(item.resourceUri.fsPath) : '';
elementId = elementId.indexOf('/') !== -1 ? elementId.replace('/', '//') : elementId;
const childrenNodes = (this.nodes.get(parentId)?.children || []);

let id: string;
let counter = 0;
tsmaeder marked this conversation as resolved.
Show resolved Hide resolved
do {
id = `${prefix}/${counter}:${elementId}`;
if (!mustReturnNew || !this.nodes.has(id) || this.nodes.get(id) === item) {
// Return first if asked for or
// Return if handle does not exist or
// Return if handle is being reused
break;
}
counter++;
} while (counter <= childrenNodes.length);

return id;
}

async getChildren(parentId: string): Promise<TreeViewItem[] | undefined> {
Expand Down Expand Up @@ -404,7 +417,7 @@ class TreeViewExtImpl<T> implements Disposable {

// Generate the ID
// ID is used for caching the element
const id = this.buildTreeItemId(parentId, treeItem);
const id = this.buildTreeItemId(parentId, treeItem, true);

const toDisposeElement = new DisposableCollection();
const node: TreeExtNode<T> = {
Expand Down