1
0
mirror of https://github.com/bitwarden/browser synced 2025-12-10 13:23:34 +00:00

[PM-2348] Close popup window before process reload (#16795)

* Close popup window before process reload

* unit test coverage
This commit is contained in:
Maciej Zieniuk
2025-10-21 19:06:55 +02:00
committed by GitHub
parent 65da23feaa
commit 63cdae92be
4 changed files with 100 additions and 0 deletions

View File

@@ -300,6 +300,7 @@ import { BrowserActionsService } from "../platform/actions/browser-actions.servi
import { DefaultBadgeBrowserApi } from "../platform/badge/badge-browser-api";
import { BadgeService } from "../platform/badge/badge.service";
import { BrowserApi } from "../platform/browser/browser-api";
import BrowserPopupUtils from "../platform/browser/browser-popup-utils";
import { flagEnabled } from "../platform/flags";
import { IpcBackgroundService } from "../platform/ipc/ipc-background.service";
import { IpcContentScriptManagerService } from "../platform/ipc/ipc-content-script-manager.service";
@@ -1237,6 +1238,12 @@ export default class MainBackground {
const systemUtilsServiceReloadCallback = async () => {
await this.taskSchedulerService.clearAllScheduledTasks();
// Close browser action popup before reloading to prevent zombie popup with invalidated context.
// The 'reloadProcess' message is sent by ProcessReloadService before this callback runs,
// and popups will close themselves upon receiving it. Poll to verify popup is actually closed.
await BrowserPopupUtils.waitForAllPopupsClose();
BrowserApi.reloadExtension();
};

View File

@@ -337,6 +337,68 @@ describe("BrowserPopupUtils", () => {
});
});
describe("waitForAllPopupsClose", () => {
beforeEach(() => {
jest.useFakeTimers();
});
afterEach(() => {
jest.useRealTimers();
});
it("should resolve immediately if no popups are open", async () => {
jest.spyOn(BrowserApi, "isPopupOpen").mockResolvedValue(false);
const promise = BrowserPopupUtils.waitForAllPopupsClose();
jest.advanceTimersByTime(100);
await expect(promise).resolves.toBeUndefined();
expect(BrowserApi.isPopupOpen).toHaveBeenCalledTimes(1);
});
it("should resolve after timeout if popup never closes when using custom timeout", async () => {
jest.spyOn(BrowserApi, "isPopupOpen").mockResolvedValue(true);
const promise = BrowserPopupUtils.waitForAllPopupsClose(500);
// Advance past the timeout
jest.advanceTimersByTime(600);
await expect(promise).resolves.toBeUndefined();
});
it("should resolve after timeout if popup never closes when using default timeout", async () => {
jest.spyOn(BrowserApi, "isPopupOpen").mockResolvedValue(true);
const promise = BrowserPopupUtils.waitForAllPopupsClose();
// Advance past the default timeout
jest.advanceTimersByTime(1100);
await expect(promise).resolves.toBeUndefined();
});
it("should stop polling after popup closes before timeout", async () => {
let callCount = 0;
jest.spyOn(BrowserApi, "isPopupOpen").mockImplementation(async () => {
callCount++;
return callCount <= 2;
});
const promise = BrowserPopupUtils.waitForAllPopupsClose(1000);
// Advance to when popup closes (300ms)
jest.advanceTimersByTime(300);
await expect(promise).resolves.toBeUndefined();
// Advance further to ensure no more calls are made
jest.advanceTimersByTime(1000);
expect(BrowserApi.isPopupOpen).toHaveBeenCalledTimes(3);
});
});
describe("isSingleActionPopoutOpen", () => {
const windowOptions = {
id: 1,

View File

@@ -1,5 +1,6 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
import { filter, firstValueFrom, interval, of, switchMap, takeWhile, timeout } from "rxjs";
import { ScrollOptions } from "./abstractions/browser-popup-utils.abstractions";
import { BrowserApi } from "./browser-api";
@@ -212,6 +213,27 @@ export default class BrowserPopupUtils {
}
}
/**
* Waits for all browser action popups to close, polling up to the specified timeout.
* Used before extension reload to prevent zombie popups with invalidated contexts.
*
* @param timeoutMs - Maximum time to wait in milliseconds. Defaults to 1 second.
* @returns Promise that resolves when all popups are closed or timeout is reached.
*/
static async waitForAllPopupsClose(timeoutMs = 1000): Promise<void> {
await firstValueFrom(
interval(100).pipe(
switchMap(() => BrowserApi.isPopupOpen()),
takeWhile((isOpen) => isOpen, true),
filter((isOpen) => !isOpen),
timeout({
first: timeoutMs,
with: () => of(true),
}),
),
);
}
/**
* Identifies if a single action window is open based on the passed popoutKey.
* Will focus the existing window, and close any other windows that might exist

View File

@@ -36,6 +36,7 @@ import {
LogoutReason,
UserDecryptionOptionsServiceAbstraction,
} from "@bitwarden/auth/common";
import { BrowserApi } from "@bitwarden/browser/platform/browser/browser-api";
import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { AuthRequestAnsweringServiceAbstraction } from "@bitwarden/common/auth/abstractions/auth-request-answering/auth-request-answering.service.abstraction";
import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
@@ -58,6 +59,7 @@ import {
} from "@bitwarden/components";
import { BiometricsService, BiometricStateService, KeyService } from "@bitwarden/key-management";
import BrowserPopupUtils from "../platform/browser/browser-popup-utils";
import { PopupCompactModeService } from "../platform/popup/layout/popup-compact-mode.service";
import { PopupSizeService } from "../platform/popup/layout/popup-size.service";
import { initPopupClosedListener } from "../platform/services/popup-view-cache-background.service";
@@ -286,6 +288,13 @@ export class AppComponent implements OnInit, OnDestroy {
await this.biometricStateService.updateLastProcessReload();
window.location.reload();
}, 2000);
} else {
// Close browser action popup before extension reload to prevent zombie popup with invalidated context.
// This issue occurs in Chromium-based browsers (Chrome, Vivaldi, etc.) where chrome.runtime.reload()
// invalidates extension contexts before popup can close naturally
if (BrowserPopupUtils.inPopup(window)) {
BrowserApi.closePopup(window);
}
}
} else if (msg.command === "reloadPopup") {
// FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.