Remove ctor initialization in session sync (#4755)

* Remove ctor initialization in session sync

* Fix error message

* Prefer messaging over storage for syncing

We still need to use storage for instances where we open a popup or
worker and need to populate from a cache. However, in MV2, this is only
ever stored in a background service, ensuring that all data is stored in
from a long-lived context (mv2) or serialized to storage (mv3).

* Test new storage scheme

apps/browser/src/browser/browserApi.ts

@@ -115,6 +115,10 @@ export class BrowserApi {
     return chrome.extension.getBackgroundPage();
   }
 
+  static isBackgroundPage(window: Window & typeof globalThis): boolean {
+    return window === chrome.extension.getBackgroundPage();
+  }
+
   static getApplicationVersion(): string {
     return chrome.runtime.getManifest().version;
   }

apps/browser/src/decorators/session-sync-observable/session-sync.decorator.spec.ts

@@ -4,11 +4,10 @@ import { sessionSync } from "./session-sync.decorator";
 
 describe("sessionSync decorator", () => {
   const initializer = (s: string) => "test";
-  const ctor = String;
   class TestClass {
-    @sessionSync({ ctor: ctor, initializer: initializer })
+    @sessionSync({ initializer: initializer })
     private testProperty = new BehaviorSubject("");
-    @sessionSync({ ctor: ctor, initializer: initializer, initializeAs: "array" })
+    @sessionSync({ initializer: initializer, initializeAs: "array" })
     private secondTestProperty = new BehaviorSubject("");
 
     complete() {
@@ -23,13 +22,11 @@ describe("sessionSync decorator", () => {
       expect.objectContaining({
         propertyKey: "testProperty",
         sessionKey: "testProperty_0",
-        ctor: ctor,
         initializer: initializer,
       }),
       expect.objectContaining({
         propertyKey: "secondTestProperty",
         sessionKey: "secondTestProperty_1",
-        ctor: ctor,
         initializer: initializer,
         initializeAs: "array",
       }),
@@ -38,7 +35,7 @@ describe("sessionSync decorator", () => {
   });
 
   class TestClass2 {
-    @sessionSync({ ctor: ctor, initializer: initializer })
+    @sessionSync({ initializer: initializer })
     private testProperty = new BehaviorSubject("");
 
     complete() {
@@ -52,7 +49,6 @@ describe("sessionSync decorator", () => {
       expect.objectContaining({
         propertyKey: "testProperty",
         sessionKey: "testProperty_2",
-        ctor: ctor,
         initializer: initializer,
       }),
     ]);

apps/browser/src/decorators/session-sync-observable/session-sync.decorator.ts

@@ -4,7 +4,6 @@ import { SessionStorable } from "./session-storable";
 import { InitializeOptions } from "./sync-item-metadata";
 
 class BuildOptions<T, TJson = Jsonify<T>> {
-  ctor?: new () => T;
   initializer?: (keyValuePair: TJson) => T;
   initializeAs?: InitializeOptions;
 }
@@ -48,7 +47,6 @@ export function sessionSync<T>(buildOptions: BuildOptions<T>) {
     p.__syncedItemMetadata.push({
       propertyKey,
       sessionKey: `${propertyKey}_${index++}`,
-      ctor: buildOptions.ctor,
       initializer: buildOptions.initializer,
       initializeAs: buildOptions.initializeAs ?? "object",
     });

apps/browser/src/decorators/session-sync-observable/session-syncer.spec.ts

@@ -53,8 +53,8 @@ describe("session syncer", () => {
         new SessionSyncer(behaviorSubject, storageService, {
           propertyKey,
           sessionKey,
-          ctor: String,
           initializeAs: "object",
+          initializer: () => null,
         })
       ).toBeDefined();
       expect(
@@ -72,8 +72,9 @@ describe("session syncer", () => {
           propertyKey,
           sessionKey,
           initializeAs: "object",
+          initializer: null,
         });
-      }).toThrowError("ctor or initializer must be provided");
+      }).toThrowError("initializer must be provided");
     });
   });
 
@@ -106,7 +107,7 @@ describe("session syncer", () => {
     it("should grab an initial value from storage if it exists", async () => {
       storageService.has.mockResolvedValue(true);
       //Block a call to update
-      const updateSpy = jest.spyOn(sut as any, "update").mockImplementation();
+      const updateSpy = jest.spyOn(sut as any, "updateFromMemory").mockImplementation();
 
       sut.init();
       await awaitAsync();
@@ -128,20 +129,15 @@ describe("session syncer", () => {
 
   describe("a value is emitted on the observable", () => {
     let sendMessageSpy: jest.SpyInstance;
+    const value = "test";
+    const serializedValue = JSON.stringify(value);
 
     beforeEach(() => {
       sendMessageSpy = jest.spyOn(BrowserApi, "sendMessage");
 
       sut.init();
 
-      behaviorSubject.next("test");
+      behaviorSubject.next(value);
-    });
-
-    it("should update the session memory", async () => {
-      // await finishing of fire-and-forget operation
-      await new Promise((resolve) => setTimeout(resolve, 100));
-      expect(storageService.save).toHaveBeenCalledTimes(1);
-      expect(storageService.save).toHaveBeenCalledWith(sessionKey, "test");
     });
 
     it("should update sessionSyncers in other contexts", async () => {
@@ -149,7 +145,10 @@ describe("session syncer", () => {
       await new Promise((resolve) => setTimeout(resolve, 100));
 
       expect(sendMessageSpy).toHaveBeenCalledTimes(1);
-      expect(sendMessageSpy).toHaveBeenCalledWith(`${sessionKey}_update`, { id: sut.id });
+      expect(sendMessageSpy).toHaveBeenCalledWith(`${sessionKey}_update`, {
+        id: sut.id,
+        serializedValue,
+      });
     });
   });
 
@@ -185,26 +184,104 @@ describe("session syncer", () => {
     it("should update from message on emit from another instance", async () => {
       const builder = jest.fn();
       jest.spyOn(SyncedItemMetadata, "builder").mockReturnValue(builder);
-      storageService.getBypassCache.mockResolvedValue("test");
+      const value = "test";
+      const serializedValue = JSON.stringify(value);
+      builder.mockReturnValue(value);
 
       // Expect no circular messaging
       await awaitAsync();
       expect(sendMessageSpy).toHaveBeenCalledTimes(0);
 
-      await sut.updateFromMessage({ command: `${sessionKey}_update`, id: "different_id" });
+      await sut.updateFromMessage({
+        command: `${sessionKey}_update`,
+        id: "different_id",
+        serializedValue,
+      });
       await awaitAsync();
 
-      expect(storageService.getBypassCache).toHaveBeenCalledTimes(1);
+      expect(storageService.getBypassCache).toHaveBeenCalledTimes(0);
-      expect(storageService.getBypassCache).toHaveBeenCalledWith(sessionKey, {
-        deserializer: builder,
-      });
 
       expect(nextSpy).toHaveBeenCalledTimes(1);
-      expect(nextSpy).toHaveBeenCalledWith("test");
+      expect(nextSpy).toHaveBeenCalledWith(value);
-      expect(behaviorSubject.value).toBe("test");
+      expect(behaviorSubject.value).toBe(value);
 
       // Expect no circular messaging
       expect(sendMessageSpy).toHaveBeenCalledTimes(0);
     });
   });
+
+  describe("memory storage", () => {
+    const value = "test";
+    const serializedValue = JSON.stringify(value);
+    let saveSpy: jest.SpyInstance;
+    const builder = jest.fn().mockReturnValue(value);
+    const manifestVersionSpy = jest.spyOn(BrowserApi, "manifestVersion", "get");
+    const isBackgroundPageSpy = jest.spyOn(BrowserApi, "isBackgroundPage");
+
+    beforeEach(async () => {
+      jest.spyOn(SyncedItemMetadata, "builder").mockReturnValue(builder);
+      saveSpy = jest.spyOn(storageService, "save");
+
+      sut.init();
+      await awaitAsync();
+    });
+
+    afterEach(() => {
+      jest.resetAllMocks();
+    });
+
+    it("should always store on observed next for manifest version 3", async () => {
+      manifestVersionSpy.mockReturnValue(3);
+      isBackgroundPageSpy.mockReturnValueOnce(true).mockReturnValueOnce(false);
+      behaviorSubject.next(value);
+      await awaitAsync();
+      behaviorSubject.next(value);
+      await awaitAsync();
+
+      expect(saveSpy).toHaveBeenCalledTimes(2);
+    });
+
+    it("should not store on message receive for manifest version 3", async () => {
+      manifestVersionSpy.mockReturnValue(3);
+      isBackgroundPageSpy.mockReturnValueOnce(true).mockReturnValueOnce(false);
+      await sut.updateFromMessage({
+        command: `${sessionKey}_update`,
+        id: "different_id",
+        serializedValue,
+      });
+      await awaitAsync();
+
+      expect(saveSpy).toHaveBeenCalledTimes(0);
+    });
+
+    it("should store on message receive for manifest version 2 for background page only", async () => {
+      manifestVersionSpy.mockReturnValue(2);
+      isBackgroundPageSpy.mockReturnValueOnce(true).mockReturnValueOnce(false);
+      await sut.updateFromMessage({
+        command: `${sessionKey}_update`,
+        id: "different_id",
+        serializedValue,
+      });
+      await awaitAsync();
+      await sut.updateFromMessage({
+        command: `${sessionKey}_update`,
+        id: "different_id",
+        serializedValue,
+      });
+      await awaitAsync();
+
+      expect(saveSpy).toHaveBeenCalledTimes(1);
+    });
+
+    it("should store on observed next for manifest version 2 for background page only", async () => {
+      manifestVersionSpy.mockReturnValue(2);
+      isBackgroundPageSpy.mockReturnValueOnce(true).mockReturnValueOnce(false);
+      behaviorSubject.next(value);
+      await awaitAsync();
+      behaviorSubject.next(value);
+      await awaitAsync();
+
+      expect(saveSpy).toHaveBeenCalledTimes(1);
+    });
+  });
 });

apps/browser/src/decorators/session-sync-observable/session-syncer.ts

@@ -23,8 +23,8 @@ export class SessionSyncer {
       throw new Error("subject must inherit from Subject");
     }
 
-    if (metaData.ctor == null && metaData.initializer == null) {
+    if (metaData.initializer == null) {
-      throw new Error("ctor or initializer must be provided");
+      throw new Error("initializer must be provided");
     }
   }
 
@@ -45,7 +45,7 @@ export class SessionSyncer {
     // must be synchronous
     const hasInSessionMemory = await this.memoryStorageService.has(this.metaData.sessionKey);
     if (hasInSessionMemory) {
-      await this.update();
+      await this.updateFromMemory();
     }
 
     this.listenForUpdates();
@@ -83,21 +83,31 @@ export class SessionSyncer {
     if (message.command != this.updateMessageCommand || message.id === this.id) {
       return;
     }
-    this.update();
+    await this.update(message.serializedValue);
   }
 
-  async update() {
+  async updateFromMemory() {
+    const value = await this.memoryStorageService.getBypassCache(this.metaData.sessionKey);
+    await this.update(value);
+  }
+
+  async update(serializedValue: any) {
+    const unBuiltValue = JSON.parse(serializedValue);
+    if (BrowserApi.manifestVersion !== 3 && BrowserApi.isBackgroundPage(self)) {
+      await this.memoryStorageService.save(this.metaData.sessionKey, serializedValue);
+    }
     const builder = SyncedItemMetadata.builder(this.metaData);
-    const value = await this.memoryStorageService.getBypassCache(this.metaData.sessionKey, {
+    const value = builder(unBuiltValue);
-      deserializer: builder,
-    });
     this.ignoreNUpdates = 1;
     this.subject.next(value);
   }
 
   private async updateSession(value: any) {
-    await this.memoryStorageService.save(this.metaData.sessionKey, value);
+    const serializedValue = JSON.stringify(value);
-    await BrowserApi.sendMessage(this.updateMessageCommand, { id: this.id });
+    if (BrowserApi.manifestVersion === 3 || BrowserApi.isBackgroundPage(self)) {
+      await this.memoryStorageService.save(this.metaData.sessionKey, serializedValue);
+    }
+    await BrowserApi.sendMessage(this.updateMessageCommand, { id: this.id, serializedValue });
   }
 
   private get updateMessageCommand() {

apps/browser/src/decorators/session-sync-observable/sync-item-metadata.ts

@@ -3,15 +3,11 @@ export type InitializeOptions = "array" | "record" | "object";
 export class SyncedItemMetadata {
   propertyKey: string;
   sessionKey: string;
-  ctor?: new () => any;
+  initializer: (keyValuePair: any) => any;
-  initializer?: (keyValuePair: any) => any;
   initializeAs: InitializeOptions;
 
   static builder(metadata: SyncedItemMetadata): (o: any) => any {
-    const itemBuilder =
+    const itemBuilder = metadata.initializer;
-      metadata.initializer != null
-        ? metadata.initializer
-        : (o: any) => Object.assign(new metadata.ctor(), o);
     if (metadata.initializeAs === "array") {
       return (keyValuePair: any) => keyValuePair.map((o: any) => itemBuilder(o));
     } else if (metadata.initializeAs === "record") {

apps/browser/src/decorators/session-sync-observable/synced-item-metadata.spec.ts

@@ -4,10 +4,8 @@ describe("builder", () => {
   const propertyKey = "propertyKey";
   const key = "key";
   const initializer = (s: any) => "used initializer";
-  class TestClass {}
-  const ctor = TestClass;
 
-  it("should use initializer if provided", () => {
+  it("should use initializer", () => {
     const metadata: SyncedItemMetadata = {
       propertyKey,
       sessionKey: key,
@@ -18,29 +16,6 @@ describe("builder", () => {
     expect(builder({})).toBe("used initializer");
   });
 
-  it("should use ctor if initializer is not provided", () => {
-    const metadata: SyncedItemMetadata = {
-      propertyKey,
-      sessionKey: key,
-      ctor,
-      initializeAs: "object",
-    };
-    const builder = SyncedItemMetadata.builder(metadata);
-    expect(builder({})).toBeInstanceOf(TestClass);
-  });
-
-  it("should prefer initializer over ctor", () => {
-    const metadata: SyncedItemMetadata = {
-      propertyKey,
-      sessionKey: key,
-      ctor,
-      initializer,
-      initializeAs: "object",
-    };
-    const builder = SyncedItemMetadata.builder(metadata);
-    expect(builder({})).toBe("used initializer");
-  });
-
   it("should honor initialize as array", () => {
     const metadata: SyncedItemMetadata = {
       propertyKey,

apps/browser/src/services/browser-i18n.service.ts

@@ -8,7 +8,7 @@ import I18nService from "./i18n.service";
 
 @browserSession
 export class BrowserI18nService extends I18nService {
-  @sessionSync({ ctor: String })
+  @sessionSync({ initializer: (s: string) => s })
   protected _locale: ReplaySubject<string>;
 
   constructor(systemLanguage: string, private stateService: StateService) {

apps/browser/src/services/browser-policy.service.ts

@@ -1,4 +1,5 @@
 import { BehaviorSubject } from "rxjs";
+import { Jsonify } from "type-fest";
 
 import { Policy } from "@bitwarden/common/models/domain/policy";
 import { PolicyService } from "@bitwarden/common/services/policy/policy.service";
@@ -7,6 +8,9 @@ import { browserSession, sessionSync } from "../decorators/session-sync-observab
 
 @browserSession
 export class BrowserPolicyService extends PolicyService {
-  @sessionSync({ ctor: Policy, initializeAs: "array" })
+  @sessionSync({
+    initializer: (obj: Jsonify<Policy>) => Object.assign(new Policy(), obj),
+    initializeAs: "array",
+  })
   protected _policies: BehaviorSubject<Policy[]>;
 }

apps/browser/src/services/browser-state.service.ts

@@ -22,9 +22,9 @@ export class BrowserStateService
     initializeAs: "record",
   })
   protected accountsSubject: BehaviorSubject<{ [userId: string]: Account }>;
-  @sessionSync({ ctor: String })
+  @sessionSync({ initializer: (s: string) => s })
   protected activeAccountSubject: BehaviorSubject<string>;
-  @sessionSync({ ctor: Boolean })
+  @sessionSync({ initializer: (b: boolean) => b })
   protected activeAccountUnlockedSubject: BehaviorSubject<boolean>;
   @sessionSync({
     initializer: Account.fromJSON as any, // TODO: Remove this any when all any types are removed from Account