From 84059825e3cbcfe2cc698e6dc45bcdee9c70d172 Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Wed, 28 Oct 2026 02:56:26 -0700 Subject: [PATCH] fix(export): cancel PDF work when requesting document retires (#34907) * fix(export): cancel PDF work when requesting document retires * test(export): assert success on non-retired repeated export cycles --------- Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> --- src/main/ipc/export.test.ts | 362 +++++++++++++++++++++++++++++++ src/main/ipc/export.ts | 12 +- src/main/lib/html-to-pdf.test.ts | 55 +++++ src/main/lib/html-to-pdf.ts | 49 ++++- 4 files changed, 469 insertions(+), 9 deletions(-) create mode 100644 src/main/ipc/export.test.ts diff --git a/src/main/ipc/export.test.ts b/src/main/ipc/export.test.ts new file mode 100644 index 00000000000..03183517ab3 --- /dev/null +++ b/src/main/ipc/export.test.ts @@ -0,0 +1,362 @@ +import { EventEmitter } from 'node:events' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { ExportHtmlToPdfArgs, ExportHtmlToPdfResult } from './export' + +type ExportHandler = ( + event: { sender: EventEmitter }, + args: ExportHtmlToPdfArgs +) => Promise + +const fake = vi.hoisted(() => ({ + handlers: new Map(), + tempFiles: new Set(), + savedFiles: new Map(), + windows: new Array<{ events: EventEmitter; destroyed: boolean }>(), + uuid: 0, + write: + vi.fn< + (path: string, data: string | Uint8Array, options?: { signal?: AbortSignal }) => Promise + >(), + load: vi.fn<(events: EventEmitter, path: string) => Promise>(), + images: vi.fn<() => Promise>(), + print: vi.fn<() => Promise>(), + dialog: vi.fn<() => Promise<{ canceled: boolean; filePath?: string }>>() +})) + +vi.mock('electron', () => ({ + app: { getPath: () => '/mock-export-temp' }, + ipcMain: { + removeHandler: () => { + fake.handlers.delete('export:html-to-pdf') + }, + handle: (_channel: string, handler: ExportHandler) => { + fake.handlers.set('export:html-to-pdf', handler) + } + }, + dialog: { showSaveDialog: fake.dialog }, + BrowserWindow: class { + static fromWebContents() { + return { id: 1 } + } + events = new EventEmitter() + destroyed = false + webContents = Object.assign(this.events, { + executeJavaScript: fake.images, + printToPDF: fake.print + }) + constructor() { + fake.windows.push(this) + } + loadFile(path: string) { + return fake.load(this.events, path) + } + isDestroyed() { + return this.destroyed + } + destroy() { + this.destroyed = true + } + } +})) +vi.mock('node:fs/promises', () => ({ + writeFile: fake.write, + unlink: async (path: string) => { + fake.tempFiles.delete(path) + } +})) +vi.mock('node:crypto', () => ({ randomUUID: () => `export-${++fake.uuid}` })) + +import { registerExportHandlers } from './export' + +const PDF = Buffer.from('exported PDF snapshot') +const ARGS = { html: '

captured markdown

', title: 'draft' } +const GONE_EVENTS = ['render-process-gone', 'did-navigate', 'destroyed'] as const + +function exportFrom(sender: EventEmitter): Promise { + const handler = fake.handlers.get('export:html-to-pdf') + if (!handler) { + throw new Error('Export handler was not registered') + } + return handler({ sender }, ARGS) +} + +async function flush(): Promise { + await vi.advanceTimersByTimeAsync(0) +} + +function expectReleased(sender: EventEmitter): void { + expect(fake.tempFiles.size).toBe(0) + expect(fake.windows.every((window) => window.destroyed)).toBe(true) + expect(vi.getTimerCount()).toBe(0) + for (const event of GONE_EVENTS) { + expect(sender.listenerCount(event)).toBe(0) + } + for (const window of fake.windows) { + expect(window.events.listenerCount('did-finish-load')).toBe(0) + expect(window.events.listenerCount('did-fail-load')).toBe(0) + } +} + +beforeEach(() => { + vi.useFakeTimers() + vi.resetAllMocks() + fake.tempFiles.clear() + fake.savedFiles.clear() + fake.windows.length = 0 + fake.write.mockImplementation(async (path, data) => { + if (typeof data === 'string') { + fake.tempFiles.add(path) + } else { + fake.savedFiles.set(path, data) + } + }) + fake.load.mockImplementation(async (events) => { + events.emit('did-finish-load') + }) + fake.images.mockResolvedValue() + fake.print.mockResolvedValue(PDF) + fake.dialog.mockResolvedValue({ canceled: true }) + registerExportHandlers() +}) + +afterEach(() => { + vi.useRealTimers() +}) + +describe('PDF export requesting document lifetime', () => { + it.each(GONE_EVENTS)('releases a hidden export on %s before images settle', async (event) => { + const images = Promise.withResolvers() + fake.images.mockReturnValueOnce(images.promise) + const sender = new EventEmitter() + const result = exportFrom(sender) + await flush() + expect(fake.tempFiles.size).toBe(1) + expect(fake.windows).toHaveLength(1) + + sender.emit(event) + await flush() + expectReleased(sender) + await expect(result).resolves.toEqual({ success: false, cancelled: true }) + expectReleased(sender) + images.resolve() + await flush() + expect(fake.print).not.toHaveBeenCalled() + expect(fake.dialog).not.toHaveBeenCalled() + }) + + it.each(['load', 'load-event', 'images', 'print'] as const)( + 'cleans up pending %s and observes its late rejection', + async (stage) => { + const pending = Promise.withResolvers() + const printing = Promise.withResolvers() + if (stage === 'load') { + fake.load.mockReturnValueOnce(pending.promise) + } + if (stage === 'load-event') { + fake.load.mockResolvedValueOnce() + } + if (stage === 'images') { + fake.images.mockReturnValueOnce(pending.promise) + } + if (stage === 'print') { + fake.print.mockReturnValueOnce(printing.promise) + } + const sender = new EventEmitter() + const result = exportFrom(sender) + await flush() + sender.emit('render-process-gone') + await flush() + expectReleased(sender) + await expect(result).resolves.toEqual({ success: false, cancelled: true }) + expectReleased(sender) + + if (stage === 'print') { + printing.reject(new Error('late print failure')) + } else if (stage === 'load-event') { + fake.windows[0]?.events.emit('did-fail-load', {}, -3, 'late failure') + } else { + pending.reject(new Error('late renderer failure')) + } + await flush() + expect(fake.dialog).not.toHaveBeenCalled() + expect(fake.print).toHaveBeenCalledTimes(stage === 'print' ? 1 : 0) + } + ) + + it.each(['resolve', 'reject'] as const)( + 'cancels initial document writing and cleans up after late %s', + async (settlement) => { + const writing = Promise.withResolvers() + fake.write.mockImplementationOnce(async (path) => { + fake.tempFiles.add(path) + await writing.promise + }) + const sender = new EventEmitter() + const result = exportFrom(sender) + await flush() + const signal = fake.write.mock.calls[0]?.[2]?.signal + expect(signal?.aborted).toBe(false) + sender.emit('did-navigate') + expect(signal?.aborted).toBe(true) + expect(fake.windows).toHaveLength(0) + expect(fake.tempFiles.size).toBe(1) + + if (settlement === 'resolve') { + writing.resolve() + } else { + writing.reject(new Error('write aborted')) + } + await expect(result).resolves.toEqual({ success: false, cancelled: true }) + expectReleased(sender) + expect(fake.windows).toHaveLength(0) + expect(fake.dialog).not.toHaveBeenCalled() + } + ) + + it('prevents a late load resolution from admitting image or print work', async () => { + const load = Promise.withResolvers() + fake.load.mockReturnValueOnce(load.promise) + const sender = new EventEmitter() + const result = exportFrom(sender) + await flush() + sender.emit('destroyed') + await flush() + expectReleased(sender) + await expect(result).resolves.toEqual({ success: false, cancelled: true }) + load.resolve() + fake.windows[0]?.events.emit('did-finish-load') + await flush() + expectReleased(sender) + expect(fake.images).not.toHaveBeenCalled() + expect(fake.print).not.toHaveBeenCalled() + }) + + it('keeps a replacement document and another requester independent of late old work', async () => { + const images = Promise.withResolvers() + const otherImages = Promise.withResolvers() + fake.images.mockReturnValueOnce(images.promise).mockReturnValueOnce(otherImages.promise) + const sender = new EventEmitter() + const oldResult = exportFrom(sender) + await flush() + const other = new EventEmitter() + const otherResult = exportFrom(other) + await flush() + sender.emit('did-navigate') + await flush() + expect(fake.windows.map((window) => window.destroyed)).toEqual([true, false]) + expect(fake.tempFiles.size).toBe(1) + expect(other.listenerCount('render-process-gone')).toBe(1) + await expect(oldResult).resolves.toEqual({ success: false, cancelled: true }) + fake.dialog.mockResolvedValueOnce({ canceled: false, filePath: '/chosen/new.pdf' }) + fake.dialog.mockResolvedValueOnce({ canceled: false, filePath: '/chosen/other.pdf' }) + await expect(exportFrom(sender)).resolves.toEqual({ + success: true, + filePath: '/chosen/new.pdf' + }) + otherImages.resolve() + await expect(otherResult).resolves.toEqual({ + success: true, + filePath: '/chosen/other.pdf' + }) + images.resolve() + await flush() + expect(fake.print).toHaveBeenCalledTimes(2) + expect(fake.dialog).toHaveBeenCalledTimes(2) + expect(fake.savedFiles).toEqual( + new Map([ + ['/chosen/new.pdf', PDF], + ['/chosen/other.pdf', PDF] + ]) + ) + expectReleased(sender) + expectReleased(other) + }) + + it('preserves captured content through tab/file switches and same-document navigation', async () => { + const images = Promise.withResolvers() + fake.images.mockReturnValueOnce(images.promise) + fake.dialog.mockResolvedValueOnce({ canceled: false, filePath: '/chosen/snapshot.pdf' }) + const sender = new EventEmitter() + const result = exportFrom(sender) + await flush() + sender.emit('did-navigate-in-page') + // Tab/file selection is renderer-local; the captured HTML request remains owned by this document. + images.resolve() + await expect(result).resolves.toEqual({ success: true, filePath: '/chosen/snapshot.pdf' }) + expect(fake.savedFiles.get('/chosen/snapshot.pdf')).toBe(PDF) + expectReleased(sender) + }) + + it('does not write a save choice returned after the requesting document retired', async () => { + const choice = Promise.withResolvers<{ canceled: boolean; filePath: string }>() + fake.dialog.mockReturnValueOnce(choice.promise) + const sender = new EventEmitter() + const result = exportFrom(sender) + await flush() + expect(fake.dialog).toHaveBeenCalledOnce() + sender.emit('render-process-gone') + choice.resolve({ canceled: false, filePath: '/chosen/late.pdf' }) + await expect(result).resolves.toEqual({ success: false, cancelled: true }) + expect(fake.savedFiles.size).toBe(0) + expectReleased(sender) + }) + + it('finishes a user-accepted write that started before retirement', async () => { + const writing = Promise.withResolvers() + fake.dialog.mockResolvedValueOnce({ canceled: false, filePath: '/chosen/accepted.pdf' }) + fake.write.mockImplementation(async (path, data) => { + if (typeof data === 'string') { + fake.tempFiles.add(path) + return + } + await writing.promise + fake.savedFiles.set(path, data) + }) + const sender = new EventEmitter() + const result = exportFrom(sender) + await flush() + expect(fake.write).toHaveBeenCalledWith('/chosen/accepted.pdf', PDF) + sender.emit('destroyed') + writing.resolve() + await expect(result).resolves.toEqual({ success: true, filePath: '/chosen/accepted.pdf' }) + expect(fake.savedFiles.get('/chosen/accepted.pdf')).toBe(PDF) + expectReleased(sender) + }) + + it('releases listeners and resources across repeated successful and retired exports', async () => { + const sender = new EventEmitter() + for (let index = 0; index < 10; index++) { + const retired = index % 2 === 0 + const filePath = `/chosen/${index}.pdf` + const images = Promise.withResolvers() + fake.images.mockReturnValueOnce(images.promise) + if (!retired) { + fake.dialog.mockResolvedValueOnce({ canceled: false, filePath }) + } + const prints = fake.print.mock.calls.length + const dialogs = fake.dialog.mock.calls.length + const result = exportFrom(sender) + await flush() + if (retired) { + sender.emit('render-process-gone') + } else { + images.resolve() + } + await flush() + expectReleased(sender) + if (retired) { + await expect(result).resolves.toEqual({ success: false, cancelled: true }) + expect(fake.print.mock.calls.length).toBe(prints) + expect(fake.dialog.mock.calls.length).toBe(dialogs) + expect(fake.savedFiles.has(filePath)).toBe(false) + } else { + await expect(result).resolves.toEqual({ success: true, filePath }) + expect(fake.print.mock.calls.length).toBe(prints + 1) + expect(fake.savedFiles.get(filePath)).toEqual(PDF) + } + images.reject(new Error('late image failure')) + await flush() + expectReleased(sender) + } + }) +}) diff --git a/src/main/ipc/export.ts b/src/main/ipc/export.ts index 7b8c944791f..f7205dab33e 100644 --- a/src/main/ipc/export.ts +++ b/src/main/ipc/export.ts @@ -1,6 +1,7 @@ import { BrowserWindow, dialog, ipcMain } from 'electron' import { writeFile } from 'node:fs/promises' import { ExportTimeoutError, htmlToPdf } from '../lib/html-to-pdf' +import { abortWhenRendererGone } from './renderer-lifetime-abort' export type ExportHtmlToPdfArgs = { html: string @@ -21,8 +22,10 @@ export function registerExportHandlers(): void { return { success: false, error: 'No content to export' } } + const lifetime = abortWhenRendererGone(event.sender) try { - const pdfBuffer = await htmlToPdf(html) + const pdfBuffer = await htmlToPdf(html, lifetime.signal) + lifetime.signal.throwIfAborted() // Keep long titles and Windows-forbidden characters out of suggested save filenames. const sanitizedTitle = title.replace(/[/\\:*?"<>|]/g, '_').slice(0, 100) || 'export' @@ -37,13 +40,18 @@ export function registerExportHandlers(): void { ? await dialog.showSaveDialog(parent, dialogOptions) : await dialog.showSaveDialog(dialogOptions) + lifetime.signal.throwIfAborted() if (canceled || !filePath) { return { success: false, cancelled: true } } + lifetime.dispose() await writeFile(filePath, pdfBuffer) return { success: true, filePath } } catch (error) { + if (lifetime.signal.aborted) { + return { success: false, cancelled: true } + } if (error instanceof ExportTimeoutError) { return { success: false, error: 'Export timed out' } } @@ -51,6 +59,8 @@ export function registerExportHandlers(): void { success: false, error: error instanceof Error ? error.message : 'Failed to export PDF' } + } finally { + lifetime.dispose() } } ) diff --git a/src/main/lib/html-to-pdf.test.ts b/src/main/lib/html-to-pdf.test.ts index 03ead7c66f1..96e739ccf76 100644 --- a/src/main/lib/html-to-pdf.test.ts +++ b/src/main/lib/html-to-pdf.test.ts @@ -25,6 +25,11 @@ vi.mock('electron', () => ({ once: (event: string, listener: (...args: unknown[]) => void) => { fake.handlers.set(event, listener) }, + removeListener: (event: string, listener: (...args: unknown[]) => void) => { + if (fake.handlers.get(event) === listener) { + fake.handlers.delete(event) + } + }, executeJavaScript: fake.images, printToPDF: fake.print } @@ -67,6 +72,56 @@ afterEach(() => { }) describe('htmlToPdf resource ownership', () => { + it('does not create a document for an already-aborted request', async () => { + const controller = new AbortController() + const error = new Error('request retired') + controller.abort(error) + + await expect(htmlToPdf(HTML, controller.signal)).rejects.toBe(error) + + expect(fake.write).not.toHaveBeenCalled() + expect(fake.construct).not.toHaveBeenCalled() + expect(vi.getTimerCount()).toBe(0) + }) + + it.each(['success', 'failure', 'abort'] as const)( + 'detaches conversion listeners after %s', + async (outcome) => { + const controller = new AbortController() + const added = vi.spyOn(controller.signal, 'addEventListener') + const removed = vi.spyOn(controller.signal, 'removeEventListener') + const images = Promise.withResolvers() + fake.images.mockReturnValueOnce(images.promise) + const error = new Error('request retired or renderer failed') + const result = htmlToPdf(HTML, controller.signal) + const assertion = + outcome === 'success' + ? expect(result).resolves.toBe(PDF) + : expect(result).rejects.toBe(error) + await vi.advanceTimersByTimeAsync(0) + expect(added).toHaveBeenCalledWith('abort', expect.any(Function), { once: true }) + + if (outcome === 'abort') { + controller.abort(error) + } else if (outcome === 'failure') { + images.reject(error) + } else { + images.resolve() + } + await assertion + + const listener = added.mock.calls.find(([event]) => event === 'abort')?.[1] + expect(listener).toBeTypeOf('function') + expect(removed).toHaveBeenCalledWith('abort', listener) + expect(fake.handlers.size).toBe(0) + expect(fake.files.size).toBe(0) + expect(fake.destroy).toHaveBeenCalledOnce() + expect(vi.getTimerCount()).toBe(0) + images.reject(new Error('late image failure')) + await Promise.resolve() + } + ) + it('removes the temporary document when window construction throws', async () => { const error = new Error('window construction failed') fake.construct.mockImplementation(() => { diff --git a/src/main/lib/html-to-pdf.ts b/src/main/lib/html-to-pdf.ts index 424d94c5578..440d0a46ded 100644 --- a/src/main/lib/html-to-pdf.ts +++ b/src/main/lib/html-to-pdf.ts @@ -27,13 +27,18 @@ new Promise((resolve) => { }) ` -export async function htmlToPdf(html: string): Promise { +export async function htmlToPdf(html: string, signal?: AbortSignal): Promise { + signal?.throwIfAborted() const tempDir = app.getPath('temp') const tempPath = path.join(tempDir, `orca-export-${randomUUID()}.html`) // Exclusive creation prevents writing through pre-seated symlinks; EEXIST paths belong to someone else. try { - await writeFile(tempPath, html, { encoding: 'utf-8', flag: 'wx' }) + await writeFile(tempPath, html, { + encoding: 'utf-8', + flag: 'wx', + ...(signal ? { signal } : {}) + }) } catch (error) { if (isAlreadyExists(error)) { throw error @@ -43,6 +48,7 @@ export async function htmlToPdf(html: string): Promise { } try { + signal?.throwIfAborted() const win = new BrowserWindow({ show: false, webPreferences: { @@ -55,24 +61,49 @@ export async function htmlToPdf(html: string): Promise { }) let timer: NodeJS.Timeout | undefined + let removeAbortListener: (() => void) | undefined + let removeLoadListeners: (() => void) | undefined try { + const abortPromise = new Promise((_resolve, reject) => { + if (!signal) { + return + } + const abort = (): void => reject(signal.reason) + signal.addEventListener('abort', abort, { once: true }) + removeAbortListener = () => signal.removeEventListener('abort', abort) + if (signal.aborted) { + abort() + } + }) // Bound loading too: a non-yielding document script never fires load completion or failure. const timeoutPromise = new Promise((_resolve, reject) => { timer = setTimeout(() => reject(new ExportTimeoutError()), EXPORT_TIMEOUT_MS) }) const loadAndPrint = (async (): Promise => { + signal?.throwIfAborted() const loadPromise = new Promise((resolve, reject) => { - win.webContents.once('did-finish-load', () => resolve()) - win.webContents.once('did-fail-load', (_event, errorCode, errorDescription) => { + const finish = (): void => resolve() + const fail = ( + _event: Electron.Event, + errorCode: number, + errorDescription: string + ): void => { reject(new Error(`Failed to load export document: ${errorDescription} (${errorCode})`)) - }) + } + win.webContents.once('did-finish-load', finish) + win.webContents.once('did-fail-load', fail) + removeLoadListeners = () => { + win.webContents.removeListener('did-finish-load', finish) + win.webContents.removeListener('did-fail-load', fail) + } }) - await win.loadFile(tempPath) - await loadPromise + await Promise.all([win.loadFile(tempPath), loadPromise]) + signal?.throwIfAborted() await win.webContents.executeJavaScript(WAIT_FOR_IMAGES_SCRIPT, true) + signal?.throwIfAborted() return win.webContents.printToPDF({ printBackground: true, pageSize: 'A4', @@ -85,8 +116,10 @@ export async function htmlToPdf(html: string): Promise { }) })() - return await Promise.race([loadAndPrint, timeoutPromise]) + return await Promise.race([loadAndPrint, timeoutPromise, abortPromise]) } finally { + removeAbortListener?.() + removeLoadListeners?.() if (timer) { clearTimeout(timer) }