Merge pull request #13899 from Snuffleupagus/includeAnnotationStorage-fix-caching

[Regression] Re-factor the *internal* `includeAnnotationStorage` handling, since it's currently subtly wrong

.eslintrc

@@ -49,6 +49,7 @@
     "unicorn/no-new-buffer": "error",
     "unicorn/no-instanceof-array": "error",
     "unicorn/no-useless-spread": "error",
+    "unicorn/prefer-date-now": "error",
     "unicorn/prefer-string-starts-ends-with": "error",
 
     // Possible errors

src/core/document.js

@@ -320,7 +320,14 @@ class Page {
     });
   }
 
-  getOperatorList({ handler, sink, task, intent, annotationStorage }) {
+  getOperatorList({
+    handler,
+    sink,
+    task,
+    intent,
+    cacheKey,
+    annotationStorage = null,
+  }) {
     const contentStreamPromise = this.getContentStream(handler);
     const resourcesPromise = this.loadResources([
       "ColorSpace",
@@ -354,7 +361,7 @@ class Page {
           this.nonBlendModesSet
         ),
         pageIndex: this.pageIndex,
-        intent,
+        cacheKey,
       });
 
       return partialEvaluator
@@ -377,7 +384,7 @@ class Page {
           pageOpList.flush(true);
           return { length: pageOpList.totalLength };
         }
-        const renderForms = !!(intent & RenderingIntentFlag.ANNOTATION_FORMS),
+        const renderForms = !!(intent & RenderingIntentFlag.ANNOTATIONS_FORMS),
           intentAny = !!(intent & RenderingIntentFlag.ANY),
           intentDisplay = !!(intent & RenderingIntentFlag.DISPLAY),
           intentPrint = !!(intent & RenderingIntentFlag.PRINT);

src/core/worker.js

@@ -688,6 +688,7 @@ class WorkerMessageHandler {
             sink,
             task,
             intent: data.intent,
+            cacheKey: data.cacheKey,
             annotationStorage: data.annotationStorage,
           })
           .then(

src/display/annotation_storage.js

@@ -21,6 +21,7 @@ import { objectFromMap } from "../shared/util.js";
 class AnnotationStorage {
   constructor() {
     this._storage = new Map();
+    this._timeStamp = Date.now();
     this._modified = false;
 
     // Callbacks to signal when the modification state is set or reset.
@@ -41,8 +42,7 @@ class AnnotationStorage {
    * @returns {Object}
    */
   getValue(key, defaultValue) {
-    const obj = this._storage.get(key);
+    return this._storage.get(key) ?? defaultValue;
-    return obj !== undefined ? obj : defaultValue;
   }
 
   /**
@@ -64,10 +64,11 @@ class AnnotationStorage {
         }
       }
     } else {
-      this._storage.set(key, value);
       modified = true;
+      this._storage.set(key, value);
     }
     if (modified) {
+      this._timeStamp = Date.now();
       this._setModified();
     }
   }
@@ -108,6 +109,14 @@ class AnnotationStorage {
   get serializable() {
     return this._storage.size > 0 ? this._storage : null;
   }
+
+  /**
+   * PLEASE NOTE: Only intended for usage within the API itself.
+   * @ignore
+   */
+  get lastModified() {
+    return this._timeStamp.toString();
+  }
 }
 
 export { AnnotationStorage };

src/display/api.js

@@ -515,29 +515,6 @@ function _fetchDocument(worker, source, pdfDataRangeTransport, docId) {
     });
 }
 
-function getRenderingIntent(intent, { renderForms = false, isOpList = false }) {
-  let renderingIntent = RenderingIntentFlag.DISPLAY; // Default value.
-  switch (intent) {
-    case "any":
-      renderingIntent = RenderingIntentFlag.ANY;
-      break;
-    case "display":
-      break;
-    case "print":
-      renderingIntent = RenderingIntentFlag.PRINT;
-      break;
-    default:
-      warn(`getRenderingIntent - invalid intent: ${intent}`);
-  }
-  if (renderForms) {
-    renderingIntent += RenderingIntentFlag.ANNOTATION_FORMS;
-  }
-  if (isOpList) {
-    renderingIntent += RenderingIntentFlag.OPLIST;
-  }
-  return renderingIntent;
-}
-
 /**
  * @typedef {Object} OnProgressParameters
  * @property {number} loaded - Currently loaded number of bytes.
@@ -1302,15 +1279,15 @@ class PDFPageProxy {
    *   {Array} of the annotation objects.
    */
   getAnnotations({ intent = "display" } = {}) {
-    const renderingIntent = getRenderingIntent(intent, {});
+    const intentArgs = this._transport.getRenderingIntent(intent, {});
 
-    let promise = this._annotationPromises.get(renderingIntent);
+    let promise = this._annotationPromises.get(intentArgs.cacheKey);
     if (!promise) {
       promise = this._transport.getAnnotations(
         this._pageIndex,
-        renderingIntent
+        intentArgs.renderingIntent
       );
-      this._annotationPromises.set(renderingIntent, promise);
+      this._annotationPromises.set(intentArgs.cacheKey, promise);
     }
     return promise;
   }
@@ -1358,8 +1335,9 @@ class PDFPageProxy {
       this._stats.time("Overall");
     }
 
-    const renderingIntent = getRenderingIntent(intent, {
+    const intentArgs = this._transport.getRenderingIntent(intent, {
       renderForms: renderInteractiveForms === true,
+      includeAnnotationStorage: includeAnnotationStorage === true,
     });
     // If there was a pending destroy, cancel it so no cleanup happens during
     // this call to render.
@@ -1369,10 +1347,10 @@ class PDFPageProxy {
       optionalContentConfigPromise = this._transport.getOptionalContentConfig();
     }
 
-    let intentState = this._intentStates.get(renderingIntent);
+    let intentState = this._intentStates.get(intentArgs.cacheKey);
     if (!intentState) {
       intentState = Object.create(null);
-      this._intentStates.set(renderingIntent, intentState);
+      this._intentStates.set(intentArgs.cacheKey, intentState);
     }
 
     // Ensure that a pending `streamReader` cancel timeout is always aborted.
@@ -1384,9 +1362,9 @@ class PDFPageProxy {
     const canvasFactoryInstance =
       canvasFactory ||
       new DefaultCanvasFactory({ ownerDocument: this._ownerDocument });
-    const annotationStorage = includeAnnotationStorage
+    const intentPrint = !!(
-      ? this._transport.annotationStorage.serializable
+      intentArgs.renderingIntent & RenderingIntentFlag.PRINT
-      : null;
+    );
 
     // If there's no displayReadyCapability yet, then the operatorList
     // was never requested before. Make the request and create the promise.
@@ -1401,11 +1379,7 @@ class PDFPageProxy {
       if (this._stats) {
         this._stats.time("Page Request");
       }
-      this._pumpOperatorList({
+      this._pumpOperatorList(intentArgs);
-        pageIndex: this._pageIndex,
-        intent: renderingIntent,
-        annotationStorage,
-      });
     }
 
     const complete = error => {
@@ -1413,10 +1387,7 @@ class PDFPageProxy {
 
       // Attempt to reduce memory usage during *printing*, by always running
       // cleanup once rendering has finished (regardless of cleanupAfterRender).
-      if (
+      if (this.cleanupAfterRender || intentPrint) {
-        this.cleanupAfterRender ||
-        renderingIntent & RenderingIntentFlag.PRINT
-      ) {
         this.pendingCleanup = true;
       }
       this._tryCleanup();
@@ -1426,7 +1397,7 @@ class PDFPageProxy {
 
         this._abortOperatorList({
           intentState,
-          reason: error,
+          reason: error instanceof Error ? error : new Error(error),
         });
       } else {
         internalRenderTask.capability.resolve();
@@ -1452,7 +1423,7 @@ class PDFPageProxy {
       operatorList: intentState.operatorList,
       pageIndex: this._pageIndex,
       canvasFactory: canvasFactoryInstance,
-      useRequestAnimationFrame: !(renderingIntent & RenderingIntentFlag.PRINT),
+      useRequestAnimationFrame: !intentPrint,
       pdfBug: this._pdfBug,
     });
 
@@ -1497,11 +1468,13 @@ class PDFPageProxy {
       }
     }
 
-    const renderingIntent = getRenderingIntent(intent, { isOpList: true });
+    const intentArgs = this._transport.getRenderingIntent(intent, {
-    let intentState = this._intentStates.get(renderingIntent);
+      isOpList: true,
+    });
+    let intentState = this._intentStates.get(intentArgs.cacheKey);
     if (!intentState) {
       intentState = Object.create(null);
-      this._intentStates.set(renderingIntent, intentState);
+      this._intentStates.set(intentArgs.cacheKey, intentState);
     }
     let opListTask;
 
@@ -1519,10 +1492,7 @@ class PDFPageProxy {
       if (this._stats) {
         this._stats.time("Page Request");
       }
-      this._pumpOperatorList({
+      this._pumpOperatorList(intentArgs);
-        pageIndex: this._pageIndex,
-        intent: renderingIntent,
-      });
     }
     return intentState.opListReadCapability.promise;
   }
@@ -1605,14 +1575,14 @@ class PDFPageProxy {
     this._transport.pageCache[this._pageIndex] = null;
 
     const waitOn = [];
-    for (const [intent, intentState] of this._intentStates) {
+    for (const intentState of this._intentStates.values()) {
       this._abortOperatorList({
         intentState,
         reason: new Error("Page was destroyed."),
         force: true,
       });
 
-      if (intent & RenderingIntentFlag.OPLIST) {
+      if (intentState.opListReadCapability) {
         // Avoid errors below, since the renderTasks are just stubs.
         continue;
       }
@@ -1670,8 +1640,8 @@ class PDFPageProxy {
   /**
    * @private
    */
-  _startRenderPage(transparency, intent) {
+  _startRenderPage(transparency, cacheKey) {
-    const intentState = this._intentStates.get(intent);
+    const intentState = this._intentStates.get(cacheKey);
     if (!intentState) {
       return; // Rendering was cancelled.
     }
@@ -1709,19 +1679,32 @@ class PDFPageProxy {
   /**
    * @private
    */
-  _pumpOperatorList(args) {
+  _pumpOperatorList({ renderingIntent, cacheKey }) {
-    assert(
+    if (
-      args.intent,
+      typeof PDFJSDev === "undefined" ||
-      'PDFPageProxy._pumpOperatorList: Expected "intent" argument.'
+      PDFJSDev.test("!PRODUCTION || TESTING")
-    );
+    ) {
+      assert(
+        Number.isInteger(renderingIntent) && renderingIntent > 0,
+        '_pumpOperatorList: Expected valid "renderingIntent" argument.'
+      );
+    }
 
     const readableStream = this._transport.messageHandler.sendWithStream(
       "GetOperatorList",
-      args
+      {
+        pageIndex: this._pageIndex,
+        intent: renderingIntent,
+        cacheKey,
+        annotationStorage:
+          renderingIntent & RenderingIntentFlag.ANNOTATIONS_STORAGE
+            ? this._transport.annotationStorage.serializable
+            : null,
+      }
     );
     const reader = readableStream.getReader();
 
-    const intentState = this._intentStates.get(args.intent);
+    const intentState = this._intentStates.get(cacheKey);
     intentState.streamReader = reader;
 
     const pump = () => {
@@ -1770,11 +1753,15 @@ class PDFPageProxy {
    * @private
    */
   _abortOperatorList({ intentState, reason, force = false }) {
-    assert(
+    if (
-      reason instanceof Error ||
+      typeof PDFJSDev === "undefined" ||
-        (typeof reason === "object" && reason !== null),
+      PDFJSDev.test("!PRODUCTION || TESTING")
-      'PDFPageProxy._abortOperatorList: Expected "reason" argument.'
+    ) {
-    );
+      assert(
+        reason instanceof Error,
+        '_abortOperatorList: Expected valid "reason" argument.'
+      );
+    }
 
     if (!intentState.streamReader) {
       return;
@@ -1808,9 +1795,9 @@ class PDFPageProxy {
     }
     // Remove the current `intentState`, since a cancelled `getOperatorList`
     // call on the worker-thread cannot be re-started...
-    for (const [intent, curIntentState] of this._intentStates) {
+    for (const [curCacheKey, curIntentState] of this._intentStates) {
       if (curIntentState === intentState) {
-        this._intentStates.delete(intent);
+        this._intentStates.delete(curCacheKey);
         break;
       }
     }
@@ -2354,6 +2341,45 @@ class WorkerTransport {
     return shadow(this, "annotationStorage", new AnnotationStorage());
   }
 
+  getRenderingIntent(
+    intent,
+    { renderForms = false, includeAnnotationStorage = false, isOpList = false }
+  ) {
+    let renderingIntent = RenderingIntentFlag.DISPLAY; // Default value.
+    let lastModified = "";
+
+    switch (intent) {
+      case "any":
+        renderingIntent = RenderingIntentFlag.ANY;
+        break;
+      case "display":
+        break;
+      case "print":
+        renderingIntent = RenderingIntentFlag.PRINT;
+        break;
+      default:
+        warn(`getRenderingIntent - invalid intent: ${intent}`);
+    }
+
+    if (renderForms) {
+      renderingIntent += RenderingIntentFlag.ANNOTATIONS_FORMS;
+    }
+    if (includeAnnotationStorage) {
+      renderingIntent += RenderingIntentFlag.ANNOTATIONS_STORAGE;
+
+      lastModified = this.annotationStorage.lastModified;
+    }
+
+    if (isOpList) {
+      renderingIntent += RenderingIntentFlag.OPLIST;
+    }
+
+    return {
+      renderingIntent,
+      cacheKey: `${renderingIntent}_${lastModified}`,
+    };
+  }
+
   destroy() {
     if (this.destroyCapability) {
       return this.destroyCapability.promise;
@@ -2611,7 +2637,7 @@ class WorkerTransport {
       }
 
       const page = this.pageCache[data.pageIndex];
-      page._startRenderPage(data.transparency, data.intent);
+      page._startRenderPage(data.transparency, data.cacheKey);
     });
 
     messageHandler.on("commonobj", data => {

src/shared/util.js

@@ -18,11 +18,23 @@ import "./compatibility.js";
 const IDENTITY_MATRIX = [1, 0, 0, 1, 0, 0];
 const FONT_IDENTITY_MATRIX = [0.001, 0, 0, 0.001, 0, 0];
 
+/**
+ * Refer to the `WorkerTransport.getRenderingIntent`-method in the API, to see
+ * how these flags are being used:
+ *  - ANY, DISPLAY, and PRINT are the normal rendering intents, note the
+ *    `PDFPageProxy.{render, getOperatorList, getAnnotations}`-methods.
+ *  - ANNOTATIONS_FORMS, and ANNOTATIONS_STORAGE controls which annotations are
+ *    rendered onto the canvas, note the `renderInteractiveForms`- respectively
+ *    `includeAnnotationStorage`-options in the `PDFPageProxy.render`-method.
+ *  - OPLIST is used with the `PDFPageProxy.getOperatorList`-method, note the
+ *    `OperatorList`-constructor (on the worker-thread).
+ */
 const RenderingIntentFlag = {
   ANY: 0x01,
   DISPLAY: 0x02,
   PRINT: 0x04,
-  ANNOTATION_FORMS: 0x20,
+  ANNOTATIONS_FORMS: 0x10,
+  ANNOTATIONS_STORAGE: 0x20,
   OPLIST: 0x100,
 };