From c6fcdf474b7b4e0f7d130e092061782a6b90e018 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 10 Jul 2019 13:24:21 +0200 Subject: [PATCH 1/2] Remove the `intentState.receivingOperatorList` boolean since it's redundant The `receivingOperatorList` property is currently tracked *twice* in the rendering code, both directly and inversely through the `intentState.operatorList.lastChunk` boolean. This type of double bookkeeping is never a good idea, since it's just too easy for the properties to accidentally fall out of sync. In this case there's even a `cleanup`-related bug caused by this, which means that `PDFPageProxy._tryCleanup` will never be able to discard any data if there's an error on the worker-thread (as handled through the 'PageError' message). Hence the simplest solution seems, at least to me, to update `PDFPageProxy._tryCleanup` to replace the `intentState.receivingOperatorList` check with a `!intentState.operatorList.lastChunk` check and completely remove the former property. --- src/display/api.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/display/api.js b/src/display/api.js index 0fc1b155a..40efb5db0 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -1023,7 +1023,6 @@ class PDFPageProxy { // If there's no displayReadyCapability yet, then the operatorList // was never requested before. Make the request and create the promise. if (!intentState.displayReadyCapability) { - intentState.receivingOperatorList = true; intentState.displayReadyCapability = createPromiseCapability(); intentState.operatorList = { fnArray: [], @@ -1125,7 +1124,6 @@ class PDFPageProxy { if (!intentState.opListReadCapability) { opListTask = {}; opListTask.operatorListChanged = operatorListChanged; - intentState.receivingOperatorList = true; intentState.opListReadCapability = createPromiseCapability(); intentState.renderTasks = []; intentState.renderTasks.push(opListTask); @@ -1241,7 +1239,7 @@ class PDFPageProxy { Object.keys(this.intentStates).some(function(intent) { const intentState = this.intentStates[intent]; return (intentState.renderTasks.length !== 0 || - intentState.receivingOperatorList); + !intentState.operatorList.lastChunk); }, this)) { return; } @@ -1290,7 +1288,6 @@ class PDFPageProxy { } if (operatorListChunk.lastChunk) { - intentState.receivingOperatorList = false; this._tryCleanup(); } } From ef48a9a7134352afb52be23c42961765a966ba32 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 10 Jul 2019 13:41:16 +0200 Subject: [PATCH 2/2] Update the `PageError` handler, in the API, to always mark the `operatorList` as done and finalize any pending renderTasks Note that, in the old code, there was a code-path which could prevent this from happening thus affecting future cleanup. Furthermore, ensure that we'll always attempt to cleanup when handling the 'PageError' message, similar to the code in e.g. the `PDFPageProxy._renderPageChunk` method. --- src/display/api.js | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/display/api.js b/src/display/api.js index 40efb5db0..97c97f95f 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -2089,19 +2089,21 @@ class WorkerTransport { const page = this.pageCache[data.pageIndex]; const intentState = page.intentStates[data.intent]; + if (intentState.operatorList) { + // Mark operator list as complete. + intentState.operatorList.lastChunk = true; + + for (let i = 0; i < intentState.renderTasks.length; i++) { + intentState.renderTasks[i].operatorListChanged(); + } + page._tryCleanup(); + } + if (intentState.displayReadyCapability) { intentState.displayReadyCapability.reject(new Error(data.error)); } else { throw new Error(data.error); } - - if (intentState.operatorList) { - // Mark operator list as complete. - intentState.operatorList.lastChunk = true; - for (let i = 0; i < intentState.renderTasks.length; i++) { - intentState.renderTasks[i].operatorListChanged(); - } - } }, this); messageHandler.on('UnsupportedFeature', this._onUnsupportedFeature, this);