From 48ff20493fb2360585934c45efd0f7f200d8dbdd Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 2 Apr 2021 12:26:32 +0200 Subject: [PATCH 1/3] Mark some internal `PDFDocumentProxy`-properties as "private" These two properties were *never* intended to be anything but "private", hence it really cannot hurt to actually indicate that they're *not* part of any official API. --- src/display/api.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/display/api.js b/src/display/api.js index 6549d85ab..fa501b074 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -1180,14 +1180,14 @@ class PDFPageProxy { * {Array} of the annotation objects. */ getAnnotations({ intent = null } = {}) { - if (!this.annotationsPromise || this.annotationsIntent !== intent) { - this.annotationsPromise = this._transport.getAnnotations( + if (!this._annotationsPromise || this._annotationsIntent !== intent) { + this._annotationsPromise = this._transport.getAnnotations( this._pageIndex, intent ); - this.annotationsIntent = intent; + this._annotationsIntent = intent; } - return this.annotationsPromise; + return this._annotationsPromise; } /** @@ -1490,7 +1490,7 @@ class PDFPageProxy { } } this.objs.clear(); - this.annotationsPromise = null; + this._annotationsPromise = null; this._jsActionsPromise = null; this._xfaPromise = null; this.pendingCleanup = false; @@ -1525,7 +1525,7 @@ class PDFPageProxy { this._intentStates.clear(); this.objs.clear(); - this.annotationsPromise = null; + this._annotationsPromise = null; this._jsActionsPromise = null; this._xfaPromise = null; if (resetStats && this._stats) { From a2bc6481a081a1093fad8b638c30d5bb8d099584 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 2 Apr 2021 12:26:39 +0200 Subject: [PATCH 2/3] [api-minor] Add an option, in `PDFDocumentProxy.cleanup`, to allow fonts to remain attached to the DOM As mentioned in the JSDoc comment, this should not be used unless you know what you're doing, since it will lead to increased memory usage. However, in some situations (e.g. SVG-rendering), we still want to be able to run general clean-up on both the main/worker-thread while keeping loaded fonts attached to the DOM.[1] As part of these changes, `WorkerTransport.startCleanup` is converted to an async method and we'll also skip clean-up when destruction has started (since it's redundant). --- [1] The SVG-rendering mode is obviously not officially supported, since it's both rather incomplete and inherently slower. However with recent changes, whereby we cache repeated images on the document rather than the page level, memory usage can be *a lot* worse than before if we never attempt to release e.g. cached image-data when the viewer is in SVG-rendering mode. --- src/display/api.js | 41 ++++++++++++++++++++++++----------------- web/app.js | 13 ++++++++----- 2 files changed, 32 insertions(+), 22 deletions(-) diff --git a/src/display/api.js b/src/display/api.js index fa501b074..617e10a64 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -921,10 +921,13 @@ class PDFDocumentProxy { * NOTE: Do not, under any circumstances, call this method when rendering is * currently ongoing since that may lead to rendering errors. * + * @param {boolean} [keepLoadedFonts] - Let fonts remain attached to the DOM. + * NOTE: This will increase persistent memory usage, hence don't use this + * option unless absolutely necessary. The default value is `false`. * @returns {Promise} A promise that is resolved when clean-up has finished. */ - cleanup() { - return this._transport.startCleanup(); + cleanup(keepLoadedFonts = false) { + return this._transport.startCleanup(keepLoadedFonts || this.isPureXfa); } /** @@ -2799,24 +2802,28 @@ class WorkerTransport { return this.messageHandler.sendWithPromise("GetStats", null); } - startCleanup() { - return this.messageHandler.sendWithPromise("Cleanup", null).then(() => { - for (let i = 0, ii = this.pageCache.length; i < ii; i++) { - const page = this.pageCache[i]; - if (page) { - const cleanupSuccessful = page.cleanup(); + async startCleanup(keepLoadedFonts = false) { + await this.messageHandler.sendWithPromise("Cleanup", null); - if (!cleanupSuccessful) { - throw new Error( - `startCleanup: Page ${i + 1} is currently rendering.` - ); - } - } + if (this.destroyed) { + return; // No need to manually clean-up when destruction has started. + } + for (let i = 0, ii = this.pageCache.length; i < ii; i++) { + const page = this.pageCache[i]; + if (!page) { + continue; } - this.commonObjs.clear(); + const cleanupSuccessful = page.cleanup(); + + if (!cleanupSuccessful) { + throw new Error(`startCleanup: Page ${i + 1} is currently rendering.`); + } + } + this.commonObjs.clear(); + if (!keepLoadedFonts) { this.fontLoader.clear(); - this._hasJSActionsPromise = null; - }); + } + this._hasJSActionsPromise = null; } get loadingParams() { diff --git a/web/app.js b/web/app.js index 3a66de48a..e9d5ca9b1 100644 --- a/web/app.js +++ b/web/app.js @@ -465,7 +465,7 @@ const PDFViewerApplication = { this.overlayManager = new OverlayManager(); const pdfRenderingQueue = new PDFRenderingQueue(); - pdfRenderingQueue.onIdle = this.cleanup.bind(this); + pdfRenderingQueue.onIdle = this._cleanup.bind(this); this.pdfRenderingQueue = pdfRenderingQueue; const pdfLinkService = new PDFLinkService({ @@ -1767,7 +1767,10 @@ const PDFViewerApplication = { } }, - cleanup() { + /** + * @private + */ + _cleanup() { if (!this.pdfDocument) { return; // run cleanup when document is loaded } @@ -1775,9 +1778,9 @@ const PDFViewerApplication = { this.pdfThumbnailViewer.cleanup(); // We don't want to remove fonts used by active page SVGs. - if (this.pdfViewer.renderer !== RendererType.SVG) { - this.pdfDocument.cleanup(); - } + this.pdfDocument.cleanup( + /* keepLoadedFonts = */ this.pdfViewer.renderer === RendererType.SVG + ); }, forceRendering() { From 232fbd28e1c23039bea3e2651ef4c76ae8420be4 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 2 Apr 2021 12:26:46 +0200 Subject: [PATCH 3/3] Re-factor the `PDFDocumentProxy.cleanup` unit-tests to use async/await --- test/unit/api_spec.js | 126 ++++++++++++++++++------------------------ 1 file changed, 54 insertions(+), 72 deletions(-) diff --git a/test/unit/api_spec.js b/test/unit/api_spec.js index 22ea78f13..d44050f6b 100644 --- a/test/unit/api_spec.js +++ b/test/unit/api_spec.js @@ -1339,12 +1339,10 @@ describe("api", function () { .catch(done.fail); }); - it("cleans up document resources", function (done) { - const promise = pdfDocument.cleanup(); - promise.then(function () { - expect(true).toEqual(true); - done(); - }, done.fail); + it("cleans up document resources", async function () { + await pdfDocument.cleanup(); + + expect(true).toEqual(true); }); it("checks that fingerprints are unique", function (done) { @@ -1982,85 +1980,69 @@ describe("api", function () { ]).then(done); }); - it("cleans up document resources after rendering of page", function (done) { + it("cleans up document resources after rendering of page", async function () { const loadingTask = getDocument(buildGetDocumentParams(basicApiFileName)); - let canvasAndCtx; + const pdfDoc = await loadingTask.promise; + const pdfPage = await pdfDoc.getPage(1); - loadingTask.promise - .then(pdfDoc => { - return pdfDoc.getPage(1).then(pdfPage => { - const viewport = pdfPage.getViewport({ scale: 1 }); - canvasAndCtx = CanvasFactory.create( - viewport.width, - viewport.height - ); + const viewport = pdfPage.getViewport({ scale: 1 }); + const canvasAndCtx = CanvasFactory.create( + viewport.width, + viewport.height + ); - const renderTask = pdfPage.render({ - canvasContext: canvasAndCtx.context, - canvasFactory: CanvasFactory, - viewport, - }); - return renderTask.promise.then(() => { - return pdfDoc.cleanup(); - }); - }); - }) - .then(() => { - expect(true).toEqual(true); + const renderTask = pdfPage.render({ + canvasContext: canvasAndCtx.context, + canvasFactory: CanvasFactory, + viewport, + }); + await renderTask.promise; - CanvasFactory.destroy(canvasAndCtx); - loadingTask.destroy().then(done); - }, done.fail); + await pdfDoc.cleanup(); + + expect(true).toEqual(true); + + CanvasFactory.destroy(canvasAndCtx); + await loadingTask.destroy(); }); - it("cleans up document resources during rendering of page", function (done) { + it("cleans up document resources during rendering of page", async function () { const loadingTask = getDocument( buildGetDocumentParams("tracemonkey.pdf") ); - let canvasAndCtx; + const pdfDoc = await loadingTask.promise; + const pdfPage = await pdfDoc.getPage(1); - loadingTask.promise - .then(pdfDoc => { - return pdfDoc.getPage(1).then(pdfPage => { - const viewport = pdfPage.getViewport({ scale: 1 }); - canvasAndCtx = CanvasFactory.create( - viewport.width, - viewport.height - ); + const viewport = pdfPage.getViewport({ scale: 1 }); + const canvasAndCtx = CanvasFactory.create( + viewport.width, + viewport.height + ); - const renderTask = pdfPage.render({ - canvasContext: canvasAndCtx.context, - canvasFactory: CanvasFactory, - viewport, - }); + const renderTask = pdfPage.render({ + canvasContext: canvasAndCtx.context, + canvasFactory: CanvasFactory, + viewport, + }); + // Ensure that clean-up runs during rendering. + renderTask.onContinue = function (cont) { + waitSome(cont); + }; - renderTask.onContinue = function (cont) { - waitSome(cont); - }; + try { + await pdfDoc.cleanup(); - return pdfDoc - .cleanup() - .then( - () => { - throw new Error("shall fail cleanup"); - }, - reason => { - expect(reason instanceof Error).toEqual(true); - expect(reason.message).toEqual( - "startCleanup: Page 1 is currently rendering." - ); - } - ) - .then(() => { - return renderTask.promise; - }) - .then(() => { - CanvasFactory.destroy(canvasAndCtx); - loadingTask.destroy().then(done); - }); - }); - }) - .catch(done.fail); + throw new Error("shall fail cleanup"); + } catch (reason) { + expect(reason instanceof Error).toEqual(true); + expect(reason.message).toEqual( + "startCleanup: Page 1 is currently rendering." + ); + } + await renderTask.promise; + + CanvasFactory.destroy(canvasAndCtx); + await loadingTask.destroy(); }); it("caches image resources at the document/page level as expected (issue 11878)", async function () {