From 70f6fa2bc91e27500900d8633d7f1279feb26e24 Mon Sep 17 00:00:00 2001
From: Daniel Imms <2193314+Tyriar@users.noreply.github.com>
Date: Sun, 13 Aug 2023 10:42:45 -0700
Subject: [PATCH] Fix SortedList.values iteration and general dec lifecycle
 fixes

Fixes #4652
---
 addons/xterm-addon-search/src/SearchAddon.ts       | 12 +++---------
 .../decorations/BufferDecorationRenderer.ts        |  4 ++++
 src/common/SortedList.test.ts                      | 14 ++++++++++++++
 src/common/SortedList.ts                           |  3 ++-
 src/common/buffer/Marker.ts                        |  4 ++--
 src/common/services/DecorationService.ts           | 14 +-------------
 6 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/addons/xterm-addon-search/src/SearchAddon.ts b/addons/xterm-addon-search/src/SearchAddon.ts
index 8568cba8..cbde1a1b 100644
--- a/addons/xterm-addon-search/src/SearchAddon.ts
+++ b/addons/xterm-addon-search/src/SearchAddon.ts
@@ -68,8 +68,6 @@ export class SearchAddon extends Disposable implements ITerminalAddon {
   private _highlightDecorations: IHighlight[] = [];
   private _selectedDecoration: IHighlight | undefined;
   private _highlightLimit: number;
-  private _onDataDisposable: IDisposable | undefined;
-  private _onResizeDisposable: IDisposable | undefined;
   private _lastSearchOptions: ISearchOptions | undefined;
   private _highlightTimeout: number | undefined;
   /**
@@ -93,13 +91,9 @@ export class SearchAddon extends Disposable implements ITerminalAddon {
 
   public activate(terminal: Terminal): void {
     this._terminal = terminal;
-    this._onDataDisposable = this.register(this._terminal.onWriteParsed(() => this._updateMatches()));
-    this._onResizeDisposable = this.register(this._terminal.onResize(() => this._updateMatches()));
-    this.register(toDisposable(() => {
-      this.clearDecorations();
-      this._onDataDisposable?.dispose();
-      this._onResizeDisposable?.dispose();
-    }));
+    this.register(this._terminal.onWriteParsed(() => this._updateMatches()));
+    this.register(this._terminal.onResize(() => this._updateMatches()));
+    this.register(toDisposable(() => this.clearDecorations()));
   }
 
   private _updateMatches(): void {
diff --git a/src/browser/decorations/BufferDecorationRenderer.ts b/src/browser/decorations/BufferDecorationRenderer.ts
index fb77ce9c..ba4f6ca8 100644
--- a/src/browser/decorations/BufferDecorationRenderer.ts
+++ b/src/browser/decorations/BufferDecorationRenderer.ts
@@ -103,6 +103,10 @@ export class BufferDecorationRenderer extends Disposable {
         decoration.element = element;
         this._decorationElements.set(decoration, element);
         this._container.appendChild(element);
+        decoration.onDispose(() => {
+          this._decorationElements.delete(decoration);
+          element!.remove();
+        });
       }
       element.style.top = `${line * this._renderService.dimensions.css.cell.height}px`;
       element.style.display = this._altBufferIsActive ? 'none' : 'block';
diff --git a/src/common/SortedList.test.ts b/src/common/SortedList.test.ts
index ecafdb8f..d2e01ba8 100644
--- a/src/common/SortedList.test.ts
+++ b/src/common/SortedList.test.ts
@@ -104,4 +104,18 @@ describe('SortedList', () => {
       { key: 10 }
     ]);
   });
+  describe('values', () => {
+    it('should iterate correctly when list items change during iteration', () => {
+      list.insert(1);
+      list.insert(2);
+      list.insert(3);
+      list.insert(4);
+      const visited: number[] = [];
+      for (const item of list.values()) {
+        visited.push(item);
+        list.delete(item);
+      }
+      deepStrictEqual(visited, [1, 2, 3, 4]);
+    });
+  });
 });
diff --git a/src/common/SortedList.ts b/src/common/SortedList.ts
index c5e7bc36..c3250091 100644
--- a/src/common/SortedList.ts
+++ b/src/common/SortedList.ts
@@ -89,7 +89,8 @@ export class SortedList<T> {
   }
 
   public values(): IterableIterator<T> {
-    return this._array.values();
+    // Duplicate the array to avoid issues when _array changes while iterating
+    return [...this._array].values();
   }
 
   private _search(key: number): number {
diff --git a/src/common/buffer/Marker.ts b/src/common/buffer/Marker.ts
index 0629e26a..96df6366 100644
--- a/src/common/buffer/Marker.ts
+++ b/src/common/buffer/Marker.ts
@@ -11,9 +11,9 @@ export class Marker implements IMarker {
   private static _nextId = 1;
 
   public isDisposed: boolean = false;
-  private _disposables: IDisposable[] = [];
+  private readonly _disposables: IDisposable[] = [];
 
-  private _id: number = Marker._nextId++;
+  private readonly _id: number = Marker._nextId++;
   public get id(): number { return this._id; }
 
   private readonly _onDispose = this.register(new EventEmitter<void>());
diff --git a/src/common/services/DecorationService.ts b/src/common/services/DecorationService.ts
index 1ccc712c..ed96fb35 100644
--- a/src/common/services/DecorationService.ts
+++ b/src/common/services/DecorationService.ts
@@ -35,12 +35,7 @@ export class DecorationService extends Disposable implements IDecorationService
   constructor() {
     super();
 
-    this.register(toDisposable(() => {
-      for (const d of this._decorations.values()) {
-        this._onDecorationRemoved.fire(d);
-      }
-      this.reset();
-    }));
+    this.register(toDisposable(() => this.reset()));
   }
 
   public registerDecoration(options: IDecorationOptions): IDecoration | undefined {
@@ -92,13 +87,6 @@ export class DecorationService extends Disposable implements IDecorationService
       }
     });
   }
-
-  public dispose(): void {
-    for (const d of this._decorations.values()) {
-      this._onDecorationRemoved.fire(d);
-    }
-    this.reset();
-  }
 }
 
 class Decoration extends Disposable implements IInternalDecoration {