From 5e675664c65aa67bafc4bcc4830dfc3822877015 Mon Sep 17 00:00:00 2001 From: Mathias Jakobsen Date: Fri, 24 Apr 2026 10:23:14 +0100 Subject: [PATCH] [web] Cleanup PDFjs instances in visual editor (#33022) GitOrigin-RevId: 2aa9ab01f88196fb56dc41749977ca33295c964f --- .../visual/utils/pdf-destroy-lock.ts | 26 +++ .../visual/visual-widgets/graphics.ts | 43 ++++- .../extensions/pdf-destroy-lock.test.ts | 167 ++++++++++++++++++ 3 files changed, 234 insertions(+), 2 deletions(-) create mode 100644 services/web/frontend/js/features/source-editor/extensions/visual/utils/pdf-destroy-lock.ts create mode 100644 services/web/test/frontend/features/source-editor/extensions/pdf-destroy-lock.test.ts diff --git a/services/web/frontend/js/features/source-editor/extensions/visual/utils/pdf-destroy-lock.ts b/services/web/frontend/js/features/source-editor/extensions/visual/utils/pdf-destroy-lock.ts new file mode 100644 index 0000000000..722c866c46 --- /dev/null +++ b/services/web/frontend/js/features/source-editor/extensions/visual/utils/pdf-destroy-lock.ts @@ -0,0 +1,26 @@ +/** + * Coordinates PDF.js document destroys across GraphicsWidget instances. + * + * PDF.js uses a single shared worker (GlobalWorkerOptions.workerPort) and + * rejects getDocument while any previous PDFDocumentLoadingTask.destroy() + * are in flight. + */ +export class PdfDestroyLock { + private readonly pendingDestroys = new Set>() + + /** Resolves once no pdf.destroy() is in flight. */ + async waitForPending(): Promise { + // Loop in case a new destroy is scheduled while we await existing ones. + while (this.pendingDestroys.size > 0) { + await Promise.all(this.pendingDestroys) + } + } + + /** Registers an in-flight destroy so waitForPending() will wait for it. */ + schedule(destroyFn: () => Promise): Promise { + const task = destroyFn() + this.pendingDestroys.add(task) + task.finally(() => this.pendingDestroys.delete(task)) + return task + } +} diff --git a/services/web/frontend/js/features/source-editor/extensions/visual/visual-widgets/graphics.ts b/services/web/frontend/js/features/source-editor/extensions/visual/visual-widgets/graphics.ts index 4b5f22362a..21e61b2b89 100644 --- a/services/web/frontend/js/features/source-editor/extensions/visual/visual-widgets/graphics.ts +++ b/services/web/frontend/js/features/source-editor/extensions/visual/visual-widgets/graphics.ts @@ -4,10 +4,24 @@ import { isEqual } from 'lodash' import { FigureData } from '../../figure-modal' import { debugConsole } from '@/utils/debugging' import { PreviewPath } from '../../../../../../../types/preview-path' +import type { PDFDocumentProxy } from 'pdfjs-dist/types/src/display/api' +import { PdfDestroyLock } from '../utils/pdf-destroy-lock' + +// Module level to synchronize across all GraphicsWidgets +const pdfDestroyLock = new PdfDestroyLock() + +function schedulePdfDestroy(pdf: PDFDocumentProxy) { + pdfDestroyLock.schedule(() => + pdf.destroy().catch(() => { + debugConsole.warn('Failed to destroy PDFjs instance') + }) + ) +} export class GraphicsWidget extends WidgetType { destroyed = false height = 300 // for estimatedHeight, updated when the image is loaded + pdfInstance: PDFDocumentProxy | null = null constructor( public filePath: string, @@ -54,6 +68,10 @@ export class GraphicsWidget extends WidgetType { ) { return true } + if (this.pdfInstance) { + schedulePdfDestroy(this.pdfInstance) + this.pdfInstance = null + } this.renderGraphic(element, view) view.requestMeasure() return true @@ -72,6 +90,10 @@ export class GraphicsWidget extends WidgetType { destroy() { this.destroyed = true + if (this.pdfInstance) { + schedulePdfDestroy(this.pdfInstance) + this.pdfInstance = null + } } coordsAt(element: HTMLElement) { @@ -104,7 +126,11 @@ export class GraphicsWidget extends WidgetType { { const canvas = document.createElement('canvas') canvas.classList.add('ol-cm-graphics') - this.renderPDF(view, canvas, preview.url).catch(debugConsole.error) + this.renderPDF(view, canvas, preview.url).catch(err => { + if (!this.destroyed) { + debugConsole.error('Failed to render PDF graphics widget', err) + } + }) element.append(canvas) } break @@ -249,10 +275,23 @@ export class GraphicsWidget extends WidgetType { return } + await pdfDestroyLock.waitForPending() + if (this.destroyed) { + return + } + const pdf = await loadPdfDocumentFromUrl(url).promise - const page = await pdf.getPage(1) + this.pdfInstance = pdf // bail out if loading the PDF took too long + if (this.destroyed) { + schedulePdfDestroy(pdf) + this.pdfInstance = null + return + } + + const page = await pdf.getPage(1) + if (this.destroyed) { return } diff --git a/services/web/test/frontend/features/source-editor/extensions/pdf-destroy-lock.test.ts b/services/web/test/frontend/features/source-editor/extensions/pdf-destroy-lock.test.ts new file mode 100644 index 0000000000..581d2b694b --- /dev/null +++ b/services/web/test/frontend/features/source-editor/extensions/pdf-destroy-lock.test.ts @@ -0,0 +1,167 @@ +import { PdfDestroyLock } from '@/features/source-editor/extensions/visual/utils/pdf-destroy-lock' +import { expect } from 'chai' + +type Deferred = { + promise: Promise + resolve: (value: T) => void + reject: (err: unknown) => void +} + +function defer(): Deferred { + let resolveRet!: (value: T) => void + let rejectRet!: (err: unknown) => void + const promise = new Promise((resolve, reject) => { + resolveRet = resolve + rejectRet = reject + }) + return { promise, resolve: resolveRet, reject: rejectRet } +} + +// Flush microtasks so that .finally() handlers attached inside schedule() +// run before we make assertions. +const flush = () => new Promise(resolve => setTimeout(resolve, 0)) + +describe('PdfDestroyLock', function () { + describe('waitForPending', function () { + it('resolves immediately when no destroys are in flight', async function () { + const lock = new PdfDestroyLock() + let resolved = false + await lock.waitForPending().then(() => { + resolved = true + }) + expect(resolved).to.be.true + }) + + it('waits for a pending destroy to settle', async function () { + const lock = new PdfDestroyLock() + const deferred = defer() + lock.schedule(() => deferred.promise) + + let waited = false + const wait = lock.waitForPending().then(() => { + waited = true + }) + + await flush() + expect(waited).to.be.false + + deferred.resolve() + await wait + expect(waited).to.be.true + }) + + it('waits for a destroy that rejects', async function () { + const lock = new PdfDestroyLock() + const deferred = defer() + // Swallow the rejection at the schedule() call site to mirror real usage, + // where callers catch errors from destroy(). + lock.schedule(() => deferred.promise.catch(() => {})) + + let waited = false + const wait = lock.waitForPending().then(() => { + waited = true + }) + + await flush() + expect(waited).to.be.false + + deferred.reject(new Error('boom')) + await wait + expect(waited).to.be.true + }) + + it('waits for destroys scheduled while waiting', async function () { + const lock = new PdfDestroyLock() + const first = defer() + const second = defer() + + lock.schedule(() => first.promise) + + let waited = false + const wait = lock.waitForPending().then(() => { + waited = true + }) + + // Schedule a second destroy before the first completes - waitForPending + // must loop and wait for this one too. + lock.schedule(() => second.promise) + + first.resolve() + await flush() + expect(waited).to.be.false + + second.resolve() + await wait + expect(waited).to.be.true + }) + + it('waits for multiple concurrent destroys', async function () { + const lock = new PdfDestroyLock() + const first = defer() + const second = defer() + + lock.schedule(() => first.promise) + lock.schedule(() => second.promise) + + let waited = false + const wait = lock.waitForPending().then(() => { + waited = true + }) + + first.resolve() + await flush() + expect(waited).to.be.false + + second.resolve() + await wait + expect(waited).to.be.true + }) + }) + + describe('schedule', function () { + it('invokes the destroy function immediately', function () { + const lock = new PdfDestroyLock() + let called = false + lock.schedule(async () => { + called = true + }) + expect(called).to.be.true + }) + + it('returns the promise from the destroy function', async function () { + const lock = new PdfDestroyLock() + const result = await lock.schedule(async () => 'done') + expect(result).to.equal('done') + }) + + it('releases the lock after a successful destroy', async function () { + const lock = new PdfDestroyLock() + await lock.schedule(async () => {}) + await flush() + + // waitForPending should now resolve synchronously - if the task was not + // removed from the set, this would hang. + let resolved = false + await lock.waitForPending().then(() => { + resolved = true + }) + expect(resolved).to.be.true + }) + + it('releases the lock after a destroy that rejects', async function () { + const lock = new PdfDestroyLock() + // Mirror real usage: destroyFn swallows its own errors so the internal + // .finally() chain in schedule() doesn't leak an unhandled rejection. + await lock.schedule(() => + Promise.reject(new Error('boom')).catch(() => {}) + ) + await flush() + + let resolved = false + await lock.waitForPending().then(() => { + resolved = true + }) + expect(resolved).to.be.true + }) + }) +})