From 11601d2d7acbca23a305e11d85010e25430daf5c Mon Sep 17 00:00:00 2001 From: Federico Brigante Date: Mon, 5 Sep 2022 01:08:21 +0700 Subject: [PATCH 1/3] Mark document as "not ready" when the context is invalidated --- src/contentScript/contentScript.ts | 18 +++++++++++++++--- src/contentScript/contentScriptCore.ts | 8 -------- src/contentScript/ready.ts | 25 +++++++++++++++---------- src/utils.ts | 13 +++++++++++++ 4 files changed, 43 insertions(+), 21 deletions(-) diff --git a/src/contentScript/contentScript.ts b/src/contentScript/contentScript.ts index 732e48a544..64c11e2683 100644 --- a/src/contentScript/contentScript.ts +++ b/src/contentScript/contentScript.ts @@ -24,8 +24,10 @@ import { isReadyInThisDocument, setInstalledInThisSession, setReadyInThisDocument, + unsetReadyInThisDocument, } from "@/contentScript/ready"; import { logPromiseDuration } from "@/utils"; +import { onContextInvalidated } from "@/errors/contextInvalidated"; // See note in `@/contentScript/ready.ts` for further details about the lifecycle of content scripts async function initContentScript() { @@ -49,12 +51,22 @@ async function initContentScript() { setInstalledInThisSession(); // Keeping the import separate ensures that no side effects are run until this point - const { init } = await logPromiseDuration( - "contentScript: imported", // "imported" timing includes the parsing of the file, which can take 500-1000ms - import(/* webpackChunkName: "contentScriptCore" */ "./contentScriptCore") + const contentScript = import( + /* webpackChunkName: "contentScriptCore" */ "./contentScriptCore" ); + + // "imported" timing includes the parsing of the file, which can take 500-1000ms + void logPromiseDuration("contentScript: imported", contentScript); + + const { init } = await contentScript; await init(uuid); setReadyInThisDocument(uuid); + + // eslint-disable-next-line promise/prefer-await-to-then -- It's an unrelated event listener + void onContextInvalidated().then(() => { + unsetReadyInThisDocument(uuid); + console.debug("contentScript: invalidated", uuid); + }); } void logPromiseDuration("contentScript: ready", initContentScript()).catch( diff --git a/src/contentScript/contentScriptCore.ts b/src/contentScript/contentScriptCore.ts index e0e862951b..731953efcb 100644 --- a/src/contentScript/contentScriptCore.ts +++ b/src/contentScript/contentScriptCore.ts @@ -36,7 +36,6 @@ import { initPartnerIntegrations } from "@/contentScript/partnerIntegrations"; import { isContextInvalidatedError, notifyContextInvalidated, - onContextInvalidated, } from "@/errors/contextInvalidated"; import { uncaughtErrorHandlers } from "@/telemetry/reportUncaughtErrors"; import { UUID } from "@/core"; @@ -77,11 +76,4 @@ export async function init(uuid: UUID): Promise { // Let the partner page know initPartnerIntegrations(); - - // Put here instead of in the sync contentScript because contextInvalidated has a transitive dependency on the - // messenger (which causes a race on messenger initialization) - // eslint-disable-next-line promise/prefer-await-to-then -- It's an unrelated event listener - void onContextInvalidated().then(() => { - console.debug("contentScript: invalidated", uuid); - }); } diff --git a/src/contentScript/ready.ts b/src/contentScript/ready.ts index bca5720806..e66db91b18 100644 --- a/src/contentScript/ready.ts +++ b/src/contentScript/ready.ts @@ -28,7 +28,9 @@ * being deactivated: https://github.com/pixiebrix/pixiebrix-extension/issues/3132 * * 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. + * and handle duplicate injections in the same session and across sessions: + * - Same session (mistake in injection): ignore + * - Context invalidated: CS must be injected again */ import { UUID } from "@/core"; @@ -36,6 +38,8 @@ import { Target } from "@/types"; import { expectContext } from "@/utils/expectContext"; import { executeFunction } from "webext-content-scripts"; +const html = globalThis.document?.documentElement; + // These two must be synched in `getTargetState` export const CONTENT_SCRIPT_INJECTED_SYMBOL = Symbol.for( "content-script-injected" @@ -63,14 +67,17 @@ export function setInstalledInThisSession(): void { } export function isReadyInThisDocument(): boolean { - return document.documentElement.hasAttribute(CONTENT_SCRIPT_READY_ATTRIBUTE); + return html.hasAttribute(CONTENT_SCRIPT_READY_ATTRIBUTE); +} + +export function setReadyInThisDocument(uuid: UUID): void { + html.setAttribute(CONTENT_SCRIPT_READY_ATTRIBUTE, uuid); } -export function setReadyInThisDocument(uuid: UUID | false): void { - if (uuid) { - document.documentElement.setAttribute(CONTENT_SCRIPT_READY_ATTRIBUTE, uuid); - } else { - document.documentElement.removeAttribute(CONTENT_SCRIPT_READY_ATTRIBUTE); +/** 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); } } @@ -90,9 +97,7 @@ export async function getTargetState(target: Target): Promise { return { url: location.href, installed: CONTENT_SCRIPT_INJECTED_SYMBOL in globalThis, - ready: document.documentElement.hasAttribute( - CONTENT_SCRIPT_READY_ATTRIBUTE - ), + ready: html.hasAttribute(CONTENT_SCRIPT_READY_ATTRIBUTE), }; }); } diff --git a/src/utils.ts b/src/utils.ts index 486e56ec8f..7410438795 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -535,6 +535,19 @@ export async function logPromiseDuration

( try { return await promise; } finally { + // Prefer `debug` level; `console.time` has `log` level + console.debug(title, `${Math.round(Date.now() - start)}ms`); + } +} + +export async function logFunctionDuration< + Fn extends (...args: unknown[]) => Promise +>(title: string, fn: Fn): Promise> { + const start = Date.now(); + try { + return (await fn()) as Awaited>; + } finally { + // Prefer `debug` level; `console.time` has `log` level console.debug(title, `${Math.round(Date.now() - start)}ms`); } } From bef3e9a4e374c6a3b3e2c347d2606e6b5f91dec3 Mon Sep 17 00:00:00 2001 From: Federico Brigante Date: Sat, 10 Sep 2022 18:37:22 +0700 Subject: [PATCH 2/3] Just ensure the messenger isn't loaded unless used --- src/contentScript/ready.ts | 14 ++++++++++---- src/errors/contextInvalidated.ts | 7 +++++-- src/utils/expectContext.ts | 3 +++ src/utils/notify.tsx | 2 +- 4 files changed, 19 insertions(+), 7 deletions(-) diff --git a/src/contentScript/ready.ts b/src/contentScript/ready.ts index e66db91b18..b513e0d5f2 100644 --- a/src/contentScript/ready.ts +++ b/src/contentScript/ready.ts @@ -35,7 +35,7 @@ import { UUID } from "@/core"; import { Target } from "@/types"; -import { expectContext } from "@/utils/expectContext"; +import { forbidContext } from "@/utils/expectContext"; import { executeFunction } from "webext-content-scripts"; const html = globalThis.document?.documentElement; @@ -86,10 +86,14 @@ export function unsetReadyInThisDocument(uuid: UUID): void { * @throws Error if background page doesn't have permission to access the tab * */ export async function getTargetState(target: Target): Promise { - expectContext("background"); + forbidContext( + "web", + "chrome.tabs is only available in chrome-extension:// pages" + ); return executeFunction(target, () => { - // Thise two must also be inlined here because `executeFunction` does not include non-local variables + // This function does not have access to globals, the outside scope, nor `import()` + // These two symbols must be repeated inline const CONTENT_SCRIPT_INJECTED_SYMBOL = Symbol.for( "content-script-injected" ); @@ -97,7 +101,9 @@ export async function getTargetState(target: Target): Promise { return { url: location.href, installed: CONTENT_SCRIPT_INJECTED_SYMBOL in globalThis, - ready: html.hasAttribute(CONTENT_SCRIPT_READY_ATTRIBUTE), + ready: document.documentElement.hasAttribute( + CONTENT_SCRIPT_READY_ATTRIBUTE + ), }; }); } diff --git a/src/errors/contextInvalidated.ts b/src/errors/contextInvalidated.ts index 88c1b6fe97..c94fb62c2f 100644 --- a/src/errors/contextInvalidated.ts +++ b/src/errors/contextInvalidated.ts @@ -16,7 +16,6 @@ */ import { expectContext } from "@/utils/expectContext"; -import notify from "@/utils/notify"; import { once } from "lodash"; import { CONTEXT_INVALIDATED_ERROR, @@ -30,7 +29,11 @@ const id = "connection-lost"; * Display a notification when the background page unloads/reloads because at this point * all communcation becomes impossible. */ -export function notifyContextInvalidated(): void { +export async function notifyContextInvalidated(): Promise { + // `import()` is only needed to avoid execution of its dependencies, not to lazy-load it + // https://github.com/pixiebrix/pixiebrix-extension/issues/4058#issuecomment-1217391772 + // eslint-disable-next-line import/dynamic-import-chunkname + const { notify } = await import(/* webpackMode: "eager" */ "@/utils/notify"); notify.error({ id, message: "PixieBrix was updated or restarted. Reload the page to continue", diff --git a/src/utils/expectContext.ts b/src/utils/expectContext.ts index 496bb2d309..2237bf9282 100644 --- a/src/utils/expectContext.ts +++ b/src/utils/expectContext.ts @@ -19,6 +19,7 @@ import { isBackground, isContentScript, isExtensionContext, + isWebPage, } from "webext-detect-page"; /** @@ -43,12 +44,14 @@ function createError( } const contexts = [ + "web", "extension", "background", "contentScript", "devTools", ] as const; const contextMap = new Map boolean>([ + ["web", isWebPage], ["extension", isExtensionContext], ["background", isBackground], ["contentScript", isContentScript], diff --git a/src/utils/notify.tsx b/src/utils/notify.tsx index e7918c4784..76ffb1406c 100644 --- a/src/utils/notify.tsx +++ b/src/utils/notify.tsx @@ -235,7 +235,7 @@ function _show( } // Please only add logic to `showNotification` -const notify = { +export const notify = { /** * @example notify.error('User message') * @example notify.error({error: new Error('Error that can be shown to the user')}) From 3aeb92061f716c67d4deff79e75e4c7ef307bfdf Mon Sep 17 00:00:00 2001 From: Federico Brigante Date: Sat, 10 Sep 2022 18:44:48 +0700 Subject: [PATCH 3/3] Lint --- src/contentScript/contentScriptCore.ts | 2 +- src/telemetry/BackgroundLogger.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/contentScript/contentScriptCore.ts b/src/contentScript/contentScriptCore.ts index 731953efcb..b0113ee032 100644 --- a/src/contentScript/contentScriptCore.ts +++ b/src/contentScript/contentScriptCore.ts @@ -47,7 +47,7 @@ function ignoreContextInvalidatedErrors( // they're actually interacting with PixieBrix, otherwise they might receive the notification // at random times. if (isContextInvalidatedError(errorEvent)) { - notifyContextInvalidated(); + void notifyContextInvalidated(); errorEvent.preventDefault(); } } diff --git a/src/telemetry/BackgroundLogger.ts b/src/telemetry/BackgroundLogger.ts index a9f36dff8a..771a3ed95c 100644 --- a/src/telemetry/BackgroundLogger.ts +++ b/src/telemetry/BackgroundLogger.ts @@ -74,7 +74,7 @@ class BackgroundLogger implements Logger { async error(error: unknown, data: JsonObject): Promise { if (wasContextInvalidated() && !isBackground() && !isDevToolsPage()) { - notifyContextInvalidated(); + void notifyContextInvalidated(); } console.error("BackgroundLogger:error", {