From 5b69d52f027ccc2f86d0ef33288194080df1fa43 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Garc=C3=ADa?=
 <dani-garcia@users.noreply.github.com>
Date: Thu, 21 Sep 2023 07:04:17 -0600
Subject: [PATCH] Ensure chrome.storage listeners also get cleaned up in Safari
 to avoid memory leak (#6354)

---
 .../src/platform/browser/browser-api.ts       | 40 ++++++++++++++-----
 .../services/browser-state.service.ts         |  3 +-
 2 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/apps/browser/src/platform/browser/browser-api.ts b/apps/browser/src/platform/browser/browser-api.ts
index 5a5596a795..b71b8b80b6 100644
--- a/apps/browser/src/platform/browser/browser-api.ts
+++ b/apps/browser/src/platform/browser/browser-api.ts
@@ -199,7 +199,10 @@ export class BrowserApi {
     BrowserApi.removeTab(tabToClose.id);
   }
 
+  // Keep track of all the events registered in a Safari popup so we can remove
+  // them when the popup gets unloaded, otherwise we cause a memory leak
   private static registeredMessageListeners: any[] = [];
+  private static registeredStorageChangeListeners: any[] = [];
 
   static messageListener(
     name: string,
@@ -207,21 +210,38 @@ export class BrowserApi {
   ) {
     chrome.runtime.onMessage.addListener(callback);
 
-    // Keep track of all the events registered in a Safari popup so we can remove
-    // them when the popup gets unloaded, otherwise we cause a memory leak
     if (BrowserApi.isSafariApi && !BrowserApi.isBackgroundPage(window)) {
       BrowserApi.registeredMessageListeners.push(callback);
-
-      // The MDN recommend using 'visibilitychange' but that event is fired any time the popup window is obscured as well
-      // 'pagehide' works just like 'unload' but is compatible with the back/forward cache, so we prefer using that one
-      window.onpagehide = () => {
-        for (const callback of BrowserApi.registeredMessageListeners) {
-          chrome.runtime.onMessage.removeListener(callback);
-        }
-      };
+      BrowserApi.setupUnloadListeners();
     }
   }
 
+  static storageChangeListener(
+    callback: Parameters<typeof chrome.storage.onChanged.addListener>[0]
+  ) {
+    chrome.storage.onChanged.addListener(callback);
+
+    if (BrowserApi.isSafariApi && !BrowserApi.isBackgroundPage(window)) {
+      BrowserApi.registeredStorageChangeListeners.push(callback);
+      BrowserApi.setupUnloadListeners();
+    }
+  }
+
+  // Setup the event to destroy all the listeners when the popup gets unloaded in Safari, otherwise we get a memory leak
+  private static setupUnloadListeners() {
+    // The MDN recommend using 'visibilitychange' but that event is fired any time the popup window is obscured as well
+    // 'pagehide' works just like 'unload' but is compatible with the back/forward cache, so we prefer using that one
+    window.onpagehide = () => {
+      for (const callback of BrowserApi.registeredMessageListeners) {
+        chrome.runtime.onMessage.removeListener(callback);
+      }
+
+      for (const callback of BrowserApi.registeredStorageChangeListeners) {
+        chrome.storage.onChanged.removeListener(callback);
+      }
+    };
+  }
+
   static sendMessage(subscriber: string, arg: any = {}) {
     const message = Object.assign({}, { command: subscriber }, arg);
     return chrome.runtime.sendMessage(message);
diff --git a/apps/browser/src/platform/services/browser-state.service.ts b/apps/browser/src/platform/services/browser-state.service.ts
index 5e356e7fbe..ec6851beb8 100644
--- a/apps/browser/src/platform/services/browser-state.service.ts
+++ b/apps/browser/src/platform/services/browser-state.service.ts
@@ -14,6 +14,7 @@ import { Account } from "../../models/account";
 import { BrowserComponentState } from "../../models/browserComponentState";
 import { BrowserGroupingsComponentState } from "../../models/browserGroupingsComponentState";
 import { BrowserSendComponentState } from "../../models/browserSendComponentState";
+import { BrowserApi } from "../browser/browser-api";
 import { browserSession, sessionSync } from "../decorators/session-sync-observable";
 
 import { BrowserStateService as StateServiceAbstraction } from "./abstractions/browser-state.service";
@@ -56,7 +57,7 @@ export class BrowserStateService
     // the background page that can get out of sync. We need to work out the
     // best way to handle caching with multiple instances of the state service.
     if (useAccountCache) {
-      chrome.storage.onChanged.addListener((changes, namespace) => {
+      BrowserApi.storageChangeListener((changes, namespace) => {
         if (namespace === "local") {
           for (const key of Object.keys(changes)) {
             if (key !== "accountActivity" && this.accountDiskCache.value[key]) {