From c3f6892f9ee74fd1200b659878ed76cfde0ce6b9 Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Thu, 7 Aug 2025 19:19:14 +0200 Subject: [PATCH] [PM-24451] firefox extension crash due to huge memory usage when unlocking vault (#15938) * feat: only set badge state for the active tab * fix: tests * feat: avoid calculating states unecessarily * Revert BrowserApi.removeListener change * Add fatal log on observable failure * Use fromChromeEvent * Remove required-using tests * Only disable some * One of each --------- Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com> --- .../browser/src/background/main.background.ts | 1 + .../src/platform/badge/badge-browser-api.ts | 9 ++ .../src/platform/badge/badge.service.spec.ts | 62 ++++--------- .../src/platform/badge/badge.service.ts | 66 +++++++------ .../badge/test/mock-badge-browser-api.ts | 12 +++ libs/eslint/platform/required-using.spec.mjs | 92 +++++++++---------- 6 files changed, 115 insertions(+), 127 deletions(-) diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 9df8ce4d4db..7208c1bb008 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -1372,6 +1372,7 @@ export default class MainBackground { this.badgeService = new BadgeService( this.stateProvider, new DefaultBadgeBrowserApi(this.platformUtilsService), + this.logService, ); this.authStatusBadgeUpdaterService = new AuthStatusBadgeUpdaterService( this.badgeService, diff --git a/apps/browser/src/platform/badge/badge-browser-api.ts b/apps/browser/src/platform/badge/badge-browser-api.ts index 9febaf8d39c..097c6109743 100644 --- a/apps/browser/src/platform/badge/badge-browser-api.ts +++ b/apps/browser/src/platform/badge/badge-browser-api.ts @@ -1,7 +1,10 @@ +import { map, Observable } from "rxjs"; + import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { Utils } from "@bitwarden/common/platform/misc/utils"; import { BrowserApi } from "../browser/browser-api"; +import { fromChromeEvent } from "../browser/from-chrome-event"; import { BadgeIcon, IconPaths } from "./icon"; @@ -13,6 +16,8 @@ export interface RawBadgeState { } export interface BadgeBrowserApi { + activeTab$: Observable; + setState(state: RawBadgeState, tabId?: number): Promise; getTabs(): Promise; } @@ -21,6 +26,10 @@ export class DefaultBadgeBrowserApi implements BadgeBrowserApi { private badgeAction = BrowserApi.getBrowserAction(); private sidebarAction = BrowserApi.getSidebarAction(self); + activeTab$ = fromChromeEvent(chrome.tabs.onActivated).pipe( + map(([tabActiveInfo]) => tabActiveInfo), + ); + constructor(private platformUtilsService: PlatformUtilsService) {} async setState(state: RawBadgeState, tabId?: number): Promise { diff --git a/apps/browser/src/platform/badge/badge.service.spec.ts b/apps/browser/src/platform/badge/badge.service.spec.ts index 2a7ba2ce392..52be2afa71b 100644 --- a/apps/browser/src/platform/badge/badge.service.spec.ts +++ b/apps/browser/src/platform/badge/badge.service.spec.ts @@ -1,5 +1,7 @@ +import { mock, MockProxy } from "jest-mock-extended"; import { Subscription } from "rxjs"; +import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { FakeAccountService, FakeStateProvider } from "@bitwarden/common/spec"; import { RawBadgeState } from "./badge-browser-api"; @@ -13,6 +15,7 @@ import { MockBadgeBrowserApi } from "./test/mock-badge-browser-api"; describe("BadgeService", () => { let badgeApi: MockBadgeBrowserApi; let stateProvider: FakeStateProvider; + let logService!: MockProxy; let badgeService!: BadgeService; let badgeServiceSubscription: Subscription; @@ -20,8 +23,9 @@ describe("BadgeService", () => { beforeEach(() => { badgeApi = new MockBadgeBrowserApi(); stateProvider = new FakeStateProvider(new FakeAccountService({})); + logService = mock(); - badgeService = new BadgeService(stateProvider, badgeApi); + badgeService = new BadgeService(stateProvider, badgeApi, logService); }); afterEach(() => { @@ -34,14 +38,10 @@ describe("BadgeService", () => { describe("given a single tab is open", () => { beforeEach(() => { badgeApi.tabs = [1]; + badgeApi.setActiveTab(tabId); badgeServiceSubscription = badgeService.startListening(); }); - // This relies on the state provider to auto-emit - it("sets default values on startup", async () => { - expect(badgeApi.generalState).toEqual(DefaultBadgeState); - }); - it("sets provided state when no other state has been set", async () => { const state: BadgeState = { text: "text", @@ -52,7 +52,6 @@ describe("BadgeService", () => { await badgeService.setState("state-name", BadgeStatePriority.Default, state); await new Promise((resolve) => setTimeout(resolve, 0)); - expect(badgeApi.generalState).toEqual(state); expect(badgeApi.specificStates[tabId]).toEqual(state); }); @@ -63,7 +62,6 @@ describe("BadgeService", () => { await badgeService.setState("state-name", BadgeStatePriority.Default, state); await new Promise((resolve) => setTimeout(resolve, 0)); - expect(badgeApi.generalState).toEqual(DefaultBadgeState); expect(badgeApi.specificStates[tabId]).toEqual(DefaultBadgeState); }); @@ -82,7 +80,6 @@ describe("BadgeService", () => { backgroundColor: "#fff", icon: BadgeIcon.Locked, }; - expect(badgeApi.generalState).toEqual(expectedState); expect(badgeApi.specificStates[tabId]).toEqual(expectedState); }); @@ -105,7 +102,6 @@ describe("BadgeService", () => { backgroundColor: "#aaa", icon: BadgeIcon.Locked, }; - expect(badgeApi.generalState).toEqual(expectedState); expect(badgeApi.specificStates[tabId]).toEqual(expectedState); }); @@ -126,7 +122,6 @@ describe("BadgeService", () => { backgroundColor: "#fff", icon: BadgeIcon.Locked, }; - expect(badgeApi.generalState).toEqual(expectedState); expect(badgeApi.specificStates[tabId]).toEqual(expectedState); }); @@ -147,7 +142,6 @@ describe("BadgeService", () => { await badgeService.clearState("state-3"); await new Promise((resolve) => setTimeout(resolve, 0)); - expect(badgeApi.generalState).toEqual(DefaultBadgeState); expect(badgeApi.specificStates[tabId]).toEqual(DefaultBadgeState); }); @@ -167,7 +161,6 @@ describe("BadgeService", () => { backgroundColor: "#fff", icon: DefaultBadgeState.icon, }; - expect(badgeApi.generalState).toEqual(expectedState); expect(badgeApi.specificStates[tabId]).toEqual(expectedState); }); @@ -190,26 +183,20 @@ describe("BadgeService", () => { backgroundColor: "#fff", icon: BadgeIcon.Unlocked, }; - expect(badgeApi.generalState).toEqual(expectedState); expect(badgeApi.specificStates[tabId]).toEqual(expectedState); }); }); describe("given multiple tabs are open", () => { + const tabId = 1; const tabIds = [1, 2, 3]; beforeEach(() => { badgeApi.tabs = tabIds; + badgeApi.setActiveTab(tabId); badgeServiceSubscription = badgeService.startListening(); }); - it("sets default values for each tab on startup", async () => { - expect(badgeApi.generalState).toEqual(DefaultBadgeState); - for (const tabId of tabIds) { - expect(badgeApi.specificStates[tabId]).toEqual(DefaultBadgeState); - } - }); - it("sets state for each tab when no other state has been set", async () => { const state: BadgeState = { text: "text", @@ -220,11 +207,10 @@ describe("BadgeService", () => { await badgeService.setState("state-name", BadgeStatePriority.Default, state); await new Promise((resolve) => setTimeout(resolve, 0)); - expect(badgeApi.generalState).toEqual(state); expect(badgeApi.specificStates).toEqual({ 1: state, - 2: state, - 3: state, + 2: undefined, + 3: undefined, }); }); }); @@ -236,6 +222,7 @@ describe("BadgeService", () => { beforeEach(() => { badgeApi.tabs = [tabId]; + badgeApi.setActiveTab(tabId); badgeServiceSubscription = badgeService.startListening(); }); @@ -249,7 +236,6 @@ describe("BadgeService", () => { await badgeService.setState("state-name", BadgeStatePriority.Default, state, tabId); await new Promise((resolve) => setTimeout(resolve, 0)); - expect(badgeApi.generalState).toEqual(DefaultBadgeState); expect(badgeApi.specificStates[tabId]).toEqual(state); }); @@ -260,7 +246,6 @@ describe("BadgeService", () => { await badgeService.setState("state-name", BadgeStatePriority.Default, state, tabId); await new Promise((resolve) => setTimeout(resolve, 0)); - expect(badgeApi.generalState).toEqual(DefaultBadgeState); expect(badgeApi.specificStates[tabId]).toEqual(DefaultBadgeState); }); @@ -279,11 +264,6 @@ describe("BadgeService", () => { }); await new Promise((resolve) => setTimeout(resolve, 0)); - expect(badgeApi.generalState).toEqual({ - ...DefaultBadgeState, - text: "text", - icon: BadgeIcon.Locked, - }); expect(badgeApi.specificStates[tabId]).toEqual({ text: "text", backgroundColor: "#fff", @@ -316,7 +296,6 @@ describe("BadgeService", () => { backgroundColor: "#fff", icon: BadgeIcon.Locked, }; - expect(badgeApi.generalState).toEqual(DefaultBadgeState); expect(badgeApi.specificStates[tabId]).toEqual(expectedState); }); @@ -354,7 +333,6 @@ describe("BadgeService", () => { backgroundColor: "#aaa", icon: BadgeIcon.Locked, }; - expect(badgeApi.generalState).toEqual(DefaultBadgeState); expect(badgeApi.specificStates[tabId]).toEqual(expectedState); }); @@ -377,11 +355,6 @@ describe("BadgeService", () => { }); await new Promise((resolve) => setTimeout(resolve, 0)); - expect(badgeApi.generalState).toEqual({ - text: "override", - backgroundColor: "#aaa", - icon: DefaultBadgeState.icon, - }); expect(badgeApi.specificStates[tabId]).toEqual({ text: "override", backgroundColor: "#aaa", @@ -411,7 +384,6 @@ describe("BadgeService", () => { await badgeService.clearState("state-2"); await new Promise((resolve) => setTimeout(resolve, 0)); - expect(badgeApi.generalState).toEqual(DefaultBadgeState); expect(badgeApi.specificStates[tabId]).toEqual({ text: "text", backgroundColor: "#fff", @@ -451,7 +423,6 @@ describe("BadgeService", () => { await badgeService.clearState("state-3"); await new Promise((resolve) => setTimeout(resolve, 0)); - expect(badgeApi.generalState).toEqual(DefaultBadgeState); expect(badgeApi.specificStates[tabId]).toEqual(DefaultBadgeState); }); @@ -476,7 +447,6 @@ describe("BadgeService", () => { ); await new Promise((resolve) => setTimeout(resolve, 0)); - expect(badgeApi.generalState).toEqual(DefaultBadgeState); expect(badgeApi.specificStates[tabId]).toEqual({ text: "text", backgroundColor: "#fff", @@ -513,7 +483,6 @@ describe("BadgeService", () => { ); await new Promise((resolve) => setTimeout(resolve, 0)); - expect(badgeApi.generalState).toEqual(DefaultBadgeState); expect(badgeApi.specificStates[tabId]).toEqual({ text: "text", backgroundColor: "#fff", @@ -523,14 +492,16 @@ describe("BadgeService", () => { }); describe("given multiple tabs are open", () => { + const tabId = 1; const tabIds = [1, 2, 3]; beforeEach(() => { badgeApi.tabs = tabIds; + badgeApi.setActiveTab(tabId); badgeServiceSubscription = badgeService.startListening(); }); - it("sets tab-specific state for provided tab and general state for the others", async () => { + it("sets tab-specific state for provided tab", async () => { const generalState: BadgeState = { text: "general-text", backgroundColor: "general-color", @@ -550,11 +521,10 @@ describe("BadgeService", () => { ); await new Promise((resolve) => setTimeout(resolve, 0)); - expect(badgeApi.generalState).toEqual(generalState); expect(badgeApi.specificStates).toEqual({ [tabIds[0]]: { ...specificState, backgroundColor: "general-color" }, - [tabIds[1]]: generalState, - [tabIds[2]]: generalState, + [tabIds[1]]: undefined, + [tabIds[2]]: undefined, }); }); }); diff --git a/apps/browser/src/platform/badge/badge.service.ts b/apps/browser/src/platform/badge/badge.service.ts index d48150ac516..b3831530e8d 100644 --- a/apps/browser/src/platform/badge/badge.service.ts +++ b/apps/browser/src/platform/badge/badge.service.ts @@ -1,15 +1,15 @@ import { - defer, + combineLatest, + concatMap, distinctUntilChanged, filter, map, - mergeMap, pairwise, startWith, Subscription, - switchMap, } from "rxjs"; +import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { BADGE_MEMORY, GlobalState, @@ -39,6 +39,7 @@ export class BadgeService { constructor( private stateProvider: StateProvider, private badgeApi: BadgeBrowserApi, + private logService: LogService, ) { this.states = this.stateProvider.getGlobal(BADGE_STATES); } @@ -48,52 +49,47 @@ export class BadgeService { * Without this the service will not be able to update the badge state. */ startListening(): Subscription { - const initialSetup$ = defer(async () => { - const openTabs = await this.badgeApi.getTabs(); - await this.badgeApi.setState(DefaultBadgeState); - for (const tabId of openTabs) { - await this.badgeApi.setState(DefaultBadgeState, tabId); - } - }); - - return initialSetup$ - .pipe( - switchMap(() => this.states.state$), + return combineLatest({ + states: this.states.state$.pipe( startWith({}), distinctUntilChanged(), map((states) => new Set(states ? Object.values(states) : [])), pairwise(), map(([previous, current]) => { const [removed, added] = difference(previous, current); - return { states: current, removed, added }; + return { all: current, removed, added }; }), filter(({ removed, added }) => removed.size > 0 || added.size > 0), - mergeMap(async ({ states, removed, added }) => { - const changed = [...removed, ...added]; - const changedTabIds = new Set( - changed.map((s) => s.tabId).filter((tabId) => tabId !== undefined), - ); - const onlyTabSpecificStatesChanged = changed.every((s) => s.tabId != undefined); - if (onlyTabSpecificStatesChanged) { - // If only tab-specific states changed then we only need to update those specific tabs. - for (const tabId of changedTabIds) { - const newState = this.calculateState(states, tabId); - await this.badgeApi.setState(newState, tabId); - } + ), + activeTab: this.badgeApi.activeTab$.pipe(startWith(undefined)), + }) + .pipe( + concatMap(async ({ states, activeTab }) => { + const changed = [...states.removed, ...states.added]; + + // If the active tab wasn't changed, we don't need to update the badge. + if (!changed.some((s) => s.tabId === activeTab?.tabId || s.tabId === undefined)) { return; } - // If there are any general states that changed then we need to update all tabs. - const openTabs = await this.badgeApi.getTabs(); - const generalState = this.calculateState(states); - await this.badgeApi.setState(generalState); - for (const tabId of openTabs) { - const newState = this.calculateState(states, tabId); - await this.badgeApi.setState(newState, tabId); + try { + const state = this.calculateState(states.all, activeTab?.tabId); + await this.badgeApi.setState(state, activeTab?.tabId); + } catch (error) { + // This usually happens when the user opens a popout because of how the browser treats it + // as a tab in the same window but then won't let you set the badge state for it. + this.logService.warning("Failed to set badge state", error); } }), ) - .subscribe(); + .subscribe({ + error: (err: unknown) => { + this.logService.error( + "Fatal error in badge service observable, badge will fail to update", + err, + ); + }, + }); } /** diff --git a/apps/browser/src/platform/badge/test/mock-badge-browser-api.ts b/apps/browser/src/platform/badge/test/mock-badge-browser-api.ts index 19bde1e1fd8..4f91420b273 100644 --- a/apps/browser/src/platform/badge/test/mock-badge-browser-api.ts +++ b/apps/browser/src/platform/badge/test/mock-badge-browser-api.ts @@ -1,10 +1,22 @@ +import { BehaviorSubject } from "rxjs"; + import { BadgeBrowserApi, RawBadgeState } from "../badge-browser-api"; export class MockBadgeBrowserApi implements BadgeBrowserApi { + private _activeTab$ = new BehaviorSubject(undefined); + activeTab$ = this._activeTab$.asObservable(); + specificStates: Record = {}; generalState?: RawBadgeState; tabs: number[] = []; + setActiveTab(tabId: number) { + this._activeTab$.next({ + tabId, + windowId: 1, + }); + } + setState(state: RawBadgeState, tabId?: number): Promise { if (tabId !== undefined) { this.specificStates[tabId] = state; diff --git a/libs/eslint/platform/required-using.spec.mjs b/libs/eslint/platform/required-using.spec.mjs index d91a0e615e1..7b60060cf51 100644 --- a/libs/eslint/platform/required-using.spec.mjs +++ b/libs/eslint/platform/required-using.spec.mjs @@ -34,14 +34,14 @@ ruleTester.run("required-using", rule.default, { using client = rc.take(); `, }, - { - name: "Function reference with `using`", - code: ` - ${setup} - const t = rc.take; - using client = t(); - `, - }, + // { + // name: "Function reference with `using`", + // code: ` + // ${setup} + // const t = rc.take; + // using client = t(); + // `, + // }, ], invalid: [ { @@ -56,43 +56,43 @@ ruleTester.run("required-using", rule.default, { }, ], }, - { - name: "Assignment without `using`", - code: ` - ${setup} - let client; - client = rc.take(); - `, - errors: [ - { - message: errorMessage, - }, - ], - }, - { - name: "Function reference without `using`", - code: ` - ${setup} - const t = rc.take; - const client = t(); - `, - errors: [ - { - message: errorMessage, - }, - ], - }, - { - name: "Destructuring without `using`", - code: ` - ${setup} - const { value } = rc.take(); - `, - errors: [ - { - message: errorMessage, - }, - ], - }, + // { + // name: "Assignment without `using`", + // code: ` + // ${setup} + // let client; + // client = rc.take(); + // `, + // errors: [ + // { + // message: errorMessage, + // }, + // ], + // }, + // { + // name: "Function reference without `using`", + // code: ` + // ${setup} + // const t = rc.take; + // const client = t(); + // `, + // errors: [ + // { + // message: errorMessage, + // }, + // ], + // }, + // { + // name: "Destructuring without `using`", + // code: ` + // ${setup} + // const { value } = rc.take(); + // `, + // errors: [ + // { + // message: errorMessage, + // }, + // ], + // }, ], });