From e543fe0fb3604b28906aaea9f43af583e355919f Mon Sep 17 00:00:00 2001 From: Eric Amodio Date: Wed, 5 Apr 2023 23:13:41 -0400 Subject: [PATCH] Protects against reusing disposed webview parent Was causing the Commit Details view to reshow itself when it shouldn't, because its `visible` flag got stuck --- src/webviews/webviewController.ts | 34 ++++++++++++++++--------------- src/webviews/webviewsController.ts | 41 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 57 insertions(+), 18 deletions(-) diff --git a/src/webviews/webviewController.ts b/src/webviews/webviewController.ts index 6c6465c..a5fd834 100644 --- a/src/webviews/webviewController.ts +++ b/src/webviews/webviewController.ts @@ -1,12 +1,5 @@ -import type { - Disposable, - Webview, - WebviewPanel, - WebviewPanelOnDidChangeViewStateEvent, - WebviewView, - WindowState, -} from 'vscode'; -import { EventEmitter, Uri, ViewColumn, window, workspace } from 'vscode'; +import type { Webview, WebviewPanel, WebviewPanelOnDidChangeViewStateEvent, WebviewView, WindowState } from 'vscode'; +import { Disposable, EventEmitter, Uri, ViewColumn, window, workspace } from 'vscode'; import { getNonce } from '@env/crypto'; import type { Commands, CustomEditorIds, WebviewIds, WebviewViewIds } from '../constants'; import type { Container } from '../container'; @@ -72,7 +65,7 @@ type WebviewViewController = WebviewController< WebviewViewDescriptor >; -@logName>((c, name) => `${name}(${c.id})`) +@logName>(c => `WebviewController(${c.id})`) export class WebviewController< State, SerializedState = State, @@ -129,7 +122,7 @@ export class WebviewController< return this._ready; } - private readonly disposables: Disposable[] = []; + private disposable: Disposable | undefined; private /*readonly*/ provider!: WebviewProvider; private readonly webview: Webview; @@ -152,7 +145,12 @@ export class WebviewController< this._initializing = resolveProvider(container, this).then(provider => { this.provider = provider; - this.disposables.push( + if (this._disposed) { + provider.dispose(); + return; + } + + this.disposable = Disposable.from( window.onDidChangeWindowState(this.onWindowStateChanged, this), parent.webview.onDidReceiveMessage(this.onMessageReceivedCore, this), isInEditor @@ -165,17 +163,19 @@ export class WebviewController< }); } + private _disposed: boolean = false; dispose() { + this._disposed = true; resetContextKeys(this.descriptor.contextKeyPrefix); - this.provider.onFocusChanged?.(false); - this.provider.onVisibilityChanged?.(false); + this.provider?.onFocusChanged?.(false); + this.provider?.onVisibilityChanged?.(false); this._ready = false; this._suspended = false; this._onDidDispose.fire(); - this.disposables.forEach(d => void d.dispose()); + this.disposable?.dispose(); } private _initializing: Promise | undefined; @@ -221,9 +221,10 @@ export class WebviewController< } get visible() { - return this.parent.visible; + return this._disposed ? false : this.parent.visible; } + @debug() async show(loading: boolean, options?: { column?: ViewColumn; preserveFocus?: boolean }, ...args: unknown[]) { if (options == null) { options = {}; @@ -289,6 +290,7 @@ export class WebviewController< this.webview.html = html; } + @debug() private onParentDisposed() { this.dispose(); } diff --git a/src/webviews/webviewsController.ts b/src/webviews/webviewsController.ts index 3090313..6feca56 100644 --- a/src/webviews/webviewsController.ts +++ b/src/webviews/webviewsController.ts @@ -10,6 +10,9 @@ import type { Commands, WebviewIds, WebviewViewIds } from '../constants'; import type { Container } from '../container'; import { ensurePlusFeaturesEnabled } from '../plus/subscription/utils'; import { executeCommand, registerCommand } from '../system/command'; +import { debug } from '../system/decorators/log'; +import { Logger } from '../system/logger'; +import { getLogScope } from '../system/logger.scope'; import type { TrackedUsageFeatures } from '../telemetry/usageTracker'; import type { WebviewProvider } from './webviewController'; import { WebviewController } from './webviewController'; @@ -79,6 +82,13 @@ export class WebviewsController implements Disposable { this.disposables.forEach(d => void d.dispose()); } + @debug({ + args: { + 0: d => d.id, + 1: false, + 2: false, + }, + }) registerWebviewView( descriptor: WebviewViewDescriptor, resolveProvider: ( @@ -87,6 +97,8 @@ export class WebviewsController implements Disposable { ) => Promise>, canResolveProvider?: () => boolean | Promise, ): WebviewViewProxy { + const scope = getLogScope(); + const registration: WebviewViewRegistration = { descriptor: descriptor }; this._views.set(descriptor.id, registration); @@ -109,6 +121,8 @@ export class WebviewsController implements Disposable { if (token.isCancellationRequested) return; } + Logger.debug(scope, `Resolving webview view (${descriptor.id})`); + webviewView.webview.options = { enableCommandUris: true, enableScripts: true, @@ -124,13 +138,17 @@ export class WebviewsController implements Disposable { webviewView, resolveProvider, ); + + registration.controller?.dispose(); registration.controller = controller; disposables.push( controller.onDidDispose(() => { + Logger.debug(scope, `Disposing webview view (${descriptor.id})`); + registration.pendingShowArgs = undefined; registration.controller = undefined; - }, this), + }), controller, ); @@ -164,12 +182,22 @@ export class WebviewsController implements Disposable { show: async (options?: { preserveFocus?: boolean }, ...args) => { if (registration.controller != null) return void registration.controller.show(false, options, ...args); + Logger.debug(scope, `Showing webview view (${descriptor.id})`); + registration.pendingShowArgs = [options, ...args]; return void executeCommand(`${descriptor.id}.focus`, options); }, } satisfies WebviewViewProxy; } + @debug({ + args: { + 0: c => c, + 1: d => d.id, + 2: false, + 3: false, + }, + }) registerWebviewPanel( command: Commands, descriptor: WebviewPanelDescriptor, @@ -179,6 +207,8 @@ export class WebviewsController implements Disposable { ) => Promise>, canResolveProvider?: () => boolean | Promise, ): WebviewPanelProxy { + const scope = getLogScope(); + const registration: WebviewPanelRegistration = { descriptor: descriptor }; this._panels.set(descriptor.id, registration); @@ -208,6 +238,8 @@ export class WebviewsController implements Disposable { let { controller } = registration; if (controller == null) { + Logger.debug(scope, `Creating webview panel (${descriptor.id})`); + const panel = window.createWebviewPanel( descriptor.id, descriptor.title, @@ -228,12 +260,17 @@ export class WebviewsController implements Disposable { registration.controller = controller; disposables.push( - controller.onDidDispose(() => (registration.controller = undefined)), + controller.onDidDispose(() => { + Logger.debug(scope, `Disposing webview panel (${descriptor.id})`); + + registration.controller = undefined; + }), controller, ); await controller.show(true, options, ...args); } else { + Logger.debug(scope, `Showing webview panel (${descriptor.id})`); await controller.show(false, options, ...args); } }