From 05b0798824aa9686b8a878ee9ccf691b04d57108 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 10 Apr 2021 12:46:45 +0200 Subject: [PATCH 1/2] Check that the correct `pdfDocument` is still active, before rendering the outline/attachments/layers *This patch fixes some technical debt in the viewer.* Given that most API methods are (purposely) asynchronous, there's always a risk that the viewer could have been `close`d before the requested data arrives. Lately we've started to check this case before using the data, to prevent errors and/or inconsistent state, however the outline/attachments/layers fetching and rendering is old enough that it pre-dates those checks. --- web/app.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/web/app.js b/web/app.js index 27dc2935f..7bc296bd4 100644 --- a/web/app.js +++ b/web/app.js @@ -1383,14 +1383,23 @@ const PDFViewerApplication = { onePageRendered.then(() => { pdfDocument.getOutline().then(outline => { + if (pdfDocument !== this.pdfDocument) { + return; // The document was closed while the outline resolved. + } this.pdfOutlineViewer.render({ outline, pdfDocument }); }); pdfDocument.getAttachments().then(attachments => { + if (pdfDocument !== this.pdfDocument) { + return; // The document was closed while the attachments resolved. + } this.pdfAttachmentViewer.render({ attachments }); }); // Ensure that the layers accurately reflects the current state in the // viewer itself, rather than the default state provided by the API. pdfViewer.optionalContentConfigPromise.then(optionalContentConfig => { + if (pdfDocument !== this.pdfDocument) { + return; // The document was closed while the layers resolved. + } this.pdfLayerViewer.render({ optionalContentConfig, pdfDocument }); }); if ( From 27062f72c27e37f4a8a93cca1a883e7fbe54cd71 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 10 Apr 2021 12:52:53 +0200 Subject: [PATCH 2/2] Improve the handling of the `currentOutlineButton` enabling/disabling (PR 12777 follow-up) It's obviously better and more correct to handle the "pagesloaded" case within `PDFOutlineViewer` *itself*, rather than essentially splitting the logic in two parts and forcing `PDFSidebar` to deal with what should've been handled internally in `PDFOutlineViewer`. This is what I *should* have done in PR 12777, but for some reason didn't figure out how to implement it well enough back then; sorry about the churn here! --- web/pdf_outline_viewer.js | 28 ++++++++++++++++++++++++++-- web/pdf_sidebar.js | 11 ++++++----- 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/web/pdf_outline_viewer.js b/web/pdf_outline_viewer.js index 96c229012..09afc9bf3 100644 --- a/web/pdf_outline_viewer.js +++ b/web/pdf_outline_viewer.js @@ -53,6 +53,15 @@ class PDFOutlineViewer extends BaseTreeViewer { }); this.eventBus._on("pagesloaded", evt => { this._isPagesLoaded = !!evt.pagesCount; + + // If the capability is still pending, note the `_dispatchEvent`-method, + // we know that the `currentOutlineItem`-button should be enabled here. + if ( + this._currentOutlineItemCapability && + !this._currentOutlineItemCapability.settled + ) { + this._currentOutlineItemCapability.resolve(/* enabled = */ true); + } }); this.eventBus._on("sidebarviewchanged", evt => { this._sidebarView = evt.view; @@ -66,17 +75,32 @@ class PDFOutlineViewer extends BaseTreeViewer { this._pageNumberToDestHashCapability = null; this._currentPageNumber = 1; this._isPagesLoaded = false; + + if ( + this._currentOutlineItemCapability && + !this._currentOutlineItemCapability.settled + ) { + this._currentOutlineItemCapability.resolve(/* enabled = */ false); + } + this._currentOutlineItemCapability = null; } /** * @private */ _dispatchEvent(outlineCount) { + this._currentOutlineItemCapability = createPromiseCapability(); + if ( + outlineCount === 0 || + this._pdfDocument?.loadingParams.disableAutoFetch + ) { + this._currentOutlineItemCapability.resolve(/* enabled = */ false); + } + this.eventBus.dispatch("outlineloaded", { source: this, outlineCount, - enableCurrentOutlineItemButton: - outlineCount > 0 && !this._pdfDocument?.loadingParams.disableAutoFetch, + currentOutlineItemPromise: this._currentOutlineItemCapability.promise, }); } diff --git a/web/pdf_sidebar.js b/web/pdf_sidebar.js index 4faaa4da3..1e7e04ad0 100644 --- a/web/pdf_sidebar.js +++ b/web/pdf_sidebar.js @@ -425,11 +425,12 @@ class PDFSidebar { this.eventBus._on("outlineloaded", evt => { onTreeLoaded(evt.outlineCount, this.outlineButton, SidebarView.OUTLINE); - if (evt.enableCurrentOutlineItemButton) { - this.pdfViewer.pagesPromise.then(() => { - this._currentOutlineItemButton.disabled = !this.isInitialViewSet; - }); - } + evt.currentOutlineItemPromise.then(enabled => { + if (!this.isInitialViewSet) { + return; + } + this._currentOutlineItemButton.disabled = !enabled; + }); }); this.eventBus._on("attachmentsloaded", evt => {