Merge pull request #9975 from Snuffleupagus/getDestination-refactor

Re-factor `destinations`/`getDestination` to reduce unnecessary duplication, and reject non-string inputs
mozilla · Aug 12, 2018 · 1268aea · 1268aea
2 parents af19ed6 + 1179584
commit 1268aea
Show file tree

Hide file tree

Showing 3 changed files with 56 additions and 50 deletions.
diff --git a/src/core/obj.js b/src/core/obj.js
@@ -28,6 +28,10 @@ import { ChunkedStream } from './chunked_stream';
 import { CipherTransformFactory } from './crypto';
 import { ColorSpace } from './colorspace';
 
+function fetchDestination(dest) {
+  return isDict(dest) ? dest.get('D') : dest;
+}
+
 var Catalog = (function CatalogClosure() {
   function Catalog(pdfManager, xref) {
     this.pdfManager = pdfManager;
@@ -173,64 +177,37 @@ var Catalog = (function CatalogClosure() {
       // shadow the prototype getter
       return shadow(this, 'numPages', obj);
     },
-    get destinations() {
-      function fetchDestination(dest) {
-        return isDict(dest) ? dest.get('D') : dest;
-      }
-
-      var xref = this.xref;
-      var dests = {}, nameTreeRef, nameDictionaryRef;
-      var obj = this.catDict.get('Names');
-      if (obj && obj.has('Dests')) {
-        nameTreeRef = obj.getRaw('Dests');
-      } else if (this.catDict.has('Dests')) {
-        nameDictionaryRef = this.catDict.get('Dests');
-      }
 
-      if (nameDictionaryRef) {
-        // reading simple destination dictionary
-        obj = nameDictionaryRef;
-        obj.forEach(function catalogForEach(key, value) {
-          if (!value) {
-            return;
-          }
-          dests[key] = fetchDestination(value);
-        });
-      }
-      if (nameTreeRef) {
-        var nameTree = new NameTree(nameTreeRef, xref);
-        var names = nameTree.getAll();
-        for (var name in names) {
+    get destinations() {
+      const obj = this._readDests(), dests = Object.create(null);
+      if (obj instanceof NameTree) {
+        const names = obj.getAll();
+        for (let name in names) {
           dests[name] = fetchDestination(names[name]);
         }
+      } else if (obj instanceof Dict) {
+        obj.forEach(function(key, value) {
+          if (value) {
+            dests[key] = fetchDestination(value);
+          }
+        });
       }
       return shadow(this, 'destinations', dests);
     },
-    getDestination: function Catalog_getDestination(destinationId) {
-      function fetchDestination(dest) {
-        return isDict(dest) ? dest.get('D') : dest;
+    getDestination(destinationId) {
+      const obj = this._readDests();
+      if (obj instanceof NameTree || obj instanceof Dict) {
+        return fetchDestination(obj.get(destinationId) || null);
       }
-
-      var xref = this.xref;
-      var dest = null, nameTreeRef, nameDictionaryRef;
-      var obj = this.catDict.get('Names');
+      return null;
+    },
+    _readDests() {
+      const obj = this.catDict.get('Names');
       if (obj && obj.has('Dests')) {
-        nameTreeRef = obj.getRaw('Dests');
-      } else if (this.catDict.has('Dests')) {
-        nameDictionaryRef = this.catDict.get('Dests');
-      }
-
-      if (nameDictionaryRef) { // Simple destination dictionary.
-        var value = nameDictionaryRef.get(destinationId);
-        if (value) {
-          dest = fetchDestination(value);
-        }
-      }
-      if (nameTreeRef) {
-        var nameTree = new NameTree(nameTreeRef, xref);
-        dest = fetchDestination(nameTree.get(destinationId));
+        return new NameTree(obj.getRaw('Dests'), this.xref);
+      } else if (this.catDict.has('Dests')) { // Simple destination dictionary.
+        return this.catDict.get('Dests');
       }
-      return dest;
     },
 
     get pageLabels() {

diff --git a/src/display/api.js b/src/display/api.js
@@ -627,7 +627,7 @@ var PDFDocumentProxy = (function PDFDocumentProxyClosure() {
       return this.transport.getDestinations();
     },
     /**
-     * @param {string} id The named destination to get.
+     * @param {string} id - The named destination to get.
      * @return {Promise} A promise that is resolved with all information
      * of the given named destination.
      */
@@ -2119,6 +2119,9 @@ var WorkerTransport = (function WorkerTransportClosure() {
     },
 
     getDestination: function WorkerTransport_getDestination(id) {
+      if (typeof id !== 'string') {
+        return Promise.reject(new Error('Invalid destination request.'));
+      }
       return this.messageHandler.sendWithPromise('GetDestination', {
         id,
       });

diff --git a/test/unit/api_spec.js b/test/unit/api_spec.js
@@ -583,6 +583,32 @@ describe('api', function() {
       });
     });
 
+    it('gets non-string destination', function(done) {
+      let numberPromise = doc.getDestination(4.3);
+      let booleanPromise = doc.getDestination(true);
+      let arrayPromise = doc.getDestination([
+        { num: 17, gen: 0, }, { name: 'XYZ', }, 0, 841.89, null]);
+
+      numberPromise = numberPromise.then(function() {
+        throw new Error('shall fail for non-string destination.');
+      }, function(reason) {
+        expect(reason instanceof Error).toEqual(true);
+      });
+      booleanPromise = booleanPromise.then(function() {
+        throw new Error('shall fail for non-string destination.');
+      }, function(reason) {
+        expect(reason instanceof Error).toEqual(true);
+      });
+      arrayPromise = arrayPromise.then(function() {
+        throw new Error('shall fail for non-string destination.');
+      }, function(reason) {
+        expect(reason instanceof Error).toEqual(true);
+      });
+
+      Promise.all([numberPromise, booleanPromise, arrayPromise]).then(
+        done, done.fail);
+    });
+
     it('gets non-existent page labels', function (done) {
       var promise = doc.getPageLabels();
       promise.then(function (data) {