Skip to content

Commit

Permalink
[PM-7809] Fix memory leak in AngularThemingService for Safari extensi…
Browse files Browse the repository at this point in the history
…on (#9434)

* [PM-7809] Fix memory leak in AngularThemingService for Safari

* Use getSystemThemeFromWindow in createSystemThemeFromWindow
  • Loading branch information
dani-garcia authored Jun 28, 2024
1 parent 4e3fb99 commit 3c7663a
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 11 deletions.
13 changes: 7 additions & 6 deletions apps/browser/src/popup/services/services.module.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { APP_INITIALIZER, NgModule, NgZone } from "@angular/core";
import { Router } from "@angular/router";
import { Subject, merge } from "rxjs";
import { Subject, merge, of } from "rxjs";

import { UnauthGuard as BaseUnauthGuardService } from "@bitwarden/angular/auth/guards";
import { AngularThemingService } from "@bitwarden/angular/platform/services/theming/angular-theming.service";
Expand Down Expand Up @@ -485,14 +485,15 @@ const safeProviders: SafeProvider[] = [
provide: SYSTEM_THEME_OBSERVABLE,
useFactory: (platformUtilsService: PlatformUtilsService) => {
// Safari doesn't properly handle the (prefers-color-scheme) media query in the popup window, it always returns light.
// In Safari, we have to use the background page instead, which comes with limitations like not dynamically changing the extension theme when the system theme is changed.
let windowContext = window;
// This means we have to use the background page instead, which comes with limitations like not dynamically
// changing the extension theme when the system theme is changed. We also have issues with memory leaks when
// holding the reference to the background page.
const backgroundWindow = BrowserApi.getBackgroundPage();
if (platformUtilsService.isSafari() && backgroundWindow) {
windowContext = backgroundWindow;
return of(AngularThemingService.getSystemThemeFromWindow(backgroundWindow));
} else {
return AngularThemingService.createSystemThemeFromWindow(window);
}

return AngularThemingService.createSystemThemeFromWindow(windowContext);
},
deps: [PlatformUtilsService],
}),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,7 @@ export class AngularThemingService implements AbstractThemingService {
static createSystemThemeFromWindow(window: Window): Observable<ThemeType> {
return merge(
// This observable should always emit at least once, so go and get the current system theme designation
of(
window.matchMedia("(prefers-color-scheme: dark)").matches
? ThemeType.Dark
: ThemeType.Light,
),
of(AngularThemingService.getSystemThemeFromWindow(window)),
// Start listening to changes
fromEvent<MediaQueryListEvent>(
window.matchMedia("(prefers-color-scheme: dark)"),
Expand All @@ -31,6 +27,17 @@ export class AngularThemingService implements AbstractThemingService {
);
}

/**
* Gets the currently active system theme based on the given window.
* @param window The window to query for the current theme.
* @returns The active system theme.
*/
static getSystemThemeFromWindow(window: Window): ThemeType {
return window.matchMedia("(prefers-color-scheme: dark)").matches
? ThemeType.Dark
: ThemeType.Light;
}

readonly theme$ = this.themeStateService.selectedTheme$.pipe(
switchMap((configuredTheme) => {
if (configuredTheme === ThemeType.System) {
Expand Down

0 comments on commit 3c7663a

Please sign in to comment.