fix(gallery): release the canvas-mode decode, and drop a now-duplicate sanitizer
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.
This commit is contained in:
@@ -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 };
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
@@ -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]);
|
||||
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user