From 5862627ec7c9e5c923f92c94e2c3fa4efef5bb87 Mon Sep 17 00:00:00 2001 From: JG Heithcock Date: Thu, 14 May 2026 14:47:03 -0700 Subject: [PATCH 1/3] [MM-68749] Guard webContents finish-load handlers against destroyed objects Several `webContents.once('did-finish-load' | 'did-frame-finish-load', ...)` handlers touched their underlying Electron object without an `isDestroyed()` check. During app quit / popout teardown, queued finish-load events can fire after the WebContents/BrowserWindow is destroyed, throwing "TypeError: Object has been destroyed" (visible in Sentry shortly after a burst of `renderer.destroyed` events). --- src/app/callsWidgetWindow.ts | 5 ++++- src/app/mainWindow/mainWindow.ts | 2 +- src/app/mainWindow/modals/modalView.ts | 5 ++++- src/app/views/MattermostWebContentsView.ts | 6 +++++- src/app/views/loadingScreen.ts | 10 +++++++++- src/app/windows/baseWindow.ts | 3 +++ src/app/windows/popoutManager.ts | 5 ++++- 7 files changed, 30 insertions(+), 6 deletions(-) diff --git a/src/app/callsWidgetWindow.ts b/src/app/callsWidgetWindow.ts index da071cf019b..e44151b32ae 100644 --- a/src/app/callsWidgetWindow.ts +++ b/src/app/callsWidgetWindow.ts @@ -359,7 +359,10 @@ export class CallsWidgetWindow { // 'did-frame-finish-load' is the earliest moment that allows us to call loadURL without throwing an error. // https://mattermost.atlassian.net/browse/MM-52756 is the proper fix for this. this.popOut.webContents.once('did-frame-finish-load', async () => { - const url = this.popOut?.webContents.getURL() || ''; + if (!this.popOut || this.popOut.isDestroyed() || this.popOut.webContents.isDestroyed()) { + return; + } + const url = this.popOut.webContents.getURL() || ''; if (!url) { return; } diff --git a/src/app/mainWindow/mainWindow.ts b/src/app/mainWindow/mainWindow.ts index 3c84d4f81eb..e314de3a7b3 100644 --- a/src/app/mainWindow/mainWindow.ts +++ b/src/app/mainWindow/mainWindow.ts @@ -91,7 +91,7 @@ export class MainWindow extends EventEmitter { } this.win.browserWindow.webContents.once('did-finish-load', () => { - if (!this.win) { + if (!this.win || this.win.browserWindow.isDestroyed()) { return; } diff --git a/src/app/mainWindow/modals/modalView.ts b/src/app/mainWindow/modals/modalView.ts index 37cc303e283..8d5e2223f2b 100644 --- a/src/app/mainWindow/modals/modalView.ts +++ b/src/app/mainWindow/modals/modalView.ts @@ -80,9 +80,12 @@ export class ModalView { this.status = Status.SHOWING; if (this.view.webContents.isLoading()) { this.view.webContents.once('did-finish-load', () => { + if (this.view.webContents.isDestroyed()) { + return; + } this.view.webContents.focus(); }); - } else { + } else if (!this.view.webContents.isDestroyed()) { this.view.webContents.focus(); } diff --git a/src/app/views/MattermostWebContentsView.ts b/src/app/views/MattermostWebContentsView.ts index 593b430283b..0393cdd5d60 100644 --- a/src/app/views/MattermostWebContentsView.ts +++ b/src/app/views/MattermostWebContentsView.ts @@ -291,8 +291,12 @@ export class MattermostWebContentsView extends EventEmitter { if (ViewManager.isPrimaryView(this.view.id)) { this.webContentsView.webContents.send(BROWSER_HISTORY_PUSH, this.lastPath); } else { + const pathToPush = this.lastPath; this.webContentsView.webContents.once('did-finish-load', () => { - this.webContentsView.webContents.send(BROWSER_HISTORY_PUSH, this.lastPath); + if (this.isDestroyed()) { + return; + } + this.webContentsView.webContents.send(BROWSER_HISTORY_PUSH, pathToPush); }); this.webContentsView.webContents.reload(); } diff --git a/src/app/views/loadingScreen.ts b/src/app/views/loadingScreen.ts index c020e772239..1e2f3eb046e 100644 --- a/src/app/views/loadingScreen.ts +++ b/src/app/views/loadingScreen.ts @@ -51,6 +51,9 @@ export class LoadingScreen { if (this.view.webContents.isLoading()) { this.view.webContents.once('did-finish-load', () => { + if (this.view.webContents.isDestroyed() || this.parent.isDestroyed()) { + return; + } if (this.state !== LoadingScreenState.VISIBLE) { return; } @@ -67,6 +70,9 @@ export class LoadingScreen { } }); } else { + if (this.view.webContents.isDestroyed() || this.parent.isDestroyed()) { + return; + } this.view.webContents.send(TOGGLE_LOADING_SCREEN_VISIBILITY, true); log.debug('show: not loading, adding loading screen view'); if (condition?.()) { @@ -83,7 +89,9 @@ export class LoadingScreen { if (this.state === LoadingScreenState.VISIBLE) { log.debug('fade: fading loading screen'); this.state = LoadingScreenState.FADING; - this.view.webContents.send(TOGGLE_LOADING_SCREEN_VISIBILITY, false); + if (!this.view.webContents.isDestroyed()) { + this.view.webContents.send(TOGGLE_LOADING_SCREEN_VISIBILITY, false); + } } }; diff --git a/src/app/windows/baseWindow.ts b/src/app/windows/baseWindow.ts index 00529c59dfe..fc54473689a 100644 --- a/src/app/windows/baseWindow.ts +++ b/src/app/windows/baseWindow.ts @@ -71,6 +71,9 @@ export default class BaseWindow { this.win.setMenuBarVisibility(false); this.win.webContents.once('did-finish-load', () => { + if (!this.win || this.win.isDestroyed() || this.win.webContents.isDestroyed()) { + return; + } this.win.webContents.zoomLevel = 0; this.ready = true; }); diff --git a/src/app/windows/popoutManager.ts b/src/app/windows/popoutManager.ts index 67d8b0f65fb..4887f3c469e 100644 --- a/src/app/windows/popoutManager.ts +++ b/src/app/windows/popoutManager.ts @@ -139,6 +139,9 @@ export class PopoutManager { private startPopoutWindow = (viewId: string, window: BaseWindow) => { window.browserWindow.webContents.once('did-finish-load', () => { + if (!window.browserWindow || window.browserWindow.isDestroyed() || window.browserWindow.webContents.isDestroyed()) { + return; + } this.handleViewUpdated(viewId); window.browserWindow.show(); }); @@ -244,7 +247,7 @@ export class PopoutManager { const view = ViewManager.getView(viewId); if (view && view.type === ViewType.WINDOW) { const window = this.popoutWindows.get(viewId); - if (window) { + if (window?.browserWindow && !window.browserWindow.isDestroyed() && !window.browserWindow.webContents.isDestroyed()) { const title = ViewManager.getViewTitle(viewId); window.browserWindow.setTitle(title); window.browserWindow.webContents.send(UPDATE_POPOUT_TITLE, viewId, title); From 3a8568f37eb5a307b0d9b77ec7d622560da0e945 Mon Sep 17 00:00:00 2001 From: JG Heithcock Date: Thu, 14 May 2026 16:01:53 -0700 Subject: [PATCH 2/3] [MM-68749] Skip destroyed WebContents in PerformanceMonitor The `did-finish-load` handlers used to register views could leak references to destroyed WebContents into the metrics maps if the event fired during teardown. The 60s metrics interval would then call `webContents.send(...)` on those entries and throw "TypeError: Object has been destroyed". Guard at registration time and lazily drop destroyed entries from the maps in `runMetrics` and `sendMetrics`. --- src/main/performanceMonitor.ts | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/src/main/performanceMonitor.ts b/src/main/performanceMonitor.ts index 99708073987..c459bb4bac0 100644 --- a/src/main/performanceMonitor.ts +++ b/src/main/performanceMonitor.ts @@ -58,6 +58,9 @@ export class PerformanceMonitor { log.debug('registerView', webContents.id, name); webContents.on('did-finish-load', () => { + if (webContents.isDestroyed()) { + return; + } this.views.set(webContents.id, {name, webContents, serverId}); }); }; @@ -66,6 +69,9 @@ export class PerformanceMonitor { log.debug('registerServerView', {webContentsId: webContents.id, serverId}); webContents.on('did-finish-load', () => { + if (webContents.isDestroyed()) { + return; + } this.serverViews.set(webContents.id, {name, webContents, serverId}); }); }; @@ -120,12 +126,20 @@ export class PerformanceMonitor { viewResolves.get(event.sender.id)?.(); }; ipcMain.on(METRICS_RECEIVE, listener); - const viewPromises = [...this.views.values(), ...this.serverViews.values()].map((view) => { - return new Promise((resolve) => { - viewResolves.set(view.webContents.id, resolve); - view.webContents.send(METRICS_REQUEST, view.name, view.serverId); + const viewPromises = [...this.views.values(), ...this.serverViews.values()]. + filter((view) => { + if (view.webContents.isDestroyed()) { + this.unregisterView(view.webContents.id); + return false; + } + return true; + }). + map((view) => { + return new Promise((resolve) => { + viewResolves.set(view.webContents.id, resolve); + view.webContents.send(METRICS_REQUEST, view.name, view.serverId); + }); }); - }); // After 5 seconds, if all the promises are not resolved, resolve them so we don't block the send // This can happen if a view doesn't send back metrics information @@ -151,6 +165,11 @@ export class PerformanceMonitor { continue; } + if (view.webContents.isDestroyed()) { + this.unregisterView(view.webContents.id); + continue; + } + const serverMetricsMap = new Map([...metricsMap].filter((value) => !value[1].serverId || value[1].serverId === view.serverId)); view.webContents.send(METRICS_SEND, serverMetricsMap); } From 8f49c5de4c8e3a095c06a682dd769dfc5ce234a0 Mon Sep 17 00:00:00 2001 From: JG Heithcock Date: Thu, 14 May 2026 16:21:38 -0700 Subject: [PATCH 3/3] Fix 7 unit tests with missing isDestroyed() mock --- src/app/callsWidgetWindow.test.js | 1 + src/app/mainWindow/mainWindow.test.js | 2 + src/app/mainWindow/modals/modalView.test.js | 1 + .../views/MattermostWebContentsView.test.js | 78 ++++++++++++++++++- src/app/views/loadingScreen.test.js | 2 + src/app/windows/baseWindow.test.js | 1 + src/app/windows/popoutManager.test.js | 4 + src/main/performanceMonitor.test.js | 61 ++++++++++++++- 8 files changed, 147 insertions(+), 3 deletions(-) diff --git a/src/app/callsWidgetWindow.test.js b/src/app/callsWidgetWindow.test.js index 4aafdc51fd4..93738a142b3 100644 --- a/src/app/callsWidgetWindow.test.js +++ b/src/app/callsWidgetWindow.test.js @@ -448,6 +448,7 @@ describe('main/windows/callsWidgetWindow', () => { id: 'webContentsId', getURL: () => ('http://myurl.com'), removeListener: jest.fn(), + isDestroyed: jest.fn(() => false), }, off: jest.fn(), loadURL: jest.fn(), diff --git a/src/app/mainWindow/mainWindow.test.js b/src/app/mainWindow/mainWindow.test.js index b395a9543f6..5c0b1dc2447 100644 --- a/src/app/mainWindow/mainWindow.test.js +++ b/src/app/mainWindow/mainWindow.test.js @@ -110,6 +110,7 @@ describe('main/windows/mainWindow', () => { send: jest.fn(), setWindowOpenHandler: jest.fn(), zoomLevel: 0, + isDestroyed: jest.fn(() => false), }, contentView: { on: jest.fn(), @@ -118,6 +119,7 @@ describe('main/windows/mainWindow', () => { isFullScreen: jest.fn(), getBounds: jest.fn(), isMinimized: jest.fn().mockReturnValue(false), + isDestroyed: jest.fn(() => false), }; beforeEach(() => { diff --git a/src/app/mainWindow/modals/modalView.test.js b/src/app/mainWindow/modals/modalView.test.js index 364e096101b..fa39428169f 100644 --- a/src/app/mainWindow/modals/modalView.test.js +++ b/src/app/mainWindow/modals/modalView.test.js @@ -17,6 +17,7 @@ jest.mock('electron', () => ({ isDevToolsOpened: jest.fn(), closeDevTools: jest.fn(), close: jest.fn(), + isDestroyed: jest.fn(() => false), }, setBounds: jest.fn(), setAutoResize: jest.fn(), diff --git a/src/app/views/MattermostWebContentsView.test.js b/src/app/views/MattermostWebContentsView.test.js index 88ae6192259..fff95581cf5 100644 --- a/src/app/views/MattermostWebContentsView.test.js +++ b/src/app/views/MattermostWebContentsView.test.js @@ -4,7 +4,7 @@ 'use strict'; import AppState from 'common/appState'; -import {LOAD_FAILED, UPDATE_TARGET_URL} from 'common/communication'; +import {BROWSER_HISTORY_PUSH, LOAD_FAILED, UPDATE_TARGET_URL} from 'common/communication'; import {MattermostServer} from 'common/servers/MattermostServer'; import ServerManager from 'common/servers/serverManager'; import {MattermostView, ViewType} from 'common/views/MattermostView'; @@ -26,6 +26,8 @@ jest.mock('electron', () => ({ webContents: { loadURL: jest.fn(), on: jest.fn(), + once: jest.fn(), + reload: jest.fn(), getTitle: () => 'title', getURL: () => 'http://server-1.com', send: jest.fn(), @@ -36,7 +38,7 @@ jest.mock('electron', () => ({ goToOffset: jest.fn(), canGoToOffset: jest.fn(), }, - isDestroyed: jest.fn(), + isDestroyed: jest.fn(() => false), }, })), ipcMain: { @@ -99,6 +101,7 @@ jest.mock('main/server/serverAPI', () => ({ })); jest.mock('common/views/viewManager', () => ({ updateViewTitle: jest.fn(), + isPrimaryView: jest.fn(), getViewLog: jest.fn().mockReturnValue({ info: jest.fn(), verbose: jest.fn(), @@ -469,4 +472,75 @@ describe('main/views/MattermostWebContentsView', () => { expect(ViewManager.updateViewTitle).toHaveBeenCalledWith(mattermostView.id, 'Just Channel Name'); }); }); + + describe('useLastPath', () => { + const window = {on: jest.fn(), webContents: {send: jest.fn()}}; + let mattermostView; + + beforeEach(() => { + MainWindow.get.mockReturnValue(window); + mattermostView = new MattermostWebContentsView(view, {}, window); + }); + + it('should send BROWSER_HISTORY_PUSH immediately for the primary view', () => { + ViewManager.isPrimaryView.mockReturnValue(true); + mattermostView.setLastPath('/team/channel'); + + mattermostView.useLastPath(); + + expect(mattermostView.webContentsView.webContents.send).toHaveBeenCalledWith(BROWSER_HISTORY_PUSH, '/team/channel'); + expect(mattermostView.webContentsView.webContents.reload).not.toHaveBeenCalled(); + expect(mattermostView.lastPath).toBeUndefined(); + }); + + it('should send the captured path after reload, even though lastPath was cleared synchronously', () => { + ViewManager.isPrimaryView.mockReturnValue(false); + + let didFinishLoadCb; + mattermostView.webContentsView.webContents.once.mockImplementation((event, cb) => { + if (event === 'did-finish-load') { + didFinishLoadCb = cb; + } + }); + + mattermostView.setLastPath('/team/channel'); + mattermostView.useLastPath(); + + expect(mattermostView.webContentsView.webContents.reload).toHaveBeenCalled(); + expect(mattermostView.lastPath).toBeUndefined(); + + didFinishLoadCb(); + + expect(mattermostView.webContentsView.webContents.send).toHaveBeenCalledWith(BROWSER_HISTORY_PUSH, '/team/channel'); + }); + + it('should not send when the webContents is destroyed before did-finish-load fires', () => { + ViewManager.isPrimaryView.mockReturnValue(false); + + let didFinishLoadCb; + mattermostView.webContentsView.webContents.once.mockImplementation((event, cb) => { + if (event === 'did-finish-load') { + didFinishLoadCb = cb; + } + }); + + mattermostView.setLastPath('/team/channel'); + mattermostView.useLastPath(); + + mattermostView.webContentsView.webContents.isDestroyed.mockReturnValue(true); + didFinishLoadCb(); + + expect(mattermostView.webContentsView.webContents.send).not.toHaveBeenCalledWith(BROWSER_HISTORY_PUSH, expect.anything()); + }); + + it('should be a no-op when lastPath is not set', () => { + ViewManager.isPrimaryView.mockReturnValue(false); + + mattermostView.useLastPath(); + + expect(mattermostView.webContentsView.webContents.once).not.toHaveBeenCalled(); + expect(mattermostView.webContentsView.webContents.reload).not.toHaveBeenCalled(); + expect(mattermostView.webContentsView.webContents.send).not.toHaveBeenCalledWith(BROWSER_HISTORY_PUSH, expect.anything()); + }); + }); }); diff --git a/src/app/views/loadingScreen.test.js b/src/app/views/loadingScreen.test.js index 35ca2876cf7..0c8a1abeb4b 100644 --- a/src/app/views/loadingScreen.test.js +++ b/src/app/views/loadingScreen.test.js @@ -19,6 +19,7 @@ jest.mock('electron', () => { mockWebContents.send = jest.fn(); mockWebContents.loadURL = jest.fn(); mockWebContents.isLoading = jest.fn(); + mockWebContents.isDestroyed = jest.fn(() => false); return { webContents: mockWebContents, @@ -50,6 +51,7 @@ describe('main/views/loadingScreen', () => { webContents: { id: 123, }, + isDestroyed: jest.fn(() => false), }; const loadingScreen = new LoadingScreen(mainWindow); diff --git a/src/app/windows/baseWindow.test.js b/src/app/windows/baseWindow.test.js index 7b789112357..2210d51e162 100644 --- a/src/app/windows/baseWindow.test.js +++ b/src/app/windows/baseWindow.test.js @@ -61,6 +61,7 @@ jest.mock('electron', () => { mockWebContents.on = jest.fn(mockWebContents.on); mockWebContents.once = jest.fn(mockWebContents.once); mockWebContents.emit = jest.fn(mockWebContents.emit); + mockWebContents.isDestroyed = jest.fn(() => false); mockBrowserWindow.webContents = mockWebContents; mockBrowserWindow.getContentBounds = jest.fn(() => ({x: 0, y: 0, width: 800, height: 600})); mockBrowserWindow.getSize = jest.fn(() => [800, 600]); diff --git a/src/app/windows/popoutManager.test.js b/src/app/windows/popoutManager.test.js index 9c63492f812..92864d1d2a8 100644 --- a/src/app/windows/popoutManager.test.js +++ b/src/app/windows/popoutManager.test.js @@ -55,6 +55,7 @@ jest.mock('app/windows/baseWindow', () => { mockWebContents.on = jest.fn(mockWebContents.on); mockWebContents.once = jest.fn(mockWebContents.once); mockWebContents.emit = jest.fn(mockWebContents.emit); + mockWebContents.isDestroyed = jest.fn(() => false); const mockBrowserWindow = { webContents: mockWebContents, contentView: { @@ -70,6 +71,7 @@ jest.mock('app/windows/baseWindow', () => { close: jest.fn(), setTitle: jest.fn(), loadURL: jest.fn(() => Promise.resolve()), + isDestroyed: jest.fn(() => false), }; return jest.fn(() => ({ @@ -150,6 +152,7 @@ describe('PopoutManager', () => { mockWebContents.on = jest.fn(mockWebContents.on); mockWebContents.once = jest.fn(mockWebContents.once); mockWebContents.emit = jest.fn(mockWebContents.emit); + mockWebContents.isDestroyed = jest.fn(() => false); const mockBaseWindow = { browserWindow: { webContents: mockWebContents, @@ -166,6 +169,7 @@ describe('PopoutManager', () => { close: jest.fn(), setTitle: jest.fn(), loadURL: jest.fn(() => Promise.resolve()), + isDestroyed: jest.fn(() => false), }, showLoadingScreen: jest.fn(), fadeLoadingScreen: jest.fn(), diff --git a/src/main/performanceMonitor.test.js b/src/main/performanceMonitor.test.js index 3b0b5678459..f041acdc49e 100644 --- a/src/main/performanceMonitor.test.js +++ b/src/main/performanceMonitor.test.js @@ -43,7 +43,7 @@ describe('main/performanceMonitor', () => { } }); - makeWebContents = (id, resolve) => ({ + makeWebContents = (id, resolve, {destroyed = false} = {}) => ({ send: jest.fn().mockImplementation((channel, arg1, arg2) => { if (channel === METRICS_REQUEST) { cb({sender: {id}}, arg1, {serverId: arg2, cpu: id, memory: id * 100}); @@ -53,6 +53,7 @@ describe('main/performanceMonitor', () => { } }), on: (_, listener) => listener(), + isDestroyed: jest.fn(() => destroyed), id, }); }); @@ -252,4 +253,62 @@ describe('main/performanceMonitor', () => { expect(await sendValue2).toEqual(new Map([['view-1', {cpu: 1, memory: 100, serverId: 'server-1'}]])); }); }); + + describe('destroyed view handling', () => { + it('should not register a view when its webContents is already destroyed at did-finish-load', () => { + const performanceMonitor = new PerformanceMonitor(); + performanceMonitor.init(); + + const destroyedWebContents = makeWebContents(42, jest.fn(), {destroyed: true}); + performanceMonitor.registerView('view-1', destroyedWebContents); + performanceMonitor.registerServerView('view-2', destroyedWebContents, 'server-1'); + + expect(performanceMonitor.views.has(42)).toBe(false); + expect(performanceMonitor.serverViews.has(42)).toBe(false); + }); + + it('runMetrics should unregister and skip destroyed views', async () => { + const performanceMonitor = new PerformanceMonitor(); + performanceMonitor.init(); + + const liveResolve = jest.fn(); + const liveWebContents = makeWebContents(1, liveResolve); + const destroyedWebContents = makeWebContents(2, jest.fn()); + + performanceMonitor.registerServerView('view-live', liveWebContents, 'server-1'); + performanceMonitor.registerServerView('view-destroyed', destroyedWebContents, 'server-1'); + + // Mark as destroyed only after registration so the views map ends up with both entries + destroyedWebContents.isDestroyed.mockReturnValue(true); + + const metrics = await performanceMonitor.runMetrics(); + + expect(performanceMonitor.serverViews.has(2)).toBe(false); + expect(performanceMonitor.serverViews.has(1)).toBe(true); + expect(destroyedWebContents.send).not.toHaveBeenCalled(); + expect(liveWebContents.send).toHaveBeenCalledWith(METRICS_REQUEST, 'view-live', 'server-1'); + expect(metrics.has('view-live')).toBe(true); + expect(metrics.has('view-destroyed')).toBe(false); + }); + + it('sendMetrics should unregister destroyed serverViews and not call send on them', async () => { + const performanceMonitor = new PerformanceMonitor(); + performanceMonitor.init(); + + const sendValue = new Promise((resolve) => { + performanceMonitor.registerServerView('view-1', makeWebContents(1, resolve), 'server-1'); + }); + const destroyedWebContents = makeWebContents(2, jest.fn()); + performanceMonitor.registerServerView('view-2', destroyedWebContents, 'server-1'); + + // After registration, mark view-2 as destroyed before the next interval fires + destroyedWebContents.isDestroyed.mockReturnValue(true); + + jest.runOnlyPendingTimers(); + await sendValue; + + expect(performanceMonitor.serverViews.has(2)).toBe(false); + expect(destroyedWebContents.send).not.toHaveBeenCalledWith(METRICS_SEND, expect.anything()); + }); + }); });