Merge pull request #2891 from github/koesie10/dispose-webview

Dispose tracked objects when panel is disposed

Author: koesie10

extensions/ql-vscode/src/common/vscode/abstract-webview.ts

@@ -9,7 +9,7 @@ import {
 import { join } from "path";
 
 import { App } from "../app";
-import { DisposableObject, DisposeHandler } from "../disposable-object";
+import { Disposable } from "../disposable-object";
 import { tmpDir } from "../../tmp-dir";
 import { getHtmlForWebview, WebviewMessage, WebviewKind } from "./webview-html";
 
@@ -27,16 +27,16 @@ export type WebviewPanelConfig = {
 export abstract class AbstractWebview<
   ToMessage extends WebviewMessage,
   FromMessage extends WebviewMessage,
-> extends DisposableObject {
+> {
   protected panel: WebviewPanel | undefined;
   protected panelLoaded = false;
   protected panelLoadedCallBacks: Array<() => void> = [];
 
   private panelResolves?: Array<(panel: WebviewPanel) => void>;
 
-  constructor(protected readonly app: App) {
-    super();
-  }
+  private disposables: Disposable[] = [];
+
+  constructor(protected readonly app: App) {}
 
   public async restoreView(panel: WebviewPanel): Promise<void> {
     this.panel = panel;
@@ -101,6 +101,7 @@ export abstract class AbstractWebview<
         this.panel = undefined;
         this.panelLoaded = false;
         this.onPanelDispose();
+        this.disposeAll();
       }, null),
     );
 
@@ -150,8 +151,27 @@ export abstract class AbstractWebview<
     return panel.webview.postMessage(msg);
   }
 
-  public dispose(disposeHandler?: DisposeHandler) {
+  public dispose() {
     this.panel?.dispose();
-    super.dispose(disposeHandler);
+    this.disposeAll();
+  }
+
+  private disposeAll() {
+    while (this.disposables.length > 0) {
+      const disposable = this.disposables.pop()!;
+      disposable.dispose();
+    }
+  }
+
+  /**
+   * Adds `obj` to a list of objects to dispose when the panel is disposed. Objects added by `push` are
+   * disposed in reverse order of being added.
+   * @param obj The object to take ownership of.
+   */
+  protected push<T extends Disposable>(obj: T): T {
+    if (obj !== undefined) {
+      this.disposables.push(obj);
+    }
+    return obj;
   }
 }

extensions/ql-vscode/src/local-queries/results-view.ts

@@ -75,6 +75,7 @@ import { telemetryListener } from "../common/vscode/telemetry";
 import { redactableError } from "../common/errors";
 import { ResultsViewCommands } from "../common/commands";
 import { App } from "../common/app";
+import { Disposable } from "../common/disposable-object";
 
 /**
  * results-view.ts
@@ -157,6 +158,12 @@ function numInterpretedPages(
   return Math.ceil(n / pageSize);
 }
 
+/**
+ * The results view is used for displaying the results of a local query. It is a singleton; only 1 results view exists
+ * in the extension. It is created when the extension is activated and disposed of when the extension is deactivated.
+ * There can be multiple panels linked to this view over the lifetime of the extension, but there is only ever 1 panel
+ * active at a time.
+ */
 export class ResultsView extends AbstractWebview<
   IntoResultsViewMsg,
   FromResultsViewMsg
@@ -168,6 +175,9 @@ export class ResultsView extends AbstractWebview<
     "codeql-query-results",
   );
 
+  // Event listeners that should be disposed of when the view is disposed.
+  private disposableEventListeners: Disposable[] = [];
+
   constructor(
     app: App,
     private databaseManager: DatabaseManager,
@@ -176,14 +186,16 @@ export class ResultsView extends AbstractWebview<
     private labelProvider: HistoryItemLabelProvider,
   ) {
     super(app);
-    this.push(this._diagnosticCollection);
-    this.push(
+
+    // We can't use this.push for these two event listeners because they need to be disposed of when the view is
+    // disposed, not when the panel is disposed. The results view is a singleton, so we shouldn't be calling this.push.
+    this.disposableEventListeners.push(
       vscode.window.onDidChangeTextEditorSelection(
         this.handleSelectionChange.bind(this),
       ),
     );
 
-    this.push(
+    this.disposableEventListeners.push(
       this.databaseManager.onDidChangeDatabaseItem(({ kind }) => {
         if (kind === DatabaseEventKind.Remove) {
           this._diagnosticCollection.clear();
@@ -981,4 +993,12 @@ export class ResultsView extends AbstractWebview<
       editor.setDecorations(shownLocationLineDecoration, []);
     }
   }
+
+  dispose() {
+    super.dispose();
+
+    this._diagnosticCollection.dispose();
+    this.disposableEventListeners.forEach((d) => d.dispose());
+    this.disposableEventListeners = [];
+  }
 }