#4190: Fix content script readiness race conditions and context invalidation handling

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(

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";
@@ -48,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();
   }
 }
@@ -77,11 +76,4 @@ export async function init(uuid: UUID): Promise<void> {
 
   // 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);
-  });
 }

src/contentScript/ready.ts

@@ -28,14 +28,18 @@
  * 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";
 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;
+
 // 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);
   }
 }
 
@@ -79,10 +86,14 @@ export function setReadyInThisDocument(uuid: UUID | false): void {
  * @throws Error if background page doesn't have permission to access the tab
  * */
 export async function getTargetState(target: Target): Promise<TargetState> {
-  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"
     );

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<void> {
+  // `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",

src/telemetry/BackgroundLogger.ts

@@ -74,7 +74,7 @@ class BackgroundLogger implements Logger {
 
   async error(error: unknown, data: JsonObject): Promise<void> {
     if (wasContextInvalidated() && !isBackground() && !isDevToolsPage()) {
-      notifyContextInvalidated();
+      void notifyContextInvalidated();
     }
 
     console.error("BackgroundLogger:error", {

src/utils.ts

@@ -535,6 +535,19 @@ export async function logPromiseDuration<P>(
   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<unknown>
+>(title: string, fn: Fn): Promise<ReturnType<Fn>> {
+  const start = Date.now();
+  try {
+    return (await fn()) as Awaited<ReturnType<Fn>>;
+  } finally {
+    // Prefer `debug` level; `console.time` has `log` level
     console.debug(title, `${Math.round(Date.now() - start)}ms`);
   }
 }

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<typeof contexts[number], () => boolean>([
+  ["web", isWebPage],
   ["extension", isExtensionContext],
   ["background", isBackground],
   ["contentScript", isContentScript],

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')})