From 6167566f1bbb424aa2a2772d1dd953aafe615a03 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Mon, 9 Aug 2021 12:02:49 +0200 Subject: [PATCH 1/3] Re-factor the `BaseException.name` handling, and clean-up some code Once we're finally able to get rid of SystemJS, which is unfortunately still blocked on [bug 1247687](https://bugzilla.mozilla.org/show_bug.cgi?id=1247687), we might also want to clean-up (or even completely remove) the `BaseException` abstraction and simply extend `Error` directly instead. At that point we'd need to (explicitly) set the `name` on each class anyway, so this patch is essentially preparing for future clean-up. Furthermore, after the `BaseException` abstraction was added there's been *multiple* issues filed about third-party minification breaking our code since `this.constructor.name` is not guaranteed to always do what you intended. While hard-coding the strings indeed feels quite unfortunate, it's likely the "best" solution to avoid the problem described above. --- src/core/core_utils.js | 20 ++++++++++++++++---- src/core/jbig2.js | 2 +- src/core/jpg.js | 10 +++++++--- src/core/jpx.js | 2 +- src/display/api.js | 13 ++----------- src/display/display_utils.js | 2 +- src/shared/message_handler.js | 24 ++++++++++++++---------- src/shared/util.js | 34 +++++++++++++++++++++++++--------- 8 files changed, 67 insertions(+), 40 deletions(-) diff --git a/src/core/core_utils.js b/src/core/core_utils.js index 253a349bc..7282eb0e5 100644 --- a/src/core/core_utils.js +++ b/src/core/core_utils.js @@ -52,17 +52,29 @@ function getArrayLookupTableFactory(initializer) { class MissingDataException extends BaseException { constructor(begin, end) { - super(`Missing data [${begin}, ${end})`); + super(`Missing data [${begin}, ${end})`, "MissingDataException"); this.begin = begin; this.end = end; } } -class ParserEOFException extends BaseException {} +class ParserEOFException extends BaseException { + constructor(msg) { + super(msg, "ParserEOFException"); + } +} -class XRefEntryException extends BaseException {} +class XRefEntryException extends BaseException { + constructor(msg) { + super(msg, "XRefEntryException"); + } +} -class XRefParseException extends BaseException {} +class XRefParseException extends BaseException { + constructor(msg) { + super(msg, "XRefParseException"); + } +} /** * Get the value of an inheritable property. diff --git a/src/core/jbig2.js b/src/core/jbig2.js index dbc430858..72f9e8dad 100644 --- a/src/core/jbig2.js +++ b/src/core/jbig2.js @@ -20,7 +20,7 @@ import { CCITTFaxDecoder } from "./ccitt.js"; class Jbig2Error extends BaseException { constructor(msg) { - super(`JBIG2 error: ${msg}`); + super(`JBIG2 error: ${msg}`, "Jbig2Error"); } } diff --git a/src/core/jpg.js b/src/core/jpg.js index 22e2af9a5..5084181ca 100644 --- a/src/core/jpg.js +++ b/src/core/jpg.js @@ -18,18 +18,22 @@ import { readUint16 } from "./core_utils.js"; class JpegError extends BaseException { constructor(msg) { - super(`JPEG error: ${msg}`); + super(`JPEG error: ${msg}`, "JpegError"); } } class DNLMarkerError extends BaseException { constructor(message, scanLines) { - super(message); + super(message, "DNLMarkerError"); this.scanLines = scanLines; } } -class EOIMarkerError extends BaseException {} +class EOIMarkerError extends BaseException { + constructor(msg) { + super(msg, "EOIMarkerError"); + } +} /** * This code was forked from https://github.com/notmasteryet/jpgjs. diff --git a/src/core/jpx.js b/src/core/jpx.js index 63dc11e8e..99bcf3c45 100644 --- a/src/core/jpx.js +++ b/src/core/jpx.js @@ -19,7 +19,7 @@ import { ArithmeticDecoder } from "./arithmetic_decoder.js"; class JpxError extends BaseException { constructor(msg) { - super(`JPX error: ${msg}`); + super(`JPX error: ${msg}`, "JpxError"); } } diff --git a/src/display/api.js b/src/display/api.js index 978b05802..1fa6367e3 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -2565,17 +2565,8 @@ class WorkerTransport { case "UnknownErrorException": reason = new UnknownErrorException(ex.message, ex.details); break; - } - if (!(reason instanceof Error)) { - const msg = "DocException - expected a valid Error."; - if ( - typeof PDFJSDev === "undefined" || - PDFJSDev.test("!PRODUCTION || TESTING") - ) { - unreachable(msg); - } else { - warn(msg); - } + default: + unreachable("DocException - expected a valid Error."); } loadingTask._capability.reject(reason); }); diff --git a/src/display/display_utils.js b/src/display/display_utils.js index e4cc62085..c94834a74 100644 --- a/src/display/display_utils.js +++ b/src/display/display_utils.js @@ -300,7 +300,7 @@ class PageViewport { class RenderingCancelledException extends BaseException { constructor(msg, type) { - super(msg); + super(msg, "RenderingCancelledException"); this.type = type; } } diff --git a/src/shared/message_handler.js b/src/shared/message_handler.js index 9f65d0c0a..372427f8a 100644 --- a/src/shared/message_handler.js +++ b/src/shared/message_handler.js @@ -20,6 +20,7 @@ import { MissingPDFException, UnexpectedResponseException, UnknownErrorException, + warn, } from "./util.js"; const CallbackKind = { @@ -42,18 +43,21 @@ const StreamKind = { function wrapReason(reason) { if ( - typeof PDFJSDev === "undefined" || - PDFJSDev.test("!PRODUCTION || TESTING") - ) { - assert( + !( reason instanceof Error || - (typeof reason === "object" && reason !== null), - 'wrapReason: Expected "reason" to be a (possibly cloned) Error.' - ); - } else { - if (typeof reason !== "object" || reason === null) { - return reason; + (typeof reason === "object" && reason !== null) + ) + ) { + if ( + typeof PDFJSDev === "undefined" || + PDFJSDev.test("!PRODUCTION || TESTING") + ) { + throw new Error( + 'wrapReason: Expected "reason" to be a (possibly cloned) Error.' + ); } + warn('wrapReason: Expected "reason" to be a (possibly cloned) Error.'); + return reason; } switch (reason.name) { case "AbortException": diff --git a/src/shared/util.js b/src/shared/util.js index 92d0ae9a0..a696fe659 100644 --- a/src/shared/util.js +++ b/src/shared/util.js @@ -459,12 +459,12 @@ function shadow(obj, prop, value) { */ const BaseException = (function BaseExceptionClosure() { // eslint-disable-next-line no-shadow - function BaseException(message) { + function BaseException(message, name) { if (this.constructor === BaseException) { unreachable("Cannot initialize BaseException."); } this.message = message; - this.name = this.constructor.name; + this.name = name; } BaseException.prototype = new Error(); BaseException.constructor = BaseException; @@ -474,25 +474,33 @@ const BaseException = (function BaseExceptionClosure() { class PasswordException extends BaseException { constructor(msg, code) { - super(msg); + super(msg, "PasswordException"); this.code = code; } } class UnknownErrorException extends BaseException { constructor(msg, details) { - super(msg); + super(msg, "UnknownErrorException"); this.details = details; } } -class InvalidPDFException extends BaseException {} +class InvalidPDFException extends BaseException { + constructor(msg) { + super(msg, "InvalidPDFException"); + } +} -class MissingPDFException extends BaseException {} +class MissingPDFException extends BaseException { + constructor(msg) { + super(msg, "MissingPDFException"); + } +} class UnexpectedResponseException extends BaseException { constructor(msg, status) { - super(msg); + super(msg, "UnexpectedResponseException"); this.status = status; } } @@ -500,12 +508,20 @@ class UnexpectedResponseException extends BaseException { /** * Error caused during parsing PDF data. */ -class FormatError extends BaseException {} +class FormatError extends BaseException { + constructor(msg) { + super(msg, "FormatError"); + } +} /** * Error used to indicate task cancellation. */ -class AbortException extends BaseException {} +class AbortException extends BaseException { + constructor(msg) { + super(msg, "AbortException"); + } +} const NullCharactersRegExp = /\x00/g; From 5ac139dea18e3f57edd3e32510f8f40d5f05114f Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Mon, 9 Aug 2021 12:05:50 +0200 Subject: [PATCH 2/3] Remove the `BaseViewer._name` property, used only when logging errors The original idea behind including the class name, when logging errors, was to improve things in the *hypothetical case* where `PDFViewer`- and `PDFSinglePageViewer`-instances would be used side-by-side. Given that all of the relevant methods are synchronous this seem unlikely to really be necessary, and furthermore it's probably best to avoid using `this.constructor.name` since that's not guaranteed to do what you intend (we've seen repeated issues with minifiers mangling function/class names). --- web/base_viewer.js | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/web/base_viewer.js b/web/base_viewer.js index b872ccac5..04e184dc2 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -152,8 +152,6 @@ class BaseViewer { `The API version "${version}" does not match the Viewer version "${viewerVersion}".` ); } - this._name = this.constructor.name; - this.container = options.container; this.viewer = options.viewer || options.container.firstElementChild; @@ -267,9 +265,7 @@ class BaseViewer { } // The intent can be to just reset a scroll position and/or scale. if (!this._setCurrentPageNumber(val, /* resetCurrentPageView = */ true)) { - console.error( - `${this._name}.currentPageNumber: "${val}" is not a valid page.` - ); + console.error(`currentPageNumber: "${val}" is not a valid page.`); } } @@ -328,9 +324,7 @@ class BaseViewer { } // The intent can be to just reset a scroll position and/or scale. if (!this._setCurrentPageNumber(page, /* resetCurrentPageView = */ true)) { - console.error( - `${this._name}.currentPageLabel: "${val}" is not a valid page.` - ); + console.error(`currentPageLabel: "${val}" is not a valid page.`); } } @@ -642,7 +636,7 @@ class BaseViewer { !(Array.isArray(labels) && this.pdfDocument.numPages === labels.length) ) { this._pageLabels = null; - console.error(`${this._name}.setPageLabels: Invalid page labels.`); + console.error(`setPageLabels: Invalid page labels.`); } else { this._pageLabels = labels; } @@ -808,9 +802,7 @@ class BaseViewer { scale = Math.min(MAX_AUTO_SCALE, horizontalScale); break; default: - console.error( - `${this._name}._setScale: "${value}" is an unknown zoom value.` - ); + console.error(`_setScale: "${value}" is an unknown zoom value.`); return; } this._setScaleUpdatePages(scale, value, noScroll, /* preset = */ true); @@ -875,8 +867,7 @@ class BaseViewer { Number.isInteger(pageNumber) && this._pages[pageNumber - 1]; if (!pageView) { console.error( - `${this._name}.scrollPageIntoView: ` + - `"${pageNumber}" is not a valid pageNumber parameter.` + `scrollPageIntoView: "${pageNumber}" is not a valid pageNumber parameter.` ); return; } @@ -955,8 +946,7 @@ class BaseViewer { break; default: console.error( - `${this._name}.scrollPageIntoView: ` + - `"${destArray[1].name}" is not a valid destination type.` + `scrollPageIntoView: "${destArray[1].name}" is not a valid destination type.` ); return; } @@ -1143,9 +1133,7 @@ class BaseViewer { pageNumber <= this.pagesCount ) ) { - console.error( - `${this._name}.isPageVisible: "${pageNumber}" is not a valid page.` - ); + console.error(`isPageVisible: "${pageNumber}" is not a valid page.`); return false; } return this._getVisiblePages().views.some(function (view) { @@ -1167,9 +1155,7 @@ class BaseViewer { pageNumber <= this.pagesCount ) ) { - console.error( - `${this._name}.isPageCached: "${pageNumber}" is not a valid page.` - ); + console.error(`isPageCached: "${pageNumber}" is not a valid page.`); return false; } const pageView = this._pages[pageNumber - 1]; From 474659be8b041bb33f9529a391faa1b99e01bc97 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Mon, 9 Aug 2021 13:16:02 +0200 Subject: [PATCH 3/3] Fix the inconsistent return types in `PDFViewerApplication._parseHashParameters` While not really relevant to the previous patches, this fixes a small inconsistency in the code. --- web/app.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/web/app.js b/web/app.js index f18da369e..060af045a 100644 --- a/web/app.js +++ b/web/app.js @@ -323,11 +323,11 @@ const PDFViewerApplication = { */ async _parseHashParameters() { if (!AppOptions.get("pdfBugEnabled")) { - return undefined; + return; } const hash = document.location.hash.substring(1); if (!hash) { - return undefined; + return; } const params = parseQueryString(hash), waitOn = []; @@ -389,11 +389,13 @@ const PDFViewerApplication = { } if (waitOn.length === 0) { - return undefined; + return; } - return Promise.all(waitOn).catch(reason => { + try { + await Promise.all(waitOn); + } catch (reason) { console.error(`_parseHashParameters: "${reason.message}".`); - }); + } }, /**