Re-factor the idFactory functionality, used in the core/-code, and move the fontID generation into it

Note how the `getFontID`-method in `src/core/fonts.js` is *completely* global, rather than properly tied to the current document. This means that if you repeatedly open and parse/render, and then close, even the *same* PDF document the `fontID`s will still be incremented continuously.

For comparison the `createObjId` method, on `idFactory`, will always create a *consistent* id, assuming of course that the document and its pages are parsed/rendered in the same order.

In order to address this inconsistency, it thus seems reasonable to add a new `createFontId` method on the `idFactory` and use that when obtaining `fontID`s. (When the current `getFontID` method was added the `idFactory` didn't actually exist yet, which explains why the code looks the way it does.)
*Please note:* Since the document id is (still) part of the `loadedName`, it's thus not possible for different documents to have identical font names.

src/core/document.js

@@ -28,6 +28,7 @@ import {
   shadow,
   stringToBytes,
   stringToPDFString,
+  unreachable,
   Util,
   warn,
 } from "../shared/util.js";
@@ -71,6 +72,7 @@ class Page {
     pageIndex,
     pageDict,
     ref,
+    globalIdFactory,
     fontCache,
     builtInCMapCache,
     globalImageCache,
@@ -89,13 +91,10 @@ class Page {
     const idCounters = {
       obj: 0,
     };
-    this.idFactory = {
+    this._localIdFactory = class extends globalIdFactory {
-      createObjId() {
+      static createObjId() {
         return `p${pageIndex}_${++idCounters.obj}`;
-      },
+      }
-      getDocId() {
-        return `g_${pdfManager.docId}`;
-      },
     };
   }
 
@@ -257,7 +256,7 @@ class Page {
       xref: this.xref,
       handler,
       pageIndex: this.pageIndex,
-      idFactory: this.idFactory,
+      idFactory: this._localIdFactory,
       fontCache: this.fontCache,
       builtInCMapCache: this.builtInCMapCache,
       globalImageCache: this.globalImageCache,
@@ -350,7 +349,7 @@ class Page {
         xref: this.xref,
         handler,
         pageIndex: this.pageIndex,
-        idFactory: this.idFactory,
+        idFactory: this._localIdFactory,
         fontCache: this.fontCache,
         builtInCMapCache: this.builtInCMapCache,
         globalImageCache: this.globalImageCache,
@@ -399,7 +398,7 @@ class Page {
               this.xref,
               annotationRef,
               this.pdfManager,
-              this.idFactory
+              this._localIdFactory
             ).catch(function (reason) {
               warn(`_parsedAnnotations: "${reason}".`);
               return null;
@@ -504,6 +503,23 @@ class PDFDocument {
     this.stream = stream;
     this.xref = new XRef(stream, pdfManager);
     this._pagePromises = [];
+
+    const idCounters = {
+      font: 0,
+    };
+    this._globalIdFactory = class {
+      static getDocId() {
+        return `g_${pdfManager.docId}`;
+      }
+
+      static createFontId() {
+        return `f${++idCounters.font}`;
+      }
+
+      static createObjId() {
+        unreachable("Abstract method `createObjId` called.");
+      }
+    };
   }
 
   parse(recoveryMode) {
@@ -808,6 +824,7 @@ class PDFDocument {
         pageIndex,
         pageDict,
         ref,
+        globalIdFactory: this._globalIdFactory,
         fontCache: catalog.fontCache,
         builtInCMapCache: catalog.builtInCMapCache,
         globalImageCache: catalog.globalImageCache,

src/core/evaluator.js

@@ -587,7 +587,7 @@ class PartialEvaluator {
       cacheGlobally = false;
 
     if (this.parsingType3Font) {
-      objId = `${this.idFactory.getDocId()}_type3res_${objId}`;
+      objId = `${this.idFactory.getDocId()}_type3_${objId}`;
     } else if (imageRef) {
       cacheGlobally = this.globalImageCache.shouldCache(
         imageRef,
@@ -998,7 +998,7 @@ class PartialEvaluator {
     var fontRefIsRef = isRef(fontRef),
       fontID;
     if (fontRefIsRef) {
-      fontID = fontRef.toString();
+      fontID = `f${fontRef.toString()}`;
     }
 
     if (hash && isDict(descriptor)) {
@@ -1015,7 +1015,7 @@ class PartialEvaluator {
         }
       } else {
         fontAliases[hash] = {
-          fontID: Font.getFontID(),
+          fontID: this.idFactory.createFontId(),
         };
       }
 
@@ -1046,15 +1046,18 @@ class PartialEvaluator {
       this.fontCache.put(fontRef, fontCapability.promise);
     } else {
       if (!fontID) {
-        fontID = this.idFactory.createObjId();
+        fontID = this.idFactory.createFontId();
       }
       this.fontCache.put(`id_${fontID}`, fontCapability.promise);
     }
-    assert(fontID, 'The "fontID" must be defined.');
+    assert(
+      fontID && fontID.startsWith("f"),
+      'The "fontID" must be (correctly) defined.'
+    );
 
     // Keep track of each font we translated so the caller can
     // load them asynchronously before calling display on a page.
-    font.loadedName = `${this.idFactory.getDocId()}_f${fontID}`;
+    font.loadedName = `${this.idFactory.getDocId()}_${fontID}`;
 
     font.translated = fontCapability.promise;
 

src/core/fonts.js

@@ -694,13 +694,6 @@ var Font = (function FontClosure() {
     this.seacMap = properties.seacMap;
   }
 
-  Font.getFontID = (function () {
-    var ID = 1;
-    return function Font_getFontID() {
-      return String(ID++);
-    };
-  })();
-
   function int16(b0, b1) {
     return (b0 << 8) + b1;
   }

test/unit/document_spec.js

@@ -17,20 +17,26 @@ import { createIdFactory } from "./test_utils.js";
 
 describe("document", function () {
   describe("Page", function () {
-    it("should create correct objId using the idFactory", function () {
+    it("should create correct objId/fontId using the idFactory", function () {
       const idFactory1 = createIdFactory(/* pageIndex = */ 0);
       const idFactory2 = createIdFactory(/* pageIndex = */ 1);
 
       expect(idFactory1.createObjId()).toEqual("p0_1");
       expect(idFactory1.createObjId()).toEqual("p0_2");
+      expect(idFactory1.createFontId()).toEqual("f1");
+      expect(idFactory1.createFontId()).toEqual("f2");
       expect(idFactory1.getDocId()).toEqual("g_d0");
 
       expect(idFactory2.createObjId()).toEqual("p1_1");
       expect(idFactory2.createObjId()).toEqual("p1_2");
+      expect(idFactory2.createFontId()).toEqual("f1");
+      expect(idFactory2.createFontId()).toEqual("f2");
       expect(idFactory2.getDocId()).toEqual("g_d0");
 
       expect(idFactory1.createObjId()).toEqual("p0_3");
       expect(idFactory1.createObjId()).toEqual("p0_4");
+      expect(idFactory1.createFontId()).toEqual("f3");
+      expect(idFactory1.createFontId()).toEqual("f4");
       expect(idFactory1.getDocId()).toEqual("g_d0");
     });
   });

test/unit/test_utils.js

@@ -13,10 +13,11 @@
  * limitations under the License.
  */
 
+import { Page, PDFDocument } from "../../src/core/document.js";
 import { assert } from "../../src/shared/util.js";
 import { isNodeJS } from "../../src/shared/is_node.js";
 import { isRef } from "../../src/core/primitives.js";
-import { Page } from "../../src/core/document.js";
+import { StringStream } from "../../src/core/stream.js";
 
 class DOMFileReaderFactory {
   static async fetch(params) {
@@ -93,15 +94,21 @@ class XRefMock {
 }
 
 function createIdFactory(pageIndex) {
-  const page = new Page({
+  const pdfManager = {
-    pdfManager: {
+    get docId() {
-      get docId() {
+      return "d0";
-        return "d0";
-      },
     },
+  };
+  const stream = new StringStream("Dummy_PDF_data");
+  const pdfDocument = new PDFDocument(pdfManager, stream);
+
+  const page = new Page({
+    pdfManager: pdfDocument.pdfManager,
+    xref: pdfDocument.xref,
     pageIndex,
+    globalIdFactory: pdfDocument._globalIdFactory,
   });
-  return page.idFactory;
+  return page._localIdFactory;
 }
 
 function isEmptyObj(obj) {