From 5ec52e2e5bc97b5d1f2086055193418260d6e892 Mon Sep 17 00:00:00 2001 From: Alf Eaton Date: Thu, 24 Oct 2024 09:39:42 +0100 Subject: [PATCH] Avoid calling destroy on the PDF.js loading task (#21277) GitOrigin-RevId: b315e78ff739d301583f2139109f3244abceade8 --- .../file-view/components/file-view-pdf.tsx | 14 ++- .../pdf-preview/components/pdf-js-viewer.tsx | 20 ++-- .../pdf-preview/util/pdf-js-wrapper.ts | 92 +++++++------------ 3 files changed, 48 insertions(+), 78 deletions(-) diff --git a/services/web/frontend/js/features/file-view/components/file-view-pdf.tsx b/services/web/frontend/js/features/file-view/components/file-view-pdf.tsx index 5222f2db7b..d23c412c7a 100644 --- a/services/web/frontend/js/features/file-view/components/file-view-pdf.tsx +++ b/services/web/frontend/js/features/file-view/components/file-view-pdf.tsx @@ -1,7 +1,6 @@ import { FC, useCallback } from 'react' import useIsMounted from '@/shared/hooks/use-is-mounted' import { useFileTreePathContext } from '@/features/file-tree/contexts/file-tree-path' -import { debugConsole } from '@/utils/debugging' const FileViewPdf: FC<{ fileId: string @@ -21,7 +20,6 @@ const FileViewPdf: FC<{ // bail out if loading PDF.js took too long if (!mountedRef.current) { - onError() return } @@ -37,7 +35,6 @@ const FileViewPdf: FC<{ // bail out if loading the PDF took too long if (!mountedRef.current) { - onError() return } @@ -47,6 +44,12 @@ const FileViewPdf: FC<{ for (let i = 1; i <= pdf.numPages; i++) { const page = await pdf.getPage(i) + + // bail out if the component has been unmounted + if (!mountedRef.current) { + return + } + const viewport = page.getViewport({ scale }) const canvas = document.createElement('canvas') @@ -64,11 +67,6 @@ const FileViewPdf: FC<{ } onLoad() - - return () => { - pdf.cleanup().catch(debugConsole.error) - pdf.destroy() - } } }, [mountedRef, pathInFolder, fileId, previewByPath, onLoad, onError] diff --git a/services/web/frontend/js/features/pdf-preview/components/pdf-js-viewer.tsx b/services/web/frontend/js/features/pdf-preview/components/pdf-js-viewer.tsx index e33d855517..6600e096d5 100644 --- a/services/web/frontend/js/features/pdf-preview/components/pdf-js-viewer.tsx +++ b/services/web/frontend/js/features/pdf-preview/components/pdf-js-viewer.tsx @@ -64,24 +64,23 @@ function PdfJsViewer({ url, pdfFile }: PdfJsViewerProps) { const handleContainer = useCallback( parent => { if (parent) { - let wrapper: PDFJSWrapper | undefined try { - wrapper = new PDFJSWrapper(parent.firstChild) - setPdfJsWrapper(wrapper) + setPdfJsWrapper(new PDFJSWrapper(parent.firstChild)) } catch (error: any) { setLoadingError(true) captureException(error) } - - return () => { - setPdfJsWrapper(null) - wrapper?.destroy().catch(debugConsole.error) - } } }, [setLoadingError] ) + useEffect(() => { + return () => { + setPdfJsWrapper(null) + } + }, []) + const [startFetch, setStartFetch] = useState(0) // listen for events and trigger rendering. @@ -181,7 +180,9 @@ function PdfJsViewer({ url, pdfFile }: PdfJsViewerProps) { pdfJsWrapper .loadDocument({ url, pdfFile, abortController, handleFetchError }) .then(doc => { - setTotalPages(doc.numPages) + if (doc) { + setTotalPages(doc.numPages) + } }) .catch(error => { if (abortController.signal.aborted) return @@ -190,7 +191,6 @@ function PdfJsViewer({ url, pdfFile }: PdfJsViewerProps) { }) return () => { abortController.abort() - pdfJsWrapper.abortDocumentLoading() } } }, [pdfJsWrapper, url, pdfFile, setError, setStartFetch]) diff --git a/services/web/frontend/js/features/pdf-preview/util/pdf-js-wrapper.ts b/services/web/frontend/js/features/pdf-preview/util/pdf-js-wrapper.ts index 40df51f073..ca1ff2ea0e 100644 --- a/services/web/frontend/js/features/pdf-preview/util/pdf-js-wrapper.ts +++ b/services/web/frontend/js/features/pdf-preview/util/pdf-js-wrapper.ts @@ -1,6 +1,5 @@ import { captureException } from '@/infrastructure/error-reporter' import { generatePdfCachingTransportFactory } from './pdf-caching-transport' -import { debugConsole } from '@/utils/debugging' import { PDFJS, loadPdfDocumentFromUrl, imageResourcesPath } from './pdf-js' import { PDFViewer, @@ -13,12 +12,11 @@ import 'pdfjs-dist/web/pdf_viewer.css' const DEFAULT_RANGE_CHUNK_SIZE = 128 * 1024 // 128K chunks export default class PDFJSWrapper { - private loadDocumentTask: PDFJS.PDFDocumentLoadingTask | undefined - public readonly viewer: PDFViewer public readonly eventBus: EventBus private readonly linkService: PDFLinkService private readonly pdfCachingTransportFactory: any + private url?: string // eslint-disable-next-line no-useless-constructor constructor(public container: HTMLDivElement) { @@ -49,7 +47,7 @@ export default class PDFJSWrapper { } // load a document from a URL - loadDocument({ + async loadDocument({ url, pdfFile, abortController, @@ -60,61 +58,45 @@ export default class PDFJSWrapper { abortController: AbortController handleFetchError: (error: Error) => void }) { - // cancel any previous loading task - if (this.loadDocumentTask) { - this.loadDocumentTask.destroy().catch(debugConsole.error) - this.loadDocumentTask = undefined + this.url = url + + const rangeTransport = this.pdfCachingTransportFactory({ + url, + pdfFile, + abortController, + handleFetchError, + }) + let rangeChunkSize = DEFAULT_RANGE_CHUNK_SIZE + if (rangeTransport && pdfFile.size < 2 * DEFAULT_RANGE_CHUNK_SIZE) { + // pdf.js disables the "bulk" download optimization when providing a + // custom range transport. Restore it by bumping the chunk size. + rangeChunkSize = pdfFile.size } - return new Promise((resolve, reject) => { - const rangeTransport = this.pdfCachingTransportFactory({ - url, - pdfFile, - abortController, - handleFetchError, - }) - let rangeChunkSize = DEFAULT_RANGE_CHUNK_SIZE - if (rangeTransport && pdfFile.size < 2 * DEFAULT_RANGE_CHUNK_SIZE) { - // pdf.js disables the "bulk" download optimization when providing a - // custom range transport. Restore it by bumping the chunk size. - rangeChunkSize = pdfFile.size - } - this.loadDocumentTask = loadPdfDocumentFromUrl(url, { + try { + const doc = await loadPdfDocumentFromUrl(url, { rangeChunkSize, range: rangeTransport, - }) + }).promise - this.loadDocumentTask.promise - .then(doc => { - if (!this.loadDocumentTask || !this.viewer) { - return // ignoring the response since loading task has been aborted - } + // check that this is still the current URL + if (url !== this.url) { + return + } - const previousDoc = this.viewer.pdfDocument - this.viewer.setDocument(doc) - this.linkService.setDocument(doc) - resolve(doc) + this.viewer.setDocument(doc) + this.linkService.setDocument(doc) - if (previousDoc) { - previousDoc.cleanup().catch(debugConsole.error) - previousDoc.destroy().catch(debugConsole.error) - } + return doc + } catch (error: any) { + if (!error || error.name !== 'MissingPDFException') { + captureException(error, { + tags: { handler: 'pdf-preview' }, }) - .catch(error => { - if (this.loadDocumentTask) { - if (!error || error.name !== 'MissingPDFException') { - captureException(error, { - tags: { handler: 'pdf-preview' }, - }) - } + } - reject(error) - } - }) - .finally(() => { - this.loadDocumentTask = undefined - }) - }) + throw error + } } // update the current scale value if the container size changes @@ -216,14 +198,4 @@ export default class PDFJSWrapper { isVisible() { return this.viewer.container.offsetParent !== null } - - abortDocumentLoading() { - this.loadDocumentTask = undefined - } - - async destroy() { - await this.loadDocumentTask?.destroy().catch(debugConsole.error) - this.loadDocumentTask = undefined - await this.viewer.pdfDocument?.destroy().catch(debugConsole.error) - } }