[MM-68749] Guard webContents finish-load handlers against destroyed objects

[jgheithcock]

Summary

Several webContents.once('did-finish-load' | 'did-frame-finish-load', ...) handlers in the main process touched their underlying Electron object inside the callback body without checking isDestroyed() first. During app.quit (and other teardown races such as removing a popout/server view) a queued finish-load event can fire after the WebContents/BrowserWindow is destroyed; the handler then calls send, focus, setTitle, show, getURL, loadURL, sets zoomLevel, etc., and Electron throws TypeError: Object has been destroyed.

This shows up in Sentry shortly after a burst of renderer.destroyed events. Representative breadcrumb timeline from the linked Sentry issue:

Time (Δms from quit)	Breadcrumb	Notes
-10000	popout window + child renderers (loading screen, URL view, server view) created	user added a non-Mattermost server (404 on /api/v4/system/ping, /api/v4/config/client)
0	window.closed x2, app.quit	main window + popout closed
+2 → +33	6× renderer.destroyed	popout, loading screen, URL view, main window webContents all torn down
+39	TypeError: Object has been destroyed	a once-handler fires against a now-destroyed WebContents

The non-Mattermost server is incidental: the partial-load state just makes pending finish-load events more likely. The same handlers can fire on any quit while a popout/loading screen is mid-load.

This PR applies the same isDestroyed() guard pattern already used elsewhere in the codebase (e.g. webContentsManager.sendToAll, tray.update, MattermostWebContentsView.loadRetry) to:

Commit 1 — did-finish-load / did-frame-finish-load handler guards:

• BaseWindow — before setting webContents.zoomLevel
• PopoutManager.startPopoutWindow + handleViewUpdated — before show / setTitle / send
• LoadingScreen.show + fade — before send / addChildView
• MattermostWebContentsView.useLastPath — before send(BROWSER_HISTORY_PUSH) (also captures lastPath in a local since the field is cleared synchronously after reload())
• ModalView.show — before focus()
• MainWindow — extends existing !this.win check with isDestroyed()
• CallsWidgetWindow — did-frame-finish-load getURL() was outside the existing try/catch

Commit 2 — PerformanceMonitor defense-in-depth:

PerformanceMonitor.registerView / registerServerView use webContents.on('did-finish-load', …) to insert into the metrics maps. If this fires during teardown, the maps end up with a destroyed WebContents reference; the 60s runMetrics / sendMetrics interval would then call send(...) on it. Skip insertion at registration time and lazily drop destroyed entries when iterating.

Ticket Link

https://mattermost.atlassian.net/browse/MM-68749 (Sentry: https://mattermost-mr.sentry.io/issues/7393437427/)

Checklist

• Added or updated unit tests (required for all new features)
• [ ] Has UI changes
• read and understood our Contributing Guidelines
• completed Mattermost Contributor Agreement
• executed npm run lint:js for proper code formatting
• Run E2E tests by adding label E2E/Run

Device Information

N/A — defensive guards in main-process code paths.

Release Note

NONE

Change Impact: 🟠 Medium

Regression Risk: Changes add defensive guards across multiple window/view modules and the PerformanceMonitor. While they are narrowly-scoped (preventing accesses to destroyed Electron objects) and avoid altering public APIs or core business logic, the modifications span several modules (BaseWindow, PopoutManager, LoadingScreen, ModalView, MainWindow, CallsWidgetWindow, MattermostWebContentsView, PerformanceMonitor) and adjust lifecycle behaviors that can affect teardown and app-quit flows. Unit tests were added for key areas (PerformanceMonitor, MattermostWebContentsView, and several view/window tests) but some guarded paths remain simple and could lack exhaustive tests. Overall, moderate risk of regressions in teardown/finish-load sequencing or metrics reporting if edge cases are missed.

QA Recommendation: Perform targeted manual QA on app quit and popout/teardown flows, including:

• Quitting the app during page loads and verifying no "Object has been destroyed" crashes.
• Creating/closing popouts rapidly and ensuring no errors and correct titles/URLs.
• Validating PerformanceMonitor metrics behavior when views are destroyed mid-cycle.
  Given existing automated tests cover many changes, a focused manual test pass is recommended rather than full regression.

Generated by CodeRabbitAI

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR adds defensive checks throughout the Electron app to detect and safely handle destroyed windows and webContents. Event handlers and callbacks now verify that windows, webContents, and views still exist and are not destroyed before attempting to operate on them, preventing errors and stale state updates.

Changes

Window and WebContents Destruction Safety

Layer / File(s)	Summary
BaseWindow load handler guard src/app/windows/baseWindow.ts	The did-finish-load event handler verifies the BrowserWindow and its webContents exist and are not destroyed before proceeding with zoom level and window-ready logic.
UI component load handler guards src/app/mainWindow/mainWindow.ts, src/app/mainWindow/modals/modalView.ts, src/app/callsWidgetWindow.ts	MainWindow, ModalView, and CallsWidgetWindow now guard their respective load handlers (did-finish-load or did-frame-finish-load) to return early if the target window or webContents has been destroyed.
View and popout lifecycle guards src/app/views/MattermostWebContentsView.ts, src/app/views/loadingScreen.ts, src/app/windows/popoutManager.ts	MattermostWebContentsView captures lastPath before registering load handlers; LoadingScreen guards both post-load and already-loaded code paths; PopoutManager guards window state in both startPopoutWindow and handleViewUpdated.
Performance monitoring destruction checks src/main/performanceMonitor.ts	Registration handlers skip destroyed views, runMetrics filters and unregisters destroyed views before creating promises, and sendMetrics skips destroyed server views during iteration to avoid sending metrics on invalid webContents.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and accurately summarizes the primary change: adding defensive guards to webContents finish-load handlers to prevent accessing destroyed Electron objects.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch mm-68749

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/app/views/loadingScreen.ts (1)

49-56: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

show() can still hit destroyed window via unconditional setBounds().

The new guards are good, but this.setBounds() at line 85 still runs unconditionally. In the loading path, it executes immediately after registering the async callback, and in both paths, a destroyed parent between the guard checks and the setBounds() call would cause a crash when accessing the parent in getWindowBoundaries().

Guard setBounds() before calling it to prevent accessing a destroyed parent during application teardown.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/views/loadingScreen.ts` around lines 49 - 56, The show method
currently calls this.setBounds() unconditionally which can access a destroyed
parent via getWindowBoundaries(); modify show (and the did-finish-load callback)
to guard before calling this.setBounds() by checking that
this.view.webContents.isDestroyed() and this.parent.isDestroyed() are false (or
that both are not destroyed) and that this.state is still
LoadingScreenState.VISIBLE, then only call this.setBounds(); update the code
paths around the this.view.webContents.once('did-finish-load', ...) callback and
the immediate path to perform these checks before invoking setBounds().

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/app/views/loadingScreen.ts`:
- Around line 49-56: The show method currently calls this.setBounds()
unconditionally which can access a destroyed parent via getWindowBoundaries();
modify show (and the did-finish-load callback) to guard before calling
this.setBounds() by checking that this.view.webContents.isDestroyed() and
this.parent.isDestroyed() are false (or that both are not destroyed) and that
this.state is still LoadingScreenState.VISIBLE, then only call this.setBounds();
update the code paths around the this.view.webContents.once('did-finish-load',
...) callback and the immediate path to perform these checks before invoking
setBounds().

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: f5416a02-2708-433a-b0cd-4dc2a93740d4

📥 Commits

Reviewing files that changed from the base of the PR and between adfcd2c and 3a8568f.

📒 Files selected for processing (8)

• src/app/callsWidgetWindow.ts
• src/app/mainWindow/mainWindow.ts
• src/app/mainWindow/modals/modalView.ts
• src/app/views/MattermostWebContentsView.ts
• src/app/views/loadingScreen.ts
• src/app/windows/baseWindow.ts
• src/app/windows/popoutManager.ts
• src/main/performanceMonitor.ts

[devinbinnie]

This bit about Electron is really annoying - first off, most of the send() calls should be no-ops if the web contents is destroyed, so I don't know why it has to throw an exception for these. Second, the fact that we have to destroy them in the first place and that they won't be automatically removed is why we have to deal with this at all.

Anyways, rant over. Just one non-blocking comment.

[devinbinnie]

Does this have to be pulled out? I don't think there's a case where this is changed between the call and the did-finish-load event.

[jgheithcock]

@devinbinnie - sorry, thought I had responded:

Yes, we need this. CodeRabbit picked this up and Opus agreed (I envision a Penguin whenever I say that). Line 292 doesn't have this issue as it is synchronous but the once('did-finish-load') is just registered, line 303 sets lastPath to undefined and some ms later, the once handler runs.