From be8d79e9c4b6148a0b3f8a1f81f05fbf5fbf6880 Mon Sep 17 00:00:00 2001
From: Paul Nothaft <paul@MacStudio-von-Paul.local>
Date: Fri, 4 Sep 2026 20:39:43 +0200
Subject: [PATCH] fix(gallery): release the canvas-mode decode, and drop a
 now-duplicate sanitizer
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Two follow-ups to yesterday's merges. Both were already known; neither
depends on the open question in #1287.

1. Canvas mode pinned every decoded image for the component's lifetime.

`AuthenticatedImage` keeps a detached Image in `imageRef` so drawToCanvas
can read it. The effect cleanup nulled onload/onerror and never cleared
that ref, so the Image — and the decode behind it — stayed held by a live
JS reference. A decoded <img> in the document is evictable under memory
pressure; one held by a ref is not.

That is not academic at gallery scale. The photo grid is NOT virtualised,
so a 546-photo event mounts 546 of these and none ever unmount — nothing
was ever released. The ref is cleared and the src dropped, so the browser
can reclaim without waiting for GC.

This is NOT presented as the fix for #1287. That investigation is still
open: the reporter has since shown the backend idle during a stall and
the renderer itself unresponsive for 45s, which rules out the theories
tried so far. This is a real leak on the same path, worth fixing on its
own terms while that question is settled.

2. newsletterService no longer carries its own remote-url() stripper.

It was added because the shared sanitizeCSS "blocked" remote URLs with a
CSS comment that parsers discard. #1290 replaced that with a lexer, so
the local copy is dead weight — and two definitions of "disallowed" would
drift apart. Verified the shared function covers every case the local one
did, including the quoted-paren and CSS-escape forms found in review.

Three tests on the release path, two of which fail without the fix:
unmount clears the ref and drops the src, the blob URL is revoked, and a
src change releases the previous image rather than accumulating one
pinned decode per photo a recycled tile has shown.
---
 backend/src/services/newsletterService.js     |  36 ++----
 .../components/common/AuthenticatedImage.tsx  |  14 +++
 .../AuthenticatedImage.canvasRelease.test.tsx | 104 ++++++++++++++++++
 3 files changed, 126 insertions(+), 28 deletions(-)
 create mode 100644 frontend/src/components/common/__tests__/AuthenticatedImage.canvasRelease.test.tsx

diff --git a/backend/src/services/newsletterService.js b/backend/src/services/newsletterService.js
index 46da3528..eaec1f76 100644
--- a/backend/src/services/newsletterService.js
+++ b/backend/src/services/newsletterService.js
@@ -141,44 +141,24 @@ function sanitizeCampaignBody(html) {
     },
   })
     // sanitize-html keeps the style ATTRIBUTE contents verbatim. Clean each.
+    // sanitizeCSS blocks remote url() properly as of #1290 — it lexes the CSS
+    // rather than pattern-matching it, so the local pass this used to need is
+    // gone. Keeping a second copy would mean two definitions of "disallowed"
+    // drifting apart.
     .replace(/style="([^"]*)"/gi, (match, css) => {
       const { sanitized } = sanitizeCSS(css);
-      const cleaned = stripRemoteCssUrls(sanitized);
-      return cleaned ? `style="${cleaned.replace(/"/g, '')}"` : '';
+      return sanitized ? `style="${sanitized.replace(/"/g, '')}"` : '';
     });
 }
 
-/**
- * Remove every `url(...)` that is not an inline data: image.
- *
- * The shared `sanitizeCSS` *detects* a remote url() and prefixes it with a
- * `/* BLOCKED URL *\/` comment — but a CSS comment is stripped during
- * tokenization, so the declaration a mail client actually parses still
- * carries the live URL. Verified:
- *
- *   sanitizeCSS('.a{background:url(https://x/p.gif)}').sanitized
- *     → '.a{background:/* BLOCKED URL *\/ url(https://x/p.gif)}'
- *
- * In a newsletter that is a tracking pixel delivered to every recipient, so
- * this pass actually removes the token. Scoped to the newsletter path on
- * purpose: the same weakness affects gallery custom CSS, but changing shared
- * sanitizer behaviour is a separate change with its own blast radius.
- */
-function stripRemoteCssUrls(css) {
-  if (!css) return '';
-  return String(css)
-    .replace(/\/\*\s*BLOCKED URL\s*\*\//gi, '')
-    .replace(/url\s*\(\s*(['"]?)([^)'"]*)\1\s*\)/gi, (match, _quote, target) =>
-      (/^data:image\/(?:jpeg|jpg|png|gif|webp)/i.test(target.trim()) ? match : 'none'));
-}
-
 /**
  * Sanitize a campaign's optional `<style>` block. Delegates to the shared
  * cssSanitizer, which already blocks `@import`, `expression(`, `behavior:`,
  * `javascript:` and every `url()` that is not a `data:` image.
  *
  * That is STRICTER than the issue's "https: images only" note — the shared
- * sanitizer allows no remote `url()` at all. Kept as-is rather than loosened:
+ * sanitizer allows no remote `url()` at all, and since #1290 it enforces
+ * that by lexing rather than by pattern-matching. Kept as-is rather than loosened:
  * a remote CSS url() in mail is a tracking pixel by another name, and a
  * campaign's images belong in `<img>` tags where the scheme filter sees them.
  *
@@ -187,7 +167,7 @@ function stripRemoteCssUrls(css) {
 function sanitizeCampaignCss(css) {
   if (!css) return { css: '', warnings: [] };
   const { sanitized, warnings } = sanitizeCSS(String(css));
-  return { css: stripRemoteCssUrls(sanitized), warnings };
+  return { css: sanitized, warnings };
 }
 
 // ---------------------------------------------------------------------------
diff --git a/frontend/src/components/common/AuthenticatedImage.tsx b/frontend/src/components/common/AuthenticatedImage.tsx
index bd3bc8fe..130da0be 100644
--- a/frontend/src/components/common/AuthenticatedImage.tsx
+++ b/frontend/src/components/common/AuthenticatedImage.tsx
@@ -286,6 +286,20 @@ export const AuthenticatedImage: React.FC<AuthenticatedImageProps> = ({
     return () => {
       img.onload = null;
       img.onerror = null;
+      // Release the decoded bitmap (#1287). `imageRef` is what drawToCanvas
+      // reads, and it was never cleared — so a detached Image, and the
+      // decode behind it, stayed pinned by a live JS reference for the
+      // lifetime of the component. A decoded <img> in the document is
+      // evictable under memory pressure; one held by a ref is not.
+      //
+      // This matters at gallery scale because the grid is not virtualised:
+      // a 546-photo event mounts 546 of these and none of them ever unmount,
+      // so nothing was ever released. Dropping the src first lets the
+      // browser reclaim the decode without waiting for GC to notice.
+      if (imageRef.current === img) {
+        imageRef.current = null;
+      }
+      img.removeAttribute('src');
     };
   }, [imageSrc, useCanvasRendering, drawToCanvas, onLoad]);
 
diff --git a/frontend/src/components/common/__tests__/AuthenticatedImage.canvasRelease.test.tsx b/frontend/src/components/common/__tests__/AuthenticatedImage.canvasRelease.test.tsx
new file mode 100644
index 00000000..a8b6ffd0
--- /dev/null
+++ b/frontend/src/components/common/__tests__/AuthenticatedImage.canvasRelease.test.tsx
@@ -0,0 +1,104 @@
+/**
+ * Canvas-mode memory release (#1287).
+ *
+ * In canvas mode the component keeps a detached `Image` in `imageRef` so
+ * `drawToCanvas` can read it. The effect cleanup nulled `onload`/`onerror`
+ * but never cleared that ref, so the Image — and the decoded bitmap behind
+ * it — stayed pinned by a live JS reference for the component's lifetime.
+ *
+ * That is not academic at gallery scale. The photo grid is NOT virtualised:
+ * a 546-photo event mounts 546 of these and none ever unmount, so nothing was
+ * ever released. A decoded <img> in the document is evictable under memory
+ * pressure; one held by a ref is not.
+ */
+import { render, waitFor } from '@testing-library/react';
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+
+vi.mock('../../../utils/galleryAuthStorage', () => ({
+  getActiveGallerySlug: () => 'demo',
+  getGalleryToken: () => 'token',
+  inferGallerySlugFromLocation: () => 'demo',
+  resolveSlugFromRequestUrl: () => 'demo',
+}));
+vi.mock('../../../utils/url', () => ({ buildResourceUrl: (u: string) => `http://localhost${u}` }));
+
+import { AuthenticatedImage } from '../AuthenticatedImage';
+
+/** Every Image the component constructs, so the test can inspect them. */
+const created: HTMLImageElement[] = [];
+let createObjectURL: ReturnType<typeof vi.fn>;
+let revokeObjectURL: ReturnType<typeof vi.fn>;
+
+beforeEach(() => {
+  created.length = 0;
+  vi.stubGlobal('fetch', vi.fn(async () => ({
+    ok: true,
+    blob: async () => new Blob(['x'], { type: 'image/png' }),
+  })));
+  // Patch the two methods rather than replacing URL — spreading the
+  // constructor loses its prototype and breaks every `new URL(...)`.
+  // Unique per call: the canvas effect keys off `imageSrc`, so a constant
+  // URL would make a src change look like no change at all.
+  let n = 0;
+  createObjectURL = vi.fn(() => `blob:mock-url-${++n}`);
+  revokeObjectURL = vi.fn();
+  URL.createObjectURL = createObjectURL as unknown as typeof URL.createObjectURL;
+  URL.revokeObjectURL = revokeObjectURL as unknown as typeof URL.revokeObjectURL;
+
+  const RealImage = globalThis.Image;
+  vi.stubGlobal('Image', class extends RealImage {
+    constructor() {
+      super();
+      created.push(this as unknown as HTMLImageElement);
+      // jsdom never fires load for a blob: src, so drive it manually.
+      setTimeout(() => this.onload?.(new Event('load')), 0);
+    }
+  });
+});
+
+afterEach(() => vi.unstubAllGlobals());
+
+describe('AuthenticatedImage canvas mode', () => {
+  it('releases the decoded image on unmount', async () => {
+    const { unmount } = render(
+      <AuthenticatedImage src="/api/gallery/demo/thumbnail/1" alt="t" useCanvasRendering />
+    );
+
+    await waitFor(() => expect(created.length).toBeGreaterThan(0));
+    const img = created[0];
+
+    unmount();
+
+    // The src is dropped so the browser can reclaim the decode without
+    // waiting for GC, and the handlers are detached.
+    expect(img.getAttribute('src')).toBeNull();
+    expect(img.onload).toBeNull();
+    expect(img.onerror).toBeNull();
+  });
+
+  it('revokes the blob URL on unmount', async () => {
+    const { unmount } = render(
+      <AuthenticatedImage src="/api/gallery/demo/thumbnail/1" alt="t" useCanvasRendering />
+    );
+
+    await waitFor(() => expect(created.length).toBeGreaterThan(0));
+    unmount();
+
+    expect(revokeObjectURL).toHaveBeenCalledWith('blob:mock-url-1');
+  });
+
+  it('releases the previous image when the src changes', async () => {
+    // A recycled tile (a layout reusing a component instance for a different
+    // photo) must not accumulate one pinned decode per photo it has shown.
+    const { rerender } = render(
+      <AuthenticatedImage src="/api/gallery/demo/thumbnail/1" alt="t" useCanvasRendering />
+    );
+    await waitFor(() => expect(created.length).toBe(1));
+    const first = created[0];
+
+    rerender(<AuthenticatedImage src="/api/gallery/demo/thumbnail/2" alt="t" useCanvasRendering />);
+    await waitFor(() => expect(created.length).toBe(2));
+
+    expect(first.getAttribute('src')).toBeNull();
+  });
+});