From 7b15094cdfdc56b1e8d932b1a8c9811e5fab2823 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 23 Sep 2020 14:11:45 +0200 Subject: [PATCH 1/6] Ignore `RenderingCancelledException` when logging errors in `PDFRenderingQueue.renderView` Note that a `RenderingCancelledException` *should* never actually reach this method, but better safe than sorry I suppose, considering that both `PDFPageView` and `PDFThumbnailView` are already catching `RenderingCancelledException`s since those are *not* Errors in the normal sense of the word. --- web/pdf_rendering_queue.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/web/pdf_rendering_queue.js b/web/pdf_rendering_queue.js index 444f1cb01..64b377dd2 100644 --- a/web/pdf_rendering_queue.js +++ b/web/pdf_rendering_queue.js @@ -13,6 +13,8 @@ * limitations under the License. */ +import { RenderingCancelledException } from "pdfjs-lib"; + const CLEANUP_TIMEOUT = 30000; const RenderingStates = { @@ -170,6 +172,9 @@ class PDFRenderingQueue { this.renderHighestPriority(); }) .catch(reason => { + if (reason instanceof RenderingCancelledException) { + return; + } console.error(`renderView: "${reason}"`); }); break; From 9efc1784b20f77a93227a15d065d069b2e420482 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 23 Sep 2020 14:29:21 +0200 Subject: [PATCH 2/6] Remove the `PDFPageView.error` property, and simply include Errors in the "pagerendered" event instead The way that rendering errors are handled in `PDFPageView` is *very* old, and predates e.g. the introduction of the `EventBus` by several years. Hence we should be able to simplify things a bit here, by including the Error (when it exists) in the "pagerendered" event and thus avoid having to reach into `PDFPageView` for it. --- web/app.js | 9 ++++----- web/pdf_page_view.js | 11 +++++++---- web/pdf_thumbnail_view.js | 2 +- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/web/app.js b/web/app.js index 6fd9d6d9f..885edab51 100644 --- a/web/app.js +++ b/web/app.js @@ -2126,8 +2126,7 @@ function webViewerResetPermissions() { appConfig.viewerContainer.classList.remove(ENABLE_PERMISSIONS_CLASS); } -function webViewerPageRendered(evt) { - const pageNumber = evt.pageNumber; +function webViewerPageRendered({ pageNumber, timestamp, error }) { const pageIndex = pageNumber - 1; const pageView = PDFViewerApplication.pdfViewer.getPageView(pageIndex); @@ -2155,7 +2154,7 @@ function webViewerPageRendered(evt) { Stats.add(pageNumber, pageView.stats); } - if (pageView.error) { + if (error) { PDFViewerApplication.l10n .get( "rendering_error", @@ -2163,13 +2162,13 @@ function webViewerPageRendered(evt) { "An error occurred while rendering the page." ) .then(msg => { - PDFViewerApplication.error(msg, pageView.error); + PDFViewerApplication.error(msg, error); }); } PDFViewerApplication.externalServices.reportTelemetry({ type: "pageInfo", - timestamp: evt.timestamp, + timestamp, }); // It is a good time to report stream and font types. PDFViewerApplication.pdfDocument.getStats().then(function (stats) { diff --git a/web/pdf_page_view.js b/web/pdf_page_view.js index 31e54ff0a..90f780df3 100644 --- a/web/pdf_page_view.js +++ b/web/pdf_page_view.js @@ -114,7 +114,7 @@ class PDFPageView { this.paintedViewportMap = new WeakMap(); this.renderingState = RenderingStates.INITIAL; this.resume = null; - this.error = null; + this._renderError = null; this.annotationLayer = null; this.textLayer = null; @@ -265,6 +265,7 @@ class PDFPageView { pageNumber: this.id, cssTransform: true, timestamp: performance.now(), + error: this._renderError, }); return; } @@ -293,6 +294,7 @@ class PDFPageView { pageNumber: this.id, cssTransform: true, timestamp: performance.now(), + error: this._renderError, }); return; } @@ -500,7 +502,7 @@ class PDFPageView { }; } - const finishPaintTask = async error => { + const finishPaintTask = async (error = null) => { // The paintTask may have been replaced by a new one, so only remove // the reference to the paintTask if it matches the one that is // triggering this callback. @@ -509,9 +511,10 @@ class PDFPageView { } if (error instanceof RenderingCancelledException) { - this.error = null; + this._renderError = null; return; } + this._renderError = error; this.renderingState = RenderingStates.FINISHED; @@ -521,7 +524,6 @@ class PDFPageView { } this._resetZoomLayer(/* removeFromDOM = */ true); - this.error = error; this.stats = pdfPage.stats; this.eventBus.dispatch("pagerendered", { @@ -529,6 +531,7 @@ class PDFPageView { pageNumber: this.id, cssTransform: false, timestamp: performance.now(), + error: this._renderError, }); if (error) { diff --git a/web/pdf_thumbnail_view.js b/web/pdf_thumbnail_view.js index 685bf53b6..dddd931b5 100644 --- a/web/pdf_thumbnail_view.js +++ b/web/pdf_thumbnail_view.js @@ -317,7 +317,7 @@ class PDFThumbnailView { this.renderingState = RenderingStates.RUNNING; const renderCapability = createPromiseCapability(); - const finishRenderTask = error => { + const finishRenderTask = (error = null) => { // The renderTask may have been replaced by a new one, so only remove // the reference to the renderTask if it matches the one that is // triggering this callback. From e46055a92c980b3cb221760ddbd8bfbd2144d1e8 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 23 Sep 2020 14:39:14 +0200 Subject: [PATCH 3/6] Remove the `PDFPageView.stats` property, and fetch it manually only when debugging is enabled Given that the default viewer only uses the "page stats" when debugging is enabled, it seems much simpler and more straightforward to simply query the API *directly* when this information is actually required. That way, there's a bit less information that needs to be stored/updated on each `PDFPageView`-instance. Finally, since the `EventBus` now exists, we no longer need to handle the "page stats"-case in the regular listeners in `web/app.js`, but can instead add special "page stats"-listeners only when debugging is enabled. --- web/app.js | 40 ++++++++++++++++++++++++++++------------ web/pdf_page_view.js | 3 --- 2 files changed, 28 insertions(+), 15 deletions(-) diff --git a/web/app.js b/web/app.js index 885edab51..169af9ff1 100644 --- a/web/app.js +++ b/web/app.js @@ -1775,6 +1775,13 @@ const PDFViewerApplication = { eventBus._on("findfromurlhash", webViewerFindFromUrlHash); eventBus._on("updatefindmatchescount", webViewerUpdateFindMatchesCount); eventBus._on("updatefindcontrolstate", webViewerUpdateFindControlState); + + if (AppOptions.get("pdfBug")) { + _boundEvents.reportPageStatsPDFBug = reportPageStatsPDFBug; + + eventBus._on("pagerendered", _boundEvents.reportPageStatsPDFBug); + eventBus._on("pagechanging", _boundEvents.reportPageStatsPDFBug); + } if (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) { eventBus._on("fileinputchange", webViewerFileInputChange); eventBus._on("openfile", webViewerOpenFile); @@ -1855,6 +1862,13 @@ const PDFViewerApplication = { eventBus._off("findfromurlhash", webViewerFindFromUrlHash); eventBus._off("updatefindmatchescount", webViewerUpdateFindMatchesCount); eventBus._off("updatefindcontrolstate", webViewerUpdateFindControlState); + + if (_boundEvents.reportPageStatsPDFBug) { + eventBus._off("pagerendered", _boundEvents.reportPageStatsPDFBug); + eventBus._off("pagechanging", _boundEvents.reportPageStatsPDFBug); + + _boundEvents.reportPageStatsPDFBug = null; + } if (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) { eventBus._off("fileinputchange", webViewerFileInputChange); eventBus._off("openfile", webViewerOpenFile); @@ -1966,6 +1980,20 @@ function loadAndEnablePDFBug(enabledTabs) { }); } +function reportPageStatsPDFBug({ pageNumber }) { + if (typeof Stats === "undefined" || !Stats.enabled) { + return; + } + const pageView = PDFViewerApplication.pdfViewer.getPageView( + /* index = */ pageNumber - 1 + ); + const pageStats = pageView && pageView.pdfPage && pageView.pdfPage.stats; + if (!pageStats) { + return; + } + Stats.add(pageNumber, pageStats); +} + function webViewerInitialized() { const appConfig = PDFViewerApplication.appConfig; let file; @@ -2150,10 +2178,6 @@ function webViewerPageRendered({ pageNumber, timestamp, error }) { thumbnailView.setImage(pageView); } - if (typeof Stats !== "undefined" && Stats.enabled && pageView.stats) { - Stats.add(pageNumber, pageView.stats); - } - if (error) { PDFViewerApplication.l10n .get( @@ -2534,14 +2558,6 @@ function webViewerPageChanging(evt) { if (PDFViewerApplication.pdfSidebar.isThumbnailViewVisible) { PDFViewerApplication.pdfThumbnailViewer.scrollThumbnailIntoView(page); } - - // We need to update stats. - if (typeof Stats !== "undefined" && Stats.enabled) { - const pageView = PDFViewerApplication.pdfViewer.getPageView(page - 1); - if (pageView && pageView.stats) { - Stats.add(page, pageView.stats); - } - } } function webViewerVisibilityChange(evt) { diff --git a/web/pdf_page_view.js b/web/pdf_page_view.js index 90f780df3..658b06b19 100644 --- a/web/pdf_page_view.js +++ b/web/pdf_page_view.js @@ -139,7 +139,6 @@ class PDFPageView { scale: this.scale * CSS_UNITS, rotation: totalRotation, }); - this.stats = pdfPage.stats; this.reset(); } @@ -524,8 +523,6 @@ class PDFPageView { } this._resetZoomLayer(/* removeFromDOM = */ true); - this.stats = pdfPage.stats; - this.eventBus.dispatch("pagerendered", { source: this, pageNumber: this.id, From a4e5458774e9e1ddaef63f3cb17845086b997a04 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 23 Sep 2020 15:38:54 +0200 Subject: [PATCH 4/6] Update all `BaseViewer.getPageView`/`PDFThumbnailViewer.getThumbnail` call-sites, in `web/app.js`, to check the returned value properly Given how those are used, there *should* not be any situation in which e.g. `undefined` is ever returned. However, actually checking that the pageView/thumbnail is defined cannot hurt. Also, re-factor `webViewerPageRendered` slightly since the `pageView` is no longer unconditionally necessary after the previous patches; note in particular that the thumbnails will only be updated when the sidebar *and* the thumbnailView is visible. Finally, fixes a bug in `webViewerPageChanging` whereby an empty string would not be treated as a valid pageLabel and instead be replaced by `null`. --- web/app.js | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/web/app.js b/web/app.js index 169af9ff1..6b8f41f1b 100644 --- a/web/app.js +++ b/web/app.js @@ -2155,27 +2155,23 @@ function webViewerResetPermissions() { } function webViewerPageRendered({ pageNumber, timestamp, error }) { - const pageIndex = pageNumber - 1; - const pageView = PDFViewerApplication.pdfViewer.getPageView(pageIndex); - // If the page is still visible when it has finished rendering, // ensure that the page number input loading indicator is hidden. if (pageNumber === PDFViewerApplication.page) { PDFViewerApplication.toolbar.updateLoadingIndicatorState(false); } - // Prevent errors in the edge-case where the PDF document is removed *before* - // the 'pagerendered' event handler is invoked. - if (!pageView) { - return; - } - // Use the rendered page to set the corresponding thumbnail image. if (PDFViewerApplication.pdfSidebar.isThumbnailViewVisible) { - const thumbnailView = PDFViewerApplication.pdfThumbnailViewer.getThumbnail( - pageIndex + const pageView = PDFViewerApplication.pdfViewer.getPageView( + /* index = */ pageNumber - 1 ); - thumbnailView.setImage(pageView); + const thumbnailView = PDFViewerApplication.pdfThumbnailViewer.getThumbnail( + /* index = */ pageNumber - 1 + ); + if (pageView && thumbnailView) { + thumbnailView.setImage(pageView); + } } if (error) { @@ -2302,9 +2298,10 @@ function webViewerUpdateViewarea(evt) { // Show/hide the loading indicator in the page number input element. const currentPage = PDFViewerApplication.pdfViewer.getPageView( - PDFViewerApplication.page - 1 + /* index = */ PDFViewerApplication.page - 1 ); - const loading = currentPage.renderingState !== RenderingStates.FINISHED; + const loading = + (currentPage && currentPage.renderingState) !== RenderingStates.FINISHED; PDFViewerApplication.toolbar.updateLoadingIndicatorState(loading); } @@ -2549,14 +2546,12 @@ function webViewerRotationChanging(evt) { PDFViewerApplication.pdfViewer.currentPageNumber = evt.pageNumber; } -function webViewerPageChanging(evt) { - const page = evt.pageNumber; - - PDFViewerApplication.toolbar.setPageNumber(page, evt.pageLabel || null); - PDFViewerApplication.secondaryToolbar.setPageNumber(page); +function webViewerPageChanging({ pageNumber, pageLabel }) { + PDFViewerApplication.toolbar.setPageNumber(pageNumber, pageLabel); + PDFViewerApplication.secondaryToolbar.setPageNumber(pageNumber); if (PDFViewerApplication.pdfSidebar.isThumbnailViewVisible) { - PDFViewerApplication.pdfThumbnailViewer.scrollThumbnailIntoView(page); + PDFViewerApplication.pdfThumbnailViewer.scrollThumbnailIntoView(pageNumber); } } From 8467f45ab7ea1ab5f83938d47257775f8d24e1fc Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 23 Sep 2020 18:08:32 +0200 Subject: [PATCH 5/6] Change the `finishRenderTask` helper function, in `PDFThumbnailView.draw`, to be asynchronous This simplifies the implementation slightly, and is also (almost) identical to the `finishPaintTask` helper function in `PDFPageView.draw`. --- web/pdf_thumbnail_view.js | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/web/pdf_thumbnail_view.js b/web/pdf_thumbnail_view.js index dddd931b5..9e70626df 100644 --- a/web/pdf_thumbnail_view.js +++ b/web/pdf_thumbnail_view.js @@ -13,11 +13,8 @@ * limitations under the License. */ -import { - createPromiseCapability, - RenderingCancelledException, -} from "pdfjs-lib"; import { getOutputScale, NullL10n } from "./ui_utils.js"; +import { RenderingCancelledException } from "pdfjs-lib"; import { RenderingStates } from "./pdf_rendering_queue.js"; const MAX_NUM_SCALING_STEPS = 3; @@ -316,8 +313,7 @@ class PDFThumbnailView { this.renderingState = RenderingStates.RUNNING; - const renderCapability = createPromiseCapability(); - const finishRenderTask = (error = null) => { + const finishRenderTask = async (error = null) => { // The renderTask may have been replaced by a new one, so only remove // the reference to the renderTask if it matches the one that is // triggering this callback. @@ -326,17 +322,14 @@ class PDFThumbnailView { } if (error instanceof RenderingCancelledException) { - renderCapability.resolve(undefined); return; } this.renderingState = RenderingStates.FINISHED; this._convertCanvasToImage(); - if (!error) { - renderCapability.resolve(undefined); - } else { - renderCapability.reject(error); + if (error) { + throw error; } }; @@ -362,7 +355,7 @@ class PDFThumbnailView { const renderTask = (this.renderTask = pdfPage.render(renderContext)); renderTask.onContinue = renderContinueCallback; - renderTask.promise.then( + const resultPromise = renderTask.promise.then( function () { finishRenderTask(null); }, @@ -370,7 +363,7 @@ class PDFThumbnailView { finishRenderTask(error); } ); - return renderCapability.promise; + return resultPromise; } setImage(pageView) { From 2043596035e82d5a29b51e2f52368dde44f3d321 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 23 Sep 2020 19:03:20 +0200 Subject: [PATCH 6/6] Use template strings when calculating the CSS `transform`s, in the `PDFPageView.cssTransform` method In my opinion this slightly improves readability, by grouping related properties together. --- web/pdf_page_view.js | 27 ++++----------------------- 1 file changed, 4 insertions(+), 23 deletions(-) diff --git a/web/pdf_page_view.js b/web/pdf_page_view.js index 658b06b19..9b6eb3f02 100644 --- a/web/pdf_page_view.js +++ b/web/pdf_page_view.js @@ -349,16 +349,7 @@ class PDFPageView { scaleX = height / width; scaleY = width / height; } - const cssTransform = - "rotate(" + - relativeRotation + - "deg) " + - "scale(" + - scaleX + - "," + - scaleY + - ")"; - target.style.transform = cssTransform; + target.style.transform = `rotate(${relativeRotation}deg) scale(${scaleX}, ${scaleY})`; if (this.textLayer) { // Rotating the text layer is more complicated since the divs inside the @@ -397,19 +388,9 @@ class PDFPageView { } textLayerDiv.style.transform = - "rotate(" + - textAbsRotation + - "deg) " + - "scale(" + - scale + - ", " + - scale + - ") " + - "translate(" + - transX + - ", " + - transY + - ")"; + `rotate(${textAbsRotation}deg) ` + + `scale(${scale}) ` + + `translate(${transX}, ${transY})`; textLayerDiv.style.transformOrigin = "0% 0%"; }