From ee2f96d3c498714a89d99e6c8c92b609e2cbf149 Mon Sep 17 00:00:00 2001 From: Matt Gibson Date: Thu, 2 May 2024 03:10:06 -0400 Subject: [PATCH] Use a service to track when to open and close offscreen document (#8977) * Use a service to track when to open and close offscreen document There some strangeness around maintaining the offscreen document for more callbacks, that need not have the same reasons and justifications as the original. We'd need to test, but perhaps the intent is something closer to maintaining a work queue ourselves and creating the offscreen page for only a single reason as it comes in, then waiting for that page to close before opening another. * Prefer builtin promise flattening * Await anything and everything --------- Co-authored-by: Cesar Gonzalez --- .../browser/src/background/main.background.ts | 6 ++ .../platform-utils-service.factory.ts | 1 + .../src/platform/browser/browser-api.spec.ts | 26 ----- .../src/platform/browser/browser-api.ts | 28 ----- .../abstractions/offscreen-document.ts | 18 ++-- .../offscreen-document.service.spec.ts | 101 ++++++++++++++++++ .../offscreen-document.service.ts | 41 +++++++ .../background-platform-utils.service.ts | 5 +- .../browser-platform-utils.service.spec.ts | 42 +++++--- .../browser-platform-utils.service.ts | 16 +-- .../foreground-platform-utils.service.ts | 5 +- .../src/popup/services/services.module.ts | 15 ++- 12 files changed, 218 insertions(+), 86 deletions(-) create mode 100644 apps/browser/src/platform/offscreen-document/offscreen-document.service.spec.ts create mode 100644 apps/browser/src/platform/offscreen-document/offscreen-document.service.ts diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 74b21204bed..f86a44bc8f9 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -212,6 +212,8 @@ import { UpdateBadge } from "../platform/listeners/update-badge"; /* eslint-disable no-restricted-imports */ import { ChromeMessageSender } from "../platform/messaging/chrome-message.sender"; /* eslint-enable no-restricted-imports */ +import { OffscreenDocumentService } from "../platform/offscreen-document/abstractions/offscreen-document"; +import { DefaultOffscreenDocumentService } from "../platform/offscreen-document/offscreen-document.service"; import { BrowserStateService as StateServiceAbstraction } from "../platform/services/abstractions/browser-state.service"; import { BrowserCryptoService } from "../platform/services/browser-crypto.service"; import { BrowserEnvironmentService } from "../platform/services/browser-environment.service"; @@ -336,6 +338,7 @@ export default class MainBackground { userAutoUnlockKeyService: UserAutoUnlockKeyService; scriptInjectorService: BrowserScriptInjectorService; kdfConfigService: kdfConfigServiceAbstraction; + offscreenDocumentService: OffscreenDocumentService; onUpdatedRan: boolean; onReplacedRan: boolean; @@ -393,11 +396,14 @@ export default class MainBackground { ), ); + this.offscreenDocumentService = new DefaultOffscreenDocumentService(); + this.platformUtilsService = new BackgroundPlatformUtilsService( this.messagingService, (clipboardValue, clearMs) => this.clearClipboard(clipboardValue, clearMs), async () => this.biometricUnlock(), self, + this.offscreenDocumentService, ); // Creates a session key for mv3 storage of large memory items diff --git a/apps/browser/src/platform/background/service-factories/platform-utils-service.factory.ts b/apps/browser/src/platform/background/service-factories/platform-utils-service.factory.ts index 6f46d87418d..2cd34ba412b 100644 --- a/apps/browser/src/platform/background/service-factories/platform-utils-service.factory.ts +++ b/apps/browser/src/platform/background/service-factories/platform-utils-service.factory.ts @@ -30,6 +30,7 @@ export function platformUtilsServiceFactory( opts.platformUtilsServiceOptions.clipboardWriteCallback, opts.platformUtilsServiceOptions.biometricCallback, opts.platformUtilsServiceOptions.win, + null, ), ); } diff --git a/apps/browser/src/platform/browser/browser-api.spec.ts b/apps/browser/src/platform/browser/browser-api.spec.ts index e452d6d8ee9..7e0c61c9d17 100644 --- a/apps/browser/src/platform/browser/browser-api.spec.ts +++ b/apps/browser/src/platform/browser/browser-api.spec.ts @@ -525,32 +525,6 @@ describe("BrowserApi", () => { }); }); - describe("createOffscreenDocument", () => { - it("creates the offscreen document with the supplied reasons and justification", async () => { - const reasons = [chrome.offscreen.Reason.CLIPBOARD]; - const justification = "justification"; - - await BrowserApi.createOffscreenDocument(reasons, justification); - - expect(chrome.offscreen.createDocument).toHaveBeenCalledWith({ - url: "offscreen-document/index.html", - reasons, - justification, - }); - }); - }); - - describe("closeOffscreenDocument", () => { - it("closes the offscreen document", () => { - const callbackMock = jest.fn(); - - BrowserApi.closeOffscreenDocument(callbackMock); - - expect(chrome.offscreen.closeDocument).toHaveBeenCalled(); - expect(callbackMock).toHaveBeenCalled(); - }); - }); - describe("registerContentScriptsMv2", () => { const details: browser.contentScripts.RegisteredContentScriptOptions = { matches: [""], diff --git a/apps/browser/src/platform/browser/browser-api.ts b/apps/browser/src/platform/browser/browser-api.ts index e804cf2b8d4..d0695d53fd1 100644 --- a/apps/browser/src/platform/browser/browser-api.ts +++ b/apps/browser/src/platform/browser/browser-api.ts @@ -558,34 +558,6 @@ export class BrowserApi { chrome.privacy.services.passwordSavingEnabled.set({ value }); } - /** - * Opens the offscreen document with the given reasons and justification. - * - * @param reasons - List of reasons for opening the offscreen document. - * @see https://developer.chrome.com/docs/extensions/reference/api/offscreen#type-Reason - * @param justification - Custom written justification for opening the offscreen document. - */ - static async createOffscreenDocument(reasons: chrome.offscreen.Reason[], justification: string) { - await chrome.offscreen.createDocument({ - url: "offscreen-document/index.html", - reasons, - justification, - }); - } - - /** - * Closes the offscreen document. - * - * @param callback - Optional callback to execute after the offscreen document is closed. - */ - static closeOffscreenDocument(callback?: () => void) { - chrome.offscreen.closeDocument(() => { - if (callback) { - callback(); - } - }); - } - /** * Handles registration of static content scripts within manifest v2. * diff --git a/apps/browser/src/platform/offscreen-document/abstractions/offscreen-document.ts b/apps/browser/src/platform/offscreen-document/abstractions/offscreen-document.ts index e5aa8c86f50..2d3c6a3e713 100644 --- a/apps/browser/src/platform/offscreen-document/abstractions/offscreen-document.ts +++ b/apps/browser/src/platform/offscreen-document/abstractions/offscreen-document.ts @@ -1,4 +1,4 @@ -type OffscreenDocumentExtensionMessage = { +export type OffscreenDocumentExtensionMessage = { [key: string]: any; command: string; text?: string; @@ -9,18 +9,20 @@ type OffscreenExtensionMessageEventParams = { sender: chrome.runtime.MessageSender; }; -type OffscreenDocumentExtensionMessageHandlers = { +export type OffscreenDocumentExtensionMessageHandlers = { [key: string]: ({ message, sender }: OffscreenExtensionMessageEventParams) => any; offscreenCopyToClipboard: ({ message }: OffscreenExtensionMessageEventParams) => any; offscreenReadFromClipboard: () => any; }; -interface OffscreenDocument { +export interface OffscreenDocument { init(): void; } -export { - OffscreenDocumentExtensionMessage, - OffscreenDocumentExtensionMessageHandlers, - OffscreenDocument, -}; +export abstract class OffscreenDocumentService { + abstract withDocument( + reasons: chrome.offscreen.Reason[], + justification: string, + callback: () => Promise | T, + ): Promise; +} diff --git a/apps/browser/src/platform/offscreen-document/offscreen-document.service.spec.ts b/apps/browser/src/platform/offscreen-document/offscreen-document.service.spec.ts new file mode 100644 index 00000000000..d6be0a924e5 --- /dev/null +++ b/apps/browser/src/platform/offscreen-document/offscreen-document.service.spec.ts @@ -0,0 +1,101 @@ +import { DefaultOffscreenDocumentService } from "./offscreen-document.service"; + +class TestCase { + synchronicity: string; + private _callback: () => Promise | any; + get callback() { + return jest.fn(this._callback); + } + + constructor(synchronicity: string, callback: () => Promise | any) { + this.synchronicity = synchronicity; + this._callback = callback; + } + + toString() { + return this.synchronicity; + } +} + +describe.each([ + new TestCase("synchronous callback", () => 42), + new TestCase("asynchronous callback", () => Promise.resolve(42)), +])("DefaultOffscreenDocumentService %s", (testCase) => { + let sut: DefaultOffscreenDocumentService; + const reasons = [chrome.offscreen.Reason.TESTING]; + const justification = "justification is testing"; + const url = "offscreen-document/index.html"; + const api = { + createDocument: jest.fn(), + closeDocument: jest.fn(), + hasDocument: jest.fn().mockResolvedValue(false), + Reason: chrome.offscreen.Reason, + }; + let callback: jest.Mock<() => Promise | number>; + + beforeEach(() => { + callback = testCase.callback; + chrome.offscreen = api; + + sut = new DefaultOffscreenDocumentService(); + }); + + afterEach(() => { + jest.resetAllMocks(); + }); + + describe("withDocument", () => { + it("creates a document when none exists", async () => { + await sut.withDocument(reasons, justification, () => {}); + + expect(chrome.offscreen.createDocument).toHaveBeenCalledWith({ + url, + reasons, + justification, + }); + }); + + it("does not create a document when one exists", async () => { + api.hasDocument.mockResolvedValue(true); + + await sut.withDocument(reasons, justification, callback); + + expect(chrome.offscreen.createDocument).not.toHaveBeenCalled(); + }); + + describe.each([true, false])("hasDocument returns %s", (hasDocument) => { + beforeEach(() => { + api.hasDocument.mockResolvedValue(hasDocument); + }); + + it("calls the callback", async () => { + await sut.withDocument(reasons, justification, callback); + + expect(callback).toHaveBeenCalled(); + }); + + it("returns the callback result", async () => { + const result = await sut.withDocument(reasons, justification, callback); + + expect(result).toBe(42); + }); + + it("closes the document when the callback completes and no other callbacks are running", async () => { + await sut.withDocument(reasons, justification, callback); + + expect(chrome.offscreen.closeDocument).toHaveBeenCalled(); + }); + + it("does not close the document when the callback completes and other callbacks are running", async () => { + await Promise.all([ + sut.withDocument(reasons, justification, callback), + sut.withDocument(reasons, justification, callback), + sut.withDocument(reasons, justification, callback), + sut.withDocument(reasons, justification, callback), + ]); + + expect(chrome.offscreen.closeDocument).toHaveBeenCalledTimes(1); + }); + }); + }); +}); diff --git a/apps/browser/src/platform/offscreen-document/offscreen-document.service.ts b/apps/browser/src/platform/offscreen-document/offscreen-document.service.ts new file mode 100644 index 00000000000..da0ca382698 --- /dev/null +++ b/apps/browser/src/platform/offscreen-document/offscreen-document.service.ts @@ -0,0 +1,41 @@ +export class DefaultOffscreenDocumentService implements DefaultOffscreenDocumentService { + private workerCount = 0; + + constructor() {} + + async withDocument( + reasons: chrome.offscreen.Reason[], + justification: string, + callback: () => Promise | T, + ): Promise { + this.workerCount++; + try { + if (!(await this.documentExists())) { + await this.create(reasons, justification); + } + + return await callback(); + } finally { + this.workerCount--; + if (this.workerCount === 0) { + await this.close(); + } + } + } + + private async create(reasons: chrome.offscreen.Reason[], justification: string): Promise { + await chrome.offscreen.createDocument({ + url: "offscreen-document/index.html", + reasons, + justification, + }); + } + + private async close(): Promise { + await chrome.offscreen.closeDocument(); + } + + private async documentExists(): Promise { + return await chrome.offscreen.hasDocument(); + } +} diff --git a/apps/browser/src/platform/services/platform-utils/background-platform-utils.service.ts b/apps/browser/src/platform/services/platform-utils/background-platform-utils.service.ts index 27ed3f016bb..ec26d6aa29b 100644 --- a/apps/browser/src/platform/services/platform-utils/background-platform-utils.service.ts +++ b/apps/browser/src/platform/services/platform-utils/background-platform-utils.service.ts @@ -1,5 +1,7 @@ import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; +import { OffscreenDocumentService } from "../../offscreen-document/abstractions/offscreen-document"; + import { BrowserPlatformUtilsService } from "./browser-platform-utils.service"; export class BackgroundPlatformUtilsService extends BrowserPlatformUtilsService { @@ -8,8 +10,9 @@ export class BackgroundPlatformUtilsService extends BrowserPlatformUtilsService clipboardWriteCallback: (clipboardValue: string, clearMs: number) => void, biometricCallback: () => Promise, win: Window & typeof globalThis, + offscreenDocumentService: OffscreenDocumentService, ) { - super(clipboardWriteCallback, biometricCallback, win); + super(clipboardWriteCallback, biometricCallback, win, offscreenDocumentService); } override showToast( diff --git a/apps/browser/src/platform/services/platform-utils/browser-platform-utils.service.spec.ts b/apps/browser/src/platform/services/platform-utils/browser-platform-utils.service.spec.ts index 0df8f26344d..02c10b62cc4 100644 --- a/apps/browser/src/platform/services/platform-utils/browser-platform-utils.service.spec.ts +++ b/apps/browser/src/platform/services/platform-utils/browser-platform-utils.service.spec.ts @@ -1,15 +1,22 @@ +import { MockProxy, mock } from "jest-mock-extended"; + import { DeviceType } from "@bitwarden/common/enums"; import { flushPromises } from "../../../autofill/spec/testing-utils"; import { SafariApp } from "../../../browser/safariApp"; import { BrowserApi } from "../../browser/browser-api"; +import { OffscreenDocumentService } from "../../offscreen-document/abstractions/offscreen-document"; import BrowserClipboardService from "../browser-clipboard.service"; import { BrowserPlatformUtilsService } from "./browser-platform-utils.service"; class TestBrowserPlatformUtilsService extends BrowserPlatformUtilsService { - constructor(clipboardSpy: jest.Mock, win: Window & typeof globalThis) { - super(clipboardSpy, null, win); + constructor( + clipboardSpy: jest.Mock, + win: Window & typeof globalThis, + offscreenDocumentService: OffscreenDocumentService, + ) { + super(clipboardSpy, null, win, offscreenDocumentService); } showToast( @@ -24,13 +31,16 @@ class TestBrowserPlatformUtilsService extends BrowserPlatformUtilsService { describe("Browser Utils Service", () => { let browserPlatformUtilsService: BrowserPlatformUtilsService; + let offscreenDocumentService: MockProxy; const clipboardWriteCallbackSpy = jest.fn(); beforeEach(() => { + offscreenDocumentService = mock(); (window as any).matchMedia = jest.fn().mockReturnValueOnce({}); browserPlatformUtilsService = new TestBrowserPlatformUtilsService( clipboardWriteCallbackSpy, window, + offscreenDocumentService, ); }); @@ -223,23 +233,23 @@ describe("Browser Utils Service", () => { .spyOn(browserPlatformUtilsService, "getDevice") .mockReturnValue(DeviceType.ChromeExtension); getManifestVersionSpy.mockReturnValue(3); - jest.spyOn(BrowserApi, "createOffscreenDocument"); - jest.spyOn(BrowserApi, "sendMessageWithResponse").mockResolvedValue(undefined); - jest.spyOn(BrowserApi, "closeOffscreenDocument"); browserPlatformUtilsService.copyToClipboard(text); await flushPromises(); expect(triggerOffscreenCopyToClipboardSpy).toHaveBeenCalledWith(text); expect(clipboardServiceCopySpy).not.toHaveBeenCalled(); - expect(BrowserApi.createOffscreenDocument).toHaveBeenCalledWith( + expect(offscreenDocumentService.withDocument).toHaveBeenCalledWith( [chrome.offscreen.Reason.CLIPBOARD], "Write text to the clipboard.", + expect.any(Function), ); + + const callback = offscreenDocumentService.withDocument.mock.calls[0][2]; + await callback(); expect(BrowserApi.sendMessageWithResponse).toHaveBeenCalledWith("offscreenCopyToClipboard", { text, }); - expect(BrowserApi.closeOffscreenDocument).toHaveBeenCalled(); }); it("skips the clipboardWriteCallback if the clipboard is clearing", async () => { @@ -298,18 +308,21 @@ describe("Browser Utils Service", () => { .spyOn(browserPlatformUtilsService, "getDevice") .mockReturnValue(DeviceType.ChromeExtension); getManifestVersionSpy.mockReturnValue(3); - jest.spyOn(BrowserApi, "createOffscreenDocument"); - jest.spyOn(BrowserApi, "sendMessageWithResponse").mockResolvedValue("test"); - jest.spyOn(BrowserApi, "closeOffscreenDocument"); + offscreenDocumentService.withDocument.mockImplementationOnce((_, __, callback) => + Promise.resolve("test"), + ); await browserPlatformUtilsService.readFromClipboard(); - expect(BrowserApi.createOffscreenDocument).toHaveBeenCalledWith( + expect(offscreenDocumentService.withDocument).toHaveBeenCalledWith( [chrome.offscreen.Reason.CLIPBOARD], "Read text from the clipboard.", + expect.any(Function), ); + + const callback = offscreenDocumentService.withDocument.mock.calls[0][2]; + await callback(); expect(BrowserApi.sendMessageWithResponse).toHaveBeenCalledWith("offscreenReadFromClipboard"); - expect(BrowserApi.closeOffscreenDocument).toHaveBeenCalled(); }); it("returns an empty string from the offscreen document if the response is not of type string", async () => { @@ -317,9 +330,10 @@ describe("Browser Utils Service", () => { .spyOn(browserPlatformUtilsService, "getDevice") .mockReturnValue(DeviceType.ChromeExtension); getManifestVersionSpy.mockReturnValue(3); - jest.spyOn(BrowserApi, "createOffscreenDocument"); jest.spyOn(BrowserApi, "sendMessageWithResponse").mockResolvedValue(1); - jest.spyOn(BrowserApi, "closeOffscreenDocument"); + offscreenDocumentService.withDocument.mockImplementationOnce((_, __, callback) => + Promise.resolve(1), + ); const result = await browserPlatformUtilsService.readFromClipboard(); diff --git a/apps/browser/src/platform/services/platform-utils/browser-platform-utils.service.ts b/apps/browser/src/platform/services/platform-utils/browser-platform-utils.service.ts index 6e3b3aa4032..855492521bd 100644 --- a/apps/browser/src/platform/services/platform-utils/browser-platform-utils.service.ts +++ b/apps/browser/src/platform/services/platform-utils/browser-platform-utils.service.ts @@ -6,6 +6,7 @@ import { import { SafariApp } from "../../../browser/safariApp"; import { BrowserApi } from "../../browser/browser-api"; +import { OffscreenDocumentService } from "../../offscreen-document/abstractions/offscreen-document"; import BrowserClipboardService from "../browser-clipboard.service"; export abstract class BrowserPlatformUtilsService implements PlatformUtilsService { @@ -15,6 +16,7 @@ export abstract class BrowserPlatformUtilsService implements PlatformUtilsServic private clipboardWriteCallback: (clipboardValue: string, clearMs: number) => void, private biometricCallback: () => Promise, private globalContext: Window | ServiceWorkerGlobalScope, + private offscreenDocumentService: OffscreenDocumentService, ) {} static getDevice(globalContext: Window | ServiceWorkerGlobalScope): DeviceType { @@ -316,24 +318,26 @@ export abstract class BrowserPlatformUtilsService implements PlatformUtilsServic * Triggers the offscreen document API to copy the text to the clipboard. */ private async triggerOffscreenCopyToClipboard(text: string) { - await BrowserApi.createOffscreenDocument( + await this.offscreenDocumentService.withDocument( [chrome.offscreen.Reason.CLIPBOARD], "Write text to the clipboard.", + async () => { + await BrowserApi.sendMessageWithResponse("offscreenCopyToClipboard", { text }); + }, ); - await BrowserApi.sendMessageWithResponse("offscreenCopyToClipboard", { text }); - BrowserApi.closeOffscreenDocument(); } /** * Triggers the offscreen document API to read the text from the clipboard. */ private async triggerOffscreenReadFromClipboard() { - await BrowserApi.createOffscreenDocument( + const response = await this.offscreenDocumentService.withDocument( [chrome.offscreen.Reason.CLIPBOARD], "Read text from the clipboard.", + async () => { + return await BrowserApi.sendMessageWithResponse("offscreenReadFromClipboard"); + }, ); - const response = await BrowserApi.sendMessageWithResponse("offscreenReadFromClipboard"); - BrowserApi.closeOffscreenDocument(); if (typeof response === "string") { return response; } diff --git a/apps/browser/src/platform/services/platform-utils/foreground-platform-utils.service.ts b/apps/browser/src/platform/services/platform-utils/foreground-platform-utils.service.ts index 24aa45d5c32..f775f049e78 100644 --- a/apps/browser/src/platform/services/platform-utils/foreground-platform-utils.service.ts +++ b/apps/browser/src/platform/services/platform-utils/foreground-platform-utils.service.ts @@ -1,5 +1,7 @@ import { ToastService } from "@bitwarden/components"; +import { OffscreenDocumentService } from "../../offscreen-document/abstractions/offscreen-document"; + import { BrowserPlatformUtilsService } from "./browser-platform-utils.service"; export class ForegroundPlatformUtilsService extends BrowserPlatformUtilsService { @@ -8,8 +10,9 @@ export class ForegroundPlatformUtilsService extends BrowserPlatformUtilsService clipboardWriteCallback: (clipboardValue: string, clearMs: number) => void, biometricCallback: () => Promise, win: Window & typeof globalThis, + offscreenDocumentService: OffscreenDocumentService, ) { - super(clipboardWriteCallback, biometricCallback, win); + super(clipboardWriteCallback, biometricCallback, win, offscreenDocumentService); } override showToast( diff --git a/apps/browser/src/popup/services/services.module.ts b/apps/browser/src/popup/services/services.module.ts index 163b2f1edb7..6e7d3c2230c 100644 --- a/apps/browser/src/popup/services/services.module.ts +++ b/apps/browser/src/popup/services/services.module.ts @@ -100,6 +100,8 @@ import { runInsideAngular } from "../../platform/browser/run-inside-angular.oper /* eslint-disable no-restricted-imports */ import { ChromeMessageSender } from "../../platform/messaging/chrome-message.sender"; /* eslint-enable no-restricted-imports */ +import { OffscreenDocumentService } from "../../platform/offscreen-document/abstractions/offscreen-document"; +import { DefaultOffscreenDocumentService } from "../../platform/offscreen-document/offscreen-document.service"; import BrowserPopupUtils from "../../platform/popup/browser-popup-utils"; import { BrowserFileDownloadService } from "../../platform/popup/services/browser-file-download.service"; import { BrowserStateService as StateServiceAbstraction } from "../../platform/services/abstractions/browser-state.service"; @@ -287,9 +289,17 @@ const safeProviders: SafeProvider[] = [ useFactory: getBgService("devicesService"), deps: [], }), + safeProvider({ + provide: OffscreenDocumentService, + useClass: DefaultOffscreenDocumentService, + deps: [], + }), safeProvider({ provide: PlatformUtilsService, - useFactory: (toastService: ToastService) => { + useFactory: ( + toastService: ToastService, + offscreenDocumentService: OffscreenDocumentService, + ) => { return new ForegroundPlatformUtilsService( toastService, (clipboardValue: string, clearMs: number) => { @@ -306,9 +316,10 @@ const safeProviders: SafeProvider[] = [ return response.result; }, window, + offscreenDocumentService, ); }, - deps: [ToastService], + deps: [ToastService, OffscreenDocumentService], }), safeProvider({ provide: PasswordGenerationServiceAbstraction,