#6649: Exclude navigation events that should not trigger page updates

jest.config.js

@@ -36,7 +36,7 @@ const config = {
     "^.+\\.txt$": "<rootDir>/src/testUtils/rawJestTransformer.mjs",
   },
   transformIgnorePatterns: [
-    "node_modules/(?!@cfworker|escape-string-regex|filename-reserved-regex|filenamify|idb|webext-|p-timeout|p-retry|p-defer|p-memoize|serialize-error|strip-outer|trim-repeated|mimic-fn|urlpattern-polyfill|url-join|uuid|nanoid|use-debounce|copy-text-to-clipboard|linkify-urls|create-html-element|stringify-attributes|escape-goat|stemmer|uint8array-extras)",
+    "node_modules/(?!@cfworker|escape-string-regex|filename-reserved-regex|filenamify|idb|webext-|p-timeout|p-retry|p-defer|p-memoize|serialize-error|strip-outer|trim-repeated|mimic-fn|urlpattern-polyfill|url-join|uuid|nanoid|use-debounce|copy-text-to-clipboard|linkify-urls|create-html-element|stringify-attributes|escape-goat|stemmer|uint8array-extras|one-event)",
   ],
   setupFiles: [
     "dotenv/config",

package.json

@@ -104,6 +104,7 @@
     "mustache": "^4.2.0",
     "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",

src/background/background.ts

@@ -55,7 +55,7 @@ registerMessenger();
 registerExternalMessenger();
 initBrowserAction();
 initInstaller();
-initNavigation();
+void initNavigation();
 initExecutor();
 initContextMenus();
 initContentScriptReadyListener();

src/background/navigation.ts

@@ -21,9 +21,9 @@ import { type Target } from "@/types/messengerTypes";
 import { canAccessTab } from "@/permissions/permissionsUtils";
 import { isScriptableUrl } from "webext-content-scripts";
 import { debounce } from "lodash";
-import { syncFlagOn } from "@/store/syncFlags";
 import { canAccessTab as canInjectTab, getTabUrl } from "webext-tools";
 import { getTargetState } from "@/contentScript/ready";
+import { flagOn } from "@/auth/authUtils";
 
 export function reactivateEveryTab(): void {
   console.debug("Reactivate all tabs");
@@ -40,33 +40,32 @@ async function traceNavigation(target: Target): Promise<void> {
     ...target,
     tabUrl,
     isScriptableUrl: isScriptableUrl(tabUrl),
-    contentScriptState: await getTargetState(target),
-    canInject: await canInjectTab(target),
-    // PixieBrix has some additional constraints on which tabs can be accessed (i.e., only https:)
-    canAccessTab: await canAccessTab(target),
+    contentScriptState: getTargetState(target),
+    canInject: canInjectTab(target),
+    // PixieBrix has some additional constraints on which tabs can be accessed (i.e., only http/https)
+    canAccessTab: canAccessTab(target),
   });
 }
 
-async function onNavigation({ tabId, frameId }: Target): Promise<void> {
-  if (syncFlagOn("navigation-trace")) {
-    await traceNavigation({ tabId, frameId });
-  }
-}
-
 // Some sites use the hash to encode page state (e.g., filters). There are some non-navigation scenarios where the hash
 // could change frequently (e.g., there is a timer in the state). Debounce to avoid overloading the messenger and
 // contentScript.
-const debouncedOnNavigation = debounce(onNavigation, 100, {
+const debouncedTraceNavigation = debounce(traceNavigation, 100, {
   leading: true,
   trailing: true,
   maxWait: 1000,
 });
 
-function initNavigation(): void {
-  // Let the content script know about navigation from the history API. Required for handling SPA navigation
-  browser.webNavigation.onHistoryStateUpdated.addListener(onNavigation);
+async function initNavigation(): Promise<void> {
+  if (!(await flagOn("navigation-trace"))) {
+    return;
+  }
+
+  browser.webNavigation.onHistoryStateUpdated.addListener(
+    debouncedTraceNavigation,
+  );
   browser.webNavigation.onReferenceFragmentUpdated.addListener(
-    debouncedOnNavigation,
+    debouncedTraceNavigation,
   );
 }
 

src/contentScript/contentScriptCore.ts

@@ -26,7 +26,7 @@ import registerMessenger from "@/contentScript/messenger/registration";
 import registerBuiltinBricks from "@/bricks/registerBuiltinBricks";
 import registerContribBlocks from "@/contrib/registerContribBlocks";
 import brickRegistry from "@/bricks/registry";
-import { handleNavigate, initNavigation } from "@/contentScript/lifecycle";
+import { initNavigation } from "@/contentScript/lifecycle";
 import { initTelemetry } from "@/background/messenger/api";
 import { ENSURE_CONTENT_SCRIPT_READY } from "@/contentScript/ready";
 import { initToaster } from "@/utils/notify";
@@ -65,8 +65,7 @@ export async function init(): Promise<void> {
   initTelemetry();
   initToaster();
 
-  await handleNavigate();
-  initNavigation();
+  void initNavigation();
 
   void initSidebarActivation();
 

src/contentScript/lifecycle.ts

@@ -22,12 +22,13 @@ import * as sidebar from "@/contentScript/sidebarController";
 import { NAVIGATION_RULES } from "@/contrib/navigationRules";
 import { testMatchPatterns } from "@/bricks/available";
 import reportError from "@/telemetry/reportError";
-import { compact, groupBy, intersection, uniq } from "lodash";
+import { compact, debounce, groupBy, intersection, uniq } from "lodash";
+import oneEvent from "one-event";
 import { resolveExtensionInnerDefinitions } from "@/registry/internal";
 import { traces } from "@/background/messenger/api";
 import { isDeploymentActive } from "@/utils/deploymentUtils";
 import { PromiseCancelled } from "@/errors/genericErrors";
-import { type FrameTarget, getThisFrame } from "webext-messenger";
+import { getThisFrame } from "webext-messenger";
 import { type StarterBrick } from "@/types/starterBrickTypes";
 import { type UUID } from "@/types/stringTypes";
 import { type RegistryId } from "@/types/registryTypes";
@@ -77,9 +78,9 @@ const _editorExtensions = new Map<UUID, StarterBrick>();
 const _activeExtensionPoints = new Set<StarterBrick>();
 
 /**
- * Mapping from frame ID to URL. Used to ignore navigation events that don't change the URL.
+ * Used to ignore navigation events that don't change the URL.
  */
-const _frameHref = new Map<number, string>();
+let lastUrl: string | undefined;
 
 /**
  * Abort controllers for navigation events for Single Page Applications (SPAs).
@@ -560,30 +561,18 @@ export async function handleNavigate({
   force,
 }: { force?: boolean } = {}): Promise<void> {
   const runReason = decideRunReason({ force });
-
-  let thisTarget: FrameTarget;
-  try {
-    // Note: We used to check for invalid (undefined) frameId after calling
-    // this, but now getThisFrame will throw an error itself internally if
-    // the frameId is not a valid number. An example situation where this
-    // happens is when the dynamic gsheets code loads a frame within the
-    // extension background page.
-    thisTarget = await getThisFrame();
-  } catch (error: unknown) {
-    console.debug("Ignoring handleNavigate because getThisFrame failed", error);
-    return;
-  }
-
+  const thisTarget = await getThisFrame();
   const { href } = location;
-
-  if (!force && _frameHref.get(thisTarget.frameId) === href) {
-    console.debug("Ignoring NOOP navigation to %s", href, thisTarget);
+  if (!force && lastUrl === href) {
+    console.debug(
+      "handleNavigate:Ignoring NOOP navigation to %s",
+      href,
+      thisTarget,
+    );
     return;
   }
 
-  _frameHref.set(thisTarget.frameId, href);
-
-  console.debug("Handling navigation to %s", href, thisTarget);
+  console.debug("handleNavigate:Handling navigation to %s", href, thisTarget);
   updateNavigationId();
   notifyNavigationListeners();
 
@@ -639,9 +628,41 @@ export async function reactivateTab(): Promise<void> {
   await handleNavigate({ force: true });
 }
 
-export function initNavigation() {
-  window.navigation?.addEventListener("navigate", async (event) => {
-    // Delay slightly to avoid catching events that navigate away from the page
-    setTimeout(handleNavigate, 0);
-  });
+// Ideally we only want to catch local URL changes, but there's no way to discern
+// navigation events that cause the current document to unload in the `navigate ` event.
+async function onNavigate(event: NavigateEvent): Promise<void> {
+  if (
+    // Ignore navigations to external pages
+    !event.destination.url.startsWith(location.origin) ||
+    // Ignore <a download> links
+    event.downloadRequest !== null // Specifically `null` and not `''`
+  ) {
+    return;
+  }
+
+  try {
+    await oneEvent(window, "beforeunload", {
+      signal: AbortSignal.timeout(0),
+    });
+  } catch {
+    // It timed out before the "beforeunload" event, so this is a same-document navigation
+    await handleNavigate();
+  }
+}
+
+export async function initNavigation() {
+  // Initiate PB for the current page
+  await handleNavigate();
+
+  // Listen to page URL changes
+  // Some sites use the hash to encode page state (e.g., filters). There are some non-navigation scenarios
+  // where the hash could change frequently (e.g., there is a timer in the state). Debounce to avoid overloading.
+  window.navigation?.addEventListener(
+    "navigate",
+    debounce(onNavigate, 100, {
+      leading: true,
+      trailing: true,
+      maxWait: 1000,
+    }),
+  );
 }