From 3838c4e27c4a530afd4683db6362a02b62b1758b Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 14 Jul 2021 21:38:19 +0200 Subject: [PATCH] Re-factor the handling of *empty* `Name`-instances (PR 13612 follow-up) When working on PR 13612, I mostly prioritized a simple solution that didn't require touching a lot of code. However, while working on PR 13735 I started to realize that the static `Name.empty` construction really wasn't a good idea. In particular, having a special `Name`-instance where the `name`-property isn't actually a String is confusing (to put it mildly) and can easily lead to issues elsewhere. The only reason for not simply allowing the `name`-property to be an *empty* string, in PR 13612, was to avoid having to touch a lot of existing code. However, it turns out that this is only limited to a few methods in the `PartialEvaluator` and a few of the `BaseLocalCache`-implementations, all of which can be easily re-factored to handle *empty* `Name`-instances. All-in-all, I think that this patch is even an *overall* improvement since we're now validating (what should always be) `Name`-data better in the `PartialEvaluator`. This is what I ought to have done from the start, sorry about the code churn here! --- src/core/evaluator.js | 26 +++++++++++++++++--------- src/core/image_utils.js | 10 +++++----- src/core/parser.js | 1 - src/core/primitives.js | 6 ------ test/unit/primitives_spec.js | 17 +++++++++++++++++ 5 files changed, 39 insertions(+), 21 deletions(-) diff --git a/src/core/evaluator.js b/src/core/evaluator.js index 614599423..0f6330c2f 100644 --- a/src/core/evaluator.js +++ b/src/core/evaluator.js @@ -1540,7 +1540,7 @@ class PartialEvaluator { timeSlotManager.reset(); const operation = {}; - let stop, i, ii, cs, name; + let stop, i, ii, cs, name, isValidName; while (!(stop = timeSlotManager.check())) { // The arguments parsed by read() are used beyond this loop, so we // cannot reuse the same array on each iteration. Therefore we pass @@ -1556,8 +1556,10 @@ class PartialEvaluator { switch (fn | 0) { case OPS.paintXObject: // eagerly compile XForm objects + isValidName = args[0] instanceof Name; name = args[0].name; - if (name) { + + if (isValidName) { const localImage = localImageCache.getByName(name); if (localImage) { operatorList.addOp(localImage.fn, localImage.args); @@ -1568,7 +1570,7 @@ class PartialEvaluator { next( new Promise(function (resolveXObject, rejectXObject) { - if (!name) { + if (!isValidName) { throw new FormatError("XObject must be referred to by name."); } @@ -1922,8 +1924,10 @@ class PartialEvaluator { fn = OPS.shadingFill; break; case OPS.setGState: + isValidName = args[0] instanceof Name; name = args[0].name; - if (name) { + + if (isValidName) { const localGStateObj = localGStateCache.getByName(name); if (localGStateObj) { if (localGStateObj.length > 0) { @@ -1936,7 +1940,7 @@ class PartialEvaluator { next( new Promise(function (resolveGState, rejectGState) { - if (!name) { + if (!isValidName) { throw new FormatError("GState must be referred to by name."); } @@ -2823,14 +2827,16 @@ class PartialEvaluator { xobjs = resources.get("XObject") || Dict.empty; } + var isValidName = args[0] instanceof Name; var name = args[0].name; - if (name && emptyXObjectCache.getByName(name)) { + + if (isValidName && emptyXObjectCache.getByName(name)) { break; } next( new Promise(function (resolveXObject, rejectXObject) { - if (!name) { + if (!isValidName) { throw new FormatError("XObject must be referred to by name."); } @@ -2935,14 +2941,16 @@ class PartialEvaluator { ); return; case OPS.setGState: + isValidName = args[0] instanceof Name; name = args[0].name; - if (name && emptyGStateCache.getByName(name)) { + + if (isValidName && emptyGStateCache.getByName(name)) { break; } next( new Promise(function (resolveGState, rejectGState) { - if (!name) { + if (!isValidName) { throw new FormatError("GState must be referred to by name."); } diff --git a/src/core/image_utils.js b/src/core/image_utils.js index 17ae9bf7c..1cfdbb83f 100644 --- a/src/core/image_utils.js +++ b/src/core/image_utils.js @@ -47,7 +47,7 @@ class BaseLocalCache { class LocalImageCache extends BaseLocalCache { set(name, ref = null, data) { - if (!name) { + if (typeof name !== "string") { throw new Error('LocalImageCache.set - expected "name" argument.'); } if (ref) { @@ -68,7 +68,7 @@ class LocalImageCache extends BaseLocalCache { class LocalColorSpaceCache extends BaseLocalCache { set(name = null, ref = null, data) { - if (!name && !ref) { + if (typeof name !== "string" && !ref) { throw new Error( 'LocalColorSpaceCache.set - expected "name" and/or "ref" argument.' ); @@ -77,7 +77,7 @@ class LocalColorSpaceCache extends BaseLocalCache { if (this._imageCache.has(ref)) { return; } - if (name) { + if (name !== null) { // Optional when `ref` is defined. this._nameRefMap.set(name, ref); } @@ -114,7 +114,7 @@ class LocalFunctionCache extends BaseLocalCache { class LocalGStateCache extends BaseLocalCache { set(name, ref = null, data) { - if (!name) { + if (typeof name !== "string") { throw new Error('LocalGStateCache.set - expected "name" argument.'); } if (ref) { @@ -135,7 +135,7 @@ class LocalGStateCache extends BaseLocalCache { class LocalTilingPatternCache extends BaseLocalCache { set(name, ref = null, data) { - if (!name) { + if (typeof name !== "string") { throw new Error( 'LocalTilingPatternCache.set - expected "name" argument.' ); diff --git a/src/core/parser.js b/src/core/parser.js index bc8381070..5671d1546 100644 --- a/src/core/parser.js +++ b/src/core/parser.js @@ -1115,7 +1115,6 @@ class Lexer { warn(`Name token is longer than allowed by the spec: ${strBuf.length}`); } else if (strBuf.length === 0) { warn("Name token is empty."); - return Name.empty; } return Name.get(strBuf.join("")); } diff --git a/src/core/primitives.js b/src/core/primitives.js index 4e96b7b85..c35dedef5 100644 --- a/src/core/primitives.js +++ b/src/core/primitives.js @@ -33,12 +33,6 @@ const Name = (function NameClosure() { return nameValue ? nameValue : (nameCache[name] = new Name(name)); } - static get empty() { - // eslint-disable-next-line no-restricted-syntax - const emptyName = new Name({ empty: true }); - return shadow(this, "empty", emptyName); - } - static _clearCache() { nameCache = Object.create(null); } diff --git a/test/unit/primitives_spec.js b/test/unit/primitives_spec.js index beb148bc2..e187c1ac9 100644 --- a/test/unit/primitives_spec.js +++ b/test/unit/primitives_spec.js @@ -50,6 +50,15 @@ describe("primitives", function () { expect(firstSubtype).toBe(secondSubtype); expect(firstFont).not.toBe(firstSubtype); }); + + it("should create only one object for *empty* names and cache it", function () { + const firstEmpty = Name.get(""); + const secondEmpty = Name.get(""); + const normalName = Name.get("string"); + + expect(firstEmpty).toBe(secondEmpty); + expect(firstEmpty).not.toBe(normalName); + }); }); describe("Cmd", function () { @@ -491,6 +500,14 @@ describe("primitives", function () { expect(isName(name, "Font")).toEqual(true); expect(isName(name, "Subtype")).toEqual(false); }); + + it("handles *empty* names, with name check", function () { + const emptyName = Name.get(""); + + expect(isName(emptyName)).toEqual(true); + expect(isName(emptyName, "")).toEqual(true); + expect(isName(emptyName, "string")).toEqual(false); + }); }); describe("isCmd", function () {