From 7117ee03d6128e698f6d343e35cf541eca00cfc0 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 7 Feb 2020 15:48:58 +0100 Subject: [PATCH] [api-minor] Change `PDFDocumentProxy.cleanup`/`PDFPageProxy.cleanup` to return data This patch makes the following changes, to improve these API methods: - Let `PDFPageProxy.cleanup` return a boolean indicating if clean-up actually happened, since ongoing rendering will block clean-up. Besides being used in other parts of this patch, it seems that an API user may also be interested in the return value given that clean-up isn't *guaranteed* to happen. - Let `PDFDocumentProxy.cleanup` return the promise indicating when clean-up is finished. - Improve the JSDoc comment for `PDFDocumentProxy.cleanup` to mention that clean-up is triggered on *both* threads (without going into unnecessary specifics regarding what *exactly* said data actually is). Add a note in the JSDoc comment about not calling this method when rendering is ongoing. - Change `WorkerTransport.startCleanup` to throw an `Error` if it's called when rendering is ongoing, to prevent rendering from breaking. Please note that this won't stop *worker-thread* clean-up from happening (since there's no general "something is rendering"-flag), however I'm not sure if that's really a problem; but please don't quote me on that :-) All of the caches that's being cleared in `Catalog.cleanup`, on the worker-thread, *should* be re-filled automatically even if cleared *during* parsing/rendering, and the only thing that probably happens is that e.g. font data would have to be re-parsed. On the main-thread, on the other hand, clearing the caches is more-or-less guaranteed to cause rendering errors, since the rendering code in `src/display/canvas.js` isn't able to re-request any image/font data that's suddenly being pulled out from under it. - Last, but not least, add a couple of basic unit-tests for the clean-up functionality. --- src/display/api.js | 26 ++++++++++--- test/unit/api_spec.js | 85 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 6 deletions(-) diff --git a/src/display/api.js b/src/display/api.js index d15deff9e..6d22a13de 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -758,10 +758,16 @@ class PDFDocumentProxy { } /** - * Cleans up resources allocated by the document, e.g. created `@font-face`. + * Cleans up resources allocated by the document, on both the main- and + * worker-threads. + * + * NOTE: Do not, under any circumstances, call this method when rendering is + * currently ongoing since that may lead to rendering errors. + * + * @returns {Promise} A promise that is resolved when clean-up has finished. */ cleanup() { - this._transport.startCleanup(); + return this._transport.startCleanup(); } /** @@ -1269,10 +1275,11 @@ class PDFPageProxy { * Cleans up resources allocated by the page. * @param {boolean} [resetStats] - Reset page stats, if enabled. * The default value is `false`. + * @returns {boolean} Indicating if clean-up was successfully run. */ cleanup(resetStats = false) { this.pendingCleanup = true; - this._tryCleanup(resetStats); + return this._tryCleanup(resetStats); } /** @@ -1290,7 +1297,7 @@ class PDFPageProxy { ); }) ) { - return; + return false; } Object.keys(this.intentStates).forEach(intent => { @@ -1302,6 +1309,7 @@ class PDFPageProxy { this._stats = new StatTimer(); } this.pendingCleanup = false; + return true; } /** @@ -2555,11 +2563,17 @@ class WorkerTransport { } startCleanup() { - this.messageHandler.sendWithPromise("Cleanup", null).then(() => { + 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) { - page.cleanup(); + const cleanupSuccessful = page.cleanup(); + + if (!cleanupSuccessful) { + throw new Error( + `startCleanup: Page ${i + 1} is currently rendering.` + ); + } } } this.commonObjs.clear(); diff --git a/test/unit/api_spec.js b/test/unit/api_spec.js index 408d15270..dfc20489c 100644 --- a/test/unit/api_spec.js +++ b/test/unit/api_spec.js @@ -1151,6 +1151,14 @@ describe("api", function() { .catch(done.fail); }); + it("cleans up document resources", function(done) { + const promise = doc.cleanup(); + promise.then(function() { + expect(true).toEqual(true); + done(); + }, done.fail); + }); + it("checks that fingerprints are unique", function(done) { const loadingTask1 = getDocument( buildGetDocumentParams("issue4436r.pdf") @@ -1764,6 +1772,83 @@ describe("api", function() { ), ]).then(done); }); + + it("cleans up document resources after rendering of page", function(done) { + const loadingTask = getDocument(buildGetDocumentParams(basicApiFileName)); + let canvasAndCtx; + + loadingTask.promise + .then(pdfDoc => { + return pdfDoc.getPage(1).then(pdfPage => { + const viewport = pdfPage.getViewport({ scale: 1 }); + 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); + + CanvasFactory.destroy(canvasAndCtx); + loadingTask.destroy().then(done); + }, done.fail); + }); + + it("cleans up document resources during rendering of page", function(done) { + const loadingTask = getDocument( + buildGetDocumentParams("tracemonkey.pdf") + ); + let canvasAndCtx; + + loadingTask.promise + .then(pdfDoc => { + return pdfDoc.getPage(1).then(pdfPage => { + const viewport = pdfPage.getViewport({ scale: 1 }); + canvasAndCtx = CanvasFactory.create( + viewport.width, + viewport.height + ); + + const renderTask = pdfPage.render({ + canvasContext: canvasAndCtx.context, + canvasFactory: CanvasFactory, + viewport, + }); + + 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); + }); }); describe("Multiple `getDocument` instances", function() { // Regression test for https://github.com/mozilla/pdf.js/issues/6205