Skip to content

Commit 8b339bf

Browse files
committed
Re-factor the PDFScriptingManager._destroyScripting method (PR 13042 follow-up)
*Please note:* Given the pre-existing issues raised in PR 13056, which seem to block immediate progress there, this patch extracts some *overall* improvements of the scripting/sandbox destruction in `PDFScriptingManager`. As can be seen in `BaseViewer.setDocument`, it's currently necessary to *manually* delay the `PDFScriptingManager`-destruction in order for things to work correctly. This is, in hindsight, obviously an *extremely poor* design choice on my part; sorry about the churn here! In order to improve things overall, the `PDFScriptingManager._destroyScripting`-method is re-factored to wait for the relevant events to be dispatched *before* sandbox-destruction occurs. To avoid the scripting/sandbox-destruction indefinitely, we utilize a timeout to force-destroy the sandbox after a short time (currently set to 1 second).
1 parent 5e3af62 commit 8b339bf

File tree

2 files changed

+25
-9
lines changed

2 files changed

+25
-9
lines changed

web/base_viewer.js

+1-4
Original file line numberDiff line numberDiff line change
@@ -470,10 +470,7 @@ class BaseViewer {
470470
this.findController.setDocument(null);
471471
}
472472
if (this._scriptingManager) {
473-
// Defer this slightly, to allow the "PageClose" event to be handled.
474-
Promise.resolve().then(() => {
475-
this._scriptingManager.setDocument(null);
476-
});
473+
this._scriptingManager.setDocument(null);
477474
}
478475
}
479476

web/pdf_scripting_manager.js

+24-5
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ import { RenderingStates } from "./pdf_rendering_queue.js";
3131

3232
class PDFScriptingManager {
3333
/**
34-
* @param {PDFScriptingManager} options
34+
* @param {PDFScriptingManagerOptions} options
3535
*/
3636
constructor({
3737
eventBus,
@@ -41,6 +41,7 @@ class PDFScriptingManager {
4141
}) {
4242
this._pdfDocument = null;
4343
this._pdfViewer = null;
44+
this._closeCapability = null;
4445
this._destroyCapability = null;
4546

4647
this._scripting = null;
@@ -124,8 +125,10 @@ class PDFScriptingManager {
124125
}
125126
this._dispatchPageOpen(pageNumber);
126127
});
127-
this._internalEvents.set("pagesdestroy", event => {
128-
this._dispatchPageClose(this._pdfViewer.currentPageNumber);
128+
this._internalEvents.set("pagesdestroy", async event => {
129+
await this._dispatchPageClose(this._pdfViewer.currentPageNumber);
130+
131+
this._closeCapability?.resolve();
129132
});
130133

131134
this._domEvents.set("mousedown", event => {
@@ -307,6 +310,8 @@ class PDFScriptingManager {
307310
visitedPages = this._visitedPages;
308311

309312
if (initialize) {
313+
this._closeCapability = createPromiseCapability();
314+
310315
this._pageEventsReady = true;
311316
}
312317
if (!this._pageEventsReady) {
@@ -417,12 +422,26 @@ class PDFScriptingManager {
417422
* @private
418423
*/
419424
async _destroyScripting() {
420-
this._pdfDocument = null; // Ensure that it's *always* reset synchronously.
421-
422425
if (!this._scripting) {
426+
this._pdfDocument = null;
427+
423428
this._destroyCapability?.resolve();
424429
return;
425430
}
431+
if (this._closeCapability) {
432+
await Promise.race([
433+
this._closeCapability.promise,
434+
new Promise(resolve => {
435+
// Avoid the scripting/sandbox-destruction hanging indefinitely.
436+
setTimeout(resolve, 1000);
437+
}),
438+
]).catch(reason => {
439+
// Ignore any errors, to ensure that the sandbox is always destroyed.
440+
});
441+
this._closeCapability = null;
442+
}
443+
this._pdfDocument = null;
444+
426445
try {
427446
await this._scripting.destroySandbox();
428447
} catch (ex) {}

0 commit comments

Comments
 (0)