[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.
This commit is contained in:
Jonas Jenwald 2020-02-07 15:48:58 +01:00
parent a5fec297c0
commit 7117ee03d6
2 changed files with 105 additions and 6 deletions

View File

@ -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();

View File

@ -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