Skip to content

[PM-41905] Add Autotype MVP Window Null Check - #22398

Open
coltonhurst wants to merge 1 commit into
mainfrom
dn/pm-41905
Open

[PM-41905] Add Autotype MVP Window Null Check#22398
coltonhurst wants to merge 1 commit into
mainfrom
dn/pm-41905

Conversation

@coltonhurst

Copy link
Copy Markdown
Member

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-41905

📔 Objective

If the main window in the desktop app is destroyed, and a user presses the autotype hotkey combination before the quit processes have completed, that the app would most likely crash due to no conditional checking that the window is still valid (not null) before we send the autotype request from the main process → render process.

Adding a null check prevents this crash, in case there are specific tasks prior to app shut down that need to happen.

@coltonhurst coltonhurst self-assigned this Aug 13, 2026
@coltonhurst coltonhurst added needs-qa Marks a PR as requiring QA approval ai-review Request a Claude code review labels Aug 13, 2026
@coltonhurst
coltonhurst marked this pull request as ready for review August 13, 2026 02:35
@coltonhurst
coltonhurst requested a review from a team as a code owner August 13, 2026 02:35
@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR adds a null check on windowMain.win before sending the autotype IPC message from the main process to the renderer, preventing a crash when the hotkey fires after the main window is destroyed during shutdown. The guard matches the win != null convention used throughout the desktop main process, and the accompanying unit test covers the new branch including the debug log. No security, data-handling, or breaking-change concerns were identified.

Code Review Details
  • 🎨 : Guard also on isDestroyed()WindowMain clears win only after an await in its closed handler, leaving a narrow window where win is non-null but destroyed
    • apps/desktop/src/autofill/main/main-desktop-autotype-mvp.service.ts:105

this.windowMain.win.webContents.send(AUTOTYPE_MVP_IPC_CHANNELS.LISTEN, {
windowTitle,
});
if (this.windowMain.win != null) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 SUGGESTED: Also guard on isDestroyed() to close the remaining shutdown race.

Details and fix

WindowMain only clears this.win inside the closed handler after an await:

// window.main.ts:474
this.win.on("closed", async () => {
  this.isClosing = false;
  await this.updateWindowState(mainWindowSizeKey, this.win); // <- yields
  this.win = null;
});

During that await the window is destroyed but win is still non-null, so a shortcut press in that gap reaches this.windowMain.win.webContents.send(...) and throws Object has been destroyed — the exact crash this PR is closing.

Suggested change
if (this.windowMain.win != null) {
if (this.windowMain.win != null && !this.windowMain.win.isDestroyed()) {

This matches the existing pattern used elsewhere in the desktop main process (window.main.ts:335 and window.main.ts:592).

@codecov

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.45%. Comparing base (a1b2dfc) to head (9736bb9).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #22398      +/-   ##
==========================================
- Coverage   53.45%   53.45%   -0.01%     
==========================================
  Files        4275     4275              
  Lines      135294   135296       +2     
  Branches    21326    21327       +1     
==========================================
- Hits        72328    72326       -2     
- Misses      57679    57683       +4     
  Partials     5287     5287              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review needs-qa Marks a PR as requiring QA approval

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant