diff --git a/knip.mjs b/knip.mjs index 266b2a49f9..8c7acbf63b 100644 --- a/knip.mjs +++ b/knip.mjs @@ -19,7 +19,6 @@ const knipConfig = { "static/*", // App messenger and common storage - "src/contentScript/externalProtocol.ts", "src/background/messenger/external/api.ts", "src/store/browserExtensionIdStorage.ts", diff --git a/package-lock.json b/package-lock.json index 273d89da55..3be3f7dfb1 100644 --- a/package-lock.json +++ b/package-lock.json @@ -87,7 +87,6 @@ "nunjucks": "^3.2.4", "object-hash": "^2.2.0", "one-event": "^4.2.0", - "one-mutation": "^2.1.0", "p-defer": "^4.0.0", "p-memoize": "^7.0.0", "p-timeout": "^6.1.2", @@ -23735,13 +23734,6 @@ "url": "https://github.com/sponsors/fregante" } }, - "node_modules/one-mutation": { - "version": "2.1.0", - "integrity": "sha512-iRdYEWwcATme2qXrQ0dFXmE5ICqEEKqIUb4imFF/jCJer/PKKwP1L755MVzmbjSIxSQZ6HNcvMSwOUnhmYSoEw==", - "funding": { - "url": "https://github.com/sponsors/fregante" - } - }, "node_modules/onetime": { "version": "5.1.2", "integrity": "sha512-kbpaSSGJTWdAY5KPVeMOKXSrPtr8C8C7wodJbcsd51jRnmD+GZu8Y0VoU6Dm5Z4vWr0Ig/1NKuWRKf7j5aaYSg==", diff --git a/package.json b/package.json index dc8c2fc60d..1349586832 100644 --- a/package.json +++ b/package.json @@ -113,7 +113,6 @@ "nunjucks": "^3.2.4", "object-hash": "^2.2.0", "one-event": "^4.2.0", - "one-mutation": "^2.1.0", "p-defer": "^4.0.0", "p-memoize": "^7.0.0", "p-timeout": "^6.1.2", diff --git a/src/contentScript/contentScript.ts b/src/contentScript/contentScript.ts index 349453f9b2..f58fd72837 100644 --- a/src/contentScript/contentScript.ts +++ b/src/contentScript/contentScript.ts @@ -23,10 +23,9 @@ import "./contentScript.scss"; import { addContentScriptIndicator } from "@/development/visualInjection"; import { uuidv4 } from "@/types/helpers"; import { - isInstalledInThisSession, - setInstalledInThisSession, - setReadyInThisDocument, - unsetReadyInThisDocument, + isContentScriptInstalled, + setContentScriptInstalled, + setContentScriptReady, } from "@/contentScript/ready"; import { onContextInvalidated } from "webext-events"; import { logPromiseDuration } from "@/utils/promiseUtils"; @@ -57,7 +56,7 @@ async function initContentScript() { const urlInfo = top === self ? "" : `in frame ${location.href}`; const uuid = uuidv4(); - if (isInstalledInThisSession()) { + if (isContentScriptInstalled()) { // Must prevent multiple injection because repeat messenger registration causes message handling errors: // https://github.com/pixiebrix/webext-messenger/issues/88 // Prior to 1.7.31 we had been using `webext-dynamic-content-scripts` which can inject the same content script @@ -84,7 +83,7 @@ async function initContentScript() { console.debug(`contentScript: importing ${uuid} ${urlInfo}`); - setInstalledInThisSession(); + setContentScriptInstalled(); // Keeping the import separate ensures that no side effects are run until this point const contentScriptPromise = import( @@ -96,10 +95,9 @@ async function initContentScript() { const { init } = await contentScriptPromise; await logPromiseDuration("contentScript: ready", init()); - setReadyInThisDocument(uuid); + setContentScriptReady(); onContextInvalidated.addListener(() => { - unsetReadyInThisDocument(uuid); console.debug("contentScript: invalidated", uuid); }); diff --git a/src/contentScript/externalProtocol.ts b/src/contentScript/externalProtocol.ts deleted file mode 100644 index 9cbc3be9f0..0000000000 --- a/src/contentScript/externalProtocol.ts +++ /dev/null @@ -1,211 +0,0 @@ -/* - * Copyright (C) 2024 PixieBrix, Inc. - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . - */ - -import { uuidv4 } from "@/types/helpers"; -import { - type HandlerEntry, - type HandlerOptions, - isErrorResponse, - toErrorResponse, -} from "@/utils/legacyMessengerUtils"; -import { type SerializableResponse } from "@/types/messengerTypes"; -import pTimeout from "p-timeout"; -import oneMutation from "one-mutation"; -import { isContentScript } from "webext-detect-page"; -import { deserializeError } from "serialize-error"; -import { expectContext, forbidContext } from "@/utils/expectContext"; -import { CONTENT_SCRIPT_READY_ATTRIBUTE } from "@/domConstants"; - -// Context for this protocol: -// - Implemented and explained in https://github.com/pixiebrix/pixiebrix-extension/pull/1019 -// - It's only one-way from the app to the content scripts -// - The communication happens via posted messages -// - Messages **are received by both ends**, so they use the `type` property as a differentiator -// - The app sends messages with a `type` property -// - The content script responds without `type` property - -// TODO: The content script should load within 200ms, but when the devtools are open it could take 4-5 seconds. -// Find out why https://github.com/pixiebrix/pixiebrix-extension/pull/1019#discussion_r684894579 -const POLL_READY_TIMEOUT = - process.env.ENVIRONMENT === "development" ? 6000 : 2000; - -// TODO: Some handlers could take longer, so it needs to be refactored. -// https://github.com/pixiebrix/pixiebrix-extension/issues/1015 -const RESPONSE_TIMEOUT_MS = 2000; - -const MESSAGE_PREFIX = "@@pixiebrix/external/"; - -interface Message { - type: string; - payload: R; - meta: { - nonce: string; - }; -} - -interface Response { - payload: R; - meta: { - nonce: string; - }; -} - -const contentScriptHandlers = new Map(); - -async function waitExtensionLoaded(): Promise { - if (document.documentElement.hasAttribute(CONTENT_SCRIPT_READY_ATTRIBUTE)) { - return; - } - - await pTimeout( - oneMutation(document.documentElement, { - attributes: true, - attributeFilter: [CONTENT_SCRIPT_READY_ATTRIBUTE], - }), - { - milliseconds: POLL_READY_TIMEOUT, - message: `The extension did not load within ${ - POLL_READY_TIMEOUT / 1000 - }s`, - }, - ); -} - -/** - * The message is also received by the same context, so it must detect that and - * ignore its own messages. Currently it uses the `type` property as a differentiator: - * The lack of it means it's a response from the content script - */ -function sendMessageToOtherSide(message: Message | Response) { - document.defaultView.postMessage(message, document.defaultView.origin); -} - -/** Content script handler for messages from app */ -async function onContentScriptReceiveMessage( - event: MessageEvent>, -): Promise { - expectContext("contentScript"); - - if (event.source !== document.defaultView) { - // The message comes from other views (PB does not send these) - return; - } - - const { type, meta, payload } = event.data; - if (!type || !Array.isArray(payload)) { - // It might be a response that `onContentScriptReceiveMessage` itself sent - return; - } - - const { handler, options } = contentScriptHandlers.get(type) ?? {}; - if (!handler) { - // Handler not registered, it might be handled elsewhere - return; - } - - if (!options.asyncResponse) { - // eslint-disable-next-line promise/prefer-await-to-then -- Legacy code - void handler(...payload).catch((error) => { - console.warn(`${type}: ${meta.nonce}: Notification error`, error); - }); - } - - const response: Response = { - meta, - payload: null, - }; - try { - response.payload = await handler(...payload); - console.debug(`${type}: ${meta.nonce}: Handler success`); - } catch (error) { - response.payload = toErrorResponse(type, error); - console.warn(`${type}: ${meta.nonce}: Handler error`, error); - } - - sendMessageToOtherSide(response); -} - -/** Set up listener for specific message via nonce */ -async function oneResponse(nonce: string): Promise { - return new Promise((resolve) => { - function onMessage(event: MessageEvent) { - // Responses *must not* have a `type`, but just a `nonce` and `payload` - if (!event.data?.type && event.data?.meta?.nonce === nonce) { - document.defaultView.removeEventListener("message", onMessage); - // eslint-disable-next-line @typescript-eslint/no-unsafe-argument -- TODO: Find a better solution than disabling - resolve(event.data.payload); - } - } - - document.defaultView.addEventListener("message", onMessage); - }); -} - -export function liftExternalToContentScript< - TArguments extends unknown[], - R extends SerializableResponse, ->( - type: string, - method: (...args: TArguments) => Promise, - options?: HandlerOptions, -): (...args: TArguments) => Promise { - const fullType = `${MESSAGE_PREFIX}${type}`; - // Set defaults - options = { - asyncResponse: true, - ...options, - }; - - if (isContentScript()) { - // Register global handler; Automatically deduplicated - document.defaultView.addEventListener( - "message", - onContentScriptReceiveMessage, - ); - - contentScriptHandlers.set(fullType, { handler: method, options }); - console.debug(`${fullType}: Installed content script handler`); - return method; - } - - return async (...args: TArguments) => { - forbidContext("extension"); - - await waitExtensionLoaded(); - - const nonce = uuidv4(); - const message: Message = { - type: fullType, - payload: args, - meta: { nonce }, - }; - - console.debug(`${fullType}: Sending from app`, message); - sendMessageToOtherSide(message); - - // Communication is completely asynchronous; This sets up response for the specific nonce - const payload = await pTimeout(oneResponse(nonce), { - milliseconds: RESPONSE_TIMEOUT_MS, - }); - - if (isErrorResponse(payload)) { - throw deserializeError(payload.$$error); - } - - return payload; - }; -} diff --git a/src/contentScript/loadActivationEnhancementsCore.test.ts b/src/contentScript/loadActivationEnhancementsCore.test.ts index 6d71ef3549..ef0b1e608e 100644 --- a/src/contentScript/loadActivationEnhancementsCore.test.ts +++ b/src/contentScript/loadActivationEnhancementsCore.test.ts @@ -31,7 +31,7 @@ import { loadActivationEnhancements, TEST_unloadActivationEnhancements, } from "@/contentScript/loadActivationEnhancementsCore"; -import { isReadyInThisDocument } from "@/contentScript/ready"; +import { isContentScriptReady } from "@/contentScript/ready"; import { isLinked } from "@/auth/authStorage"; import { array } from "cooky-cutter"; import { MARKETPLACE_URL } from "@/urlConstants"; @@ -49,10 +49,7 @@ jest.mock("@/auth/authStorage", () => ({ const isLinkedMock = jest.mocked(isLinked); -jest.mock("@/contentScript/ready", () => ({ - isReadyInThisDocument: jest.fn(() => true), -})); - +jest.mock("@/contentScript/ready"); jest.mock("@/store/extensionsStorage"); jest.mock("@/background/messenger/external/_implementation"); jest.mock("@/sidebar/store"); @@ -79,7 +76,7 @@ describe("marketplace enhancements", () => { beforeEach(() => { document.body.innerHTML = ""; document.body.innerHTML = getDocument(activateButtonsHtml).body.innerHTML; - jest.mocked(isReadyInThisDocument).mockImplementation(() => true); + jest.mocked(isContentScriptReady).mockImplementation(() => true); getActivatedModIdsMock.mockResolvedValue(new Set()); getActivatingModsMock.mockResolvedValue([]); }); diff --git a/src/contentScript/loadActivationEnhancementsCore.ts b/src/contentScript/loadActivationEnhancementsCore.ts index 8b73d6f064..6822338195 100644 --- a/src/contentScript/loadActivationEnhancementsCore.ts +++ b/src/contentScript/loadActivationEnhancementsCore.ts @@ -19,7 +19,7 @@ import { isEmpty, startsWith } from "lodash"; import { DEFAULT_SERVICE_URL, MARKETPLACE_URL } from "@/urlConstants"; import { getActivatedModIds } from "@/store/extensionsStorage"; import { pollUntilTruthy } from "@/utils/promiseUtils"; -import { isReadyInThisDocument } from "@/contentScript/ready"; +import { isContentScriptReady } from "@/contentScript/ready"; import { getRegistryIdsFromActivateUrlSearchParams } from "@/activation/activationLinkUtils"; import { type ACTIVATE_EVENT_DETAIL, @@ -97,15 +97,12 @@ export async function loadActivationEnhancements(): Promise { button.addEventListener("click", async (event) => { event.preventDefault(); - const isContentScriptReady = await pollUntilTruthy( - isReadyInThisDocument, - { - maxWaitMillis: 2000, - intervalMillis: 100, - }, - ); + const isReady = await pollUntilTruthy(isContentScriptReady, { + maxWaitMillis: 2000, + intervalMillis: 100, + }); - if (isContentScriptReady) { + if (isReady) { const detail: ACTIVATE_EVENT_DETAIL = { // The button href may have changed since the listener was added, extract ids again activateUrl: button.href, diff --git a/src/contentScript/ready.ts b/src/contentScript/ready.ts index 5f6e67fd26..23778bbecf 100644 --- a/src/contentScript/ready.ts +++ b/src/contentScript/ready.ts @@ -19,32 +19,25 @@ /** * @file Handles the definition and state of "readiness" of the content script (CS) context. * - * `Symbol.for(x)` maintains the same symbol until the context is invalidated, even if - * the CS is injected multiple times. This should not happen but it's something we - * need to account for: https://github.com/pixiebrix/pixiebrix-extension/issues/3510 + * There's one "content script" context per extension load. All the content scripts injected + * by the extension share this context and its symbols, even if injected repeatedly. * - * When a context is invalidated, the CS and the changes it made may remain on the page, - * but the new background is not able to message the old CS, so the CS must be injected - * again. This might cause issues if the previous CS keeps "touching" the page after - * being deactivated: https://github.com/pixiebrix/pixiebrix-extension/issues/3132 + * When the extension is deactivated or reloaded, the content script is still kept active but it's + * unable to communicate to the background page/worker (because that context has been invalidated). + * In the console you can see multiple content scripts running at the same time, until you reload the tab. * - * Using both a symbol and an attribute accounts for these 2 situations, to detect - * and handle duplicate injections in the same session and across sessions: - * - Same session (mistake in injection): ignore - * - Context invalidated: CS must be injected again + * When the (background) context is invalidated, we must manually remove all of its listeners and + * attached widgets to avoid conflicts with future content script injections: + * https://github.com/pixiebrix/pixiebrix-extension/issues/4258 */ -import { type UUID } from "@/types/stringTypes"; import { type Target } from "@/types/messengerTypes"; import { forbidContext } from "@/utils/expectContext"; import { executeFunction } from "webext-content-scripts"; -import { CONTENT_SCRIPT_READY_ATTRIBUTE } from "@/domConstants"; - -// eslint-disable-next-line local-rules/persistBackgroundData -- Static -const html = globalThis.document?.documentElement; // These two must be synched in `getTargetState` const CONTENT_SCRIPT_INJECTED_SYMBOL = Symbol.for("content-script-injected"); +const CONTENT_SCRIPT_READY_SYMBOL = Symbol.for("content-script-ready"); /** Communicates readiness to `ensureContentScript` */ export const ENSURE_CONTENT_SCRIPT_READY = @@ -53,6 +46,7 @@ export const ENSURE_CONTENT_SCRIPT_READY = declare global { interface Window { [CONTENT_SCRIPT_INJECTED_SYMBOL]?: true; + [CONTENT_SCRIPT_READY_SYMBOL]?: true; } } @@ -65,31 +59,25 @@ interface TargetState { /** * Returns true iff the content script has been injected in the content script Javascript VM for the window. */ -export function isInstalledInThisSession(): boolean { - return CONTENT_SCRIPT_INJECTED_SYMBOL in globalThis; +export function isContentScriptInstalled(): boolean { + return CONTENT_SCRIPT_INJECTED_SYMBOL in window; } /** * Mark that the content script has been injected in content script Javascript VM for the window. */ -export function setInstalledInThisSession(): void { +export function setContentScriptInstalled(): void { // eslint-disable-next-line security/detect-object-injection -- symbol window[CONTENT_SCRIPT_INJECTED_SYMBOL] = true; } -export function isReadyInThisDocument(): boolean { - return html.hasAttribute(CONTENT_SCRIPT_READY_ATTRIBUTE); -} - -export function setReadyInThisDocument(uuid: UUID): void { - html.setAttribute(CONTENT_SCRIPT_READY_ATTRIBUTE, uuid); +export function isContentScriptReady(): boolean { + return CONTENT_SCRIPT_READY_SYMBOL in window; } -/** Only removes the attribute if the `uuid` matches. This avoids race conditions with the new content script */ -export function unsetReadyInThisDocument(uuid: UUID): void { - if (uuid === html.getAttribute(CONTENT_SCRIPT_READY_ATTRIBUTE)) { - html.removeAttribute(CONTENT_SCRIPT_READY_ATTRIBUTE); - } +export function setContentScriptReady(): void { + // eslint-disable-next-line security/detect-object-injection -- symbol + window[CONTENT_SCRIPT_READY_SYMBOL] = true; } /** @@ -108,13 +96,12 @@ export async function getTargetState(target: Target): Promise { const CONTENT_SCRIPT_INJECTED_SYMBOL = Symbol.for( "content-script-injected", ); - const CONTENT_SCRIPT_READY_ATTRIBUTE = "data-pb-ready"; + + const CONTENT_SCRIPT_READY_SYMBOL = Symbol.for("content-script-ready"); return { url: location.href, installed: CONTENT_SCRIPT_INJECTED_SYMBOL in globalThis, - ready: document.documentElement.hasAttribute( - CONTENT_SCRIPT_READY_ATTRIBUTE, - ), + ready: CONTENT_SCRIPT_READY_SYMBOL in globalThis, }; }); } diff --git a/src/domConstants.ts b/src/domConstants.ts index fdc2195f58..47181e3f8e 100644 --- a/src/domConstants.ts +++ b/src/domConstants.ts @@ -21,8 +21,6 @@ export const NOTIFICATIONS_Z_INDEX = 2_147_483_647; export const MAX_Z_INDEX = NOTIFICATIONS_Z_INDEX - 1; // Let notifications always be higher -export const CONTENT_SCRIPT_READY_ATTRIBUTE = "data-pb-ready"; - export const SELECTION_MENU_READY_ATTRIBUTE = "data-pb-selection-menu-ready"; export const PANEL_FRAME_ID = "pixiebrix-extension"; @@ -51,6 +49,5 @@ export const PRIVATE_ATTRIBUTES_SELECTOR = ` .${PIXIEBRIX_QUICK_BAR_CONTAINER_CLASS}, .${PIXIEBRIX_NOTIFICATION_CLASS}, [${PIXIEBRIX_DATA_ATTR}], - [${CONTENT_SCRIPT_READY_ATTRIBUTE}], [${EXTENSION_POINT_DATA_ATTR}] `; diff --git a/src/utils/inference/markupInference.ts b/src/utils/inference/markupInference.ts index d0db09fd0e..35c295c148 100644 --- a/src/utils/inference/markupInference.ts +++ b/src/utils/inference/markupInference.ts @@ -16,7 +16,6 @@ */ import { - CONTENT_SCRIPT_READY_ATTRIBUTE, EXTENSION_POINT_DATA_ATTR, PANEL_FRAME_ID, PIXIEBRIX_DATA_ATTR, @@ -59,7 +58,6 @@ const TEMPLATE_ATTR_EXCLUDE_PATTERNS = [ EXTENSION_POINT_DATA_ATTR, PIXIEBRIX_DATA_ATTR, - CONTENT_SCRIPT_READY_ATTRIBUTE, // Angular attributes /^_ngcontent-/, diff --git a/src/utils/inference/selectorInference.ts b/src/utils/inference/selectorInference.ts index 1b6f4abbfd..41f5d004a0 100644 --- a/src/utils/inference/selectorInference.ts +++ b/src/utils/inference/selectorInference.ts @@ -21,11 +21,7 @@ import type { CssSelectorMatch, CssSelectorType, } from "css-selector-generator/types/types.js"; -import { - CONTENT_SCRIPT_READY_ATTRIBUTE, - EXTENSION_POINT_DATA_ATTR, - PIXIEBRIX_DATA_ATTR, -} from "@/domConstants"; +import { EXTENSION_POINT_DATA_ATTR, PIXIEBRIX_DATA_ATTR } from "@/domConstants"; import { guessUsefulness, isRandomString } from "@/utils/detectRandomString"; import { getSiteSelectorHint, @@ -87,7 +83,6 @@ const UNSTABLE_SELECTORS = [ // Our attributes EXTENSION_POINT_DATA_ATTR, PIXIEBRIX_DATA_ATTR, - CONTENT_SCRIPT_READY_ATTRIBUTE, "style", ), ];