Implement the `AESBaseCipher` class and let the `AES128Cipher` and `AES256Cipher` classes extend it #9412

timvandermeij · 2018-01-27T22:08:25Z

More information is provided in the commit messages. Unit tests are already available and still pass. I highly recommend to review this per commit instead of looking at the full diff and reviewing the second commit with the ?w=1 flag.

The diff of the two classes after the first commit can be found at https://www.diffchecker.com/fBgVIwRm. This clearly shows the amount of code duplication and that basically everything can live in the base class except for _expandKey. The decryptBlock differences are resolved by taking the one for AES-256 in the base class and setting the iv parameter to null by default so that for AES-128 nothing changes.

Fixes #5155.

timvandermeij · 2018-01-27T22:19:30Z

/botio-linux preview

pdfjsbot · 2018-01-27T22:19:31Z

From: Bot.io (Linux m4)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/c3b9d9ea39fc88c/output.txt

pdfjsbot · 2018-01-27T22:27:52Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/c3b9d9ea39fc88c/output.txt

Total script time: 8.34 mins

Published

timvandermeij · 2018-01-27T22:28:05Z

/botio test

pdfjsbot · 2018-01-27T22:28:06Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/1421af42bbce4f2/output.txt

pdfjsbot · 2018-01-27T22:28:06Z

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.215.176.217:8877/329cd466fec145f/output.txt

pdfjsbot · 2018-01-27T22:52:03Z

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/329cd466fec145f/output.txt

Total script time: 23.94 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/329cd466fec145f/reftest-analyzer.html#web=eq.log

pdfjsbot · 2018-01-27T23:07:16Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/1421af42bbce4f2/output.txt

Total script time: 39.16 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

Snuffleupagus

Thanks for taking this on, since there's a lot of duplication removed here!

Given that we've thus far avoided using ES6 classes in src/core/ code, since the performance might still not be on-par with the old way of simulating classes, would it be possible to find a PDF file (e.g. in the test-suite) which make use of either of these algorithms and benchmark the impact of this PR?

Edit: One thing that's probably missing from those instructions, and which I forgot myself once, is that you need to remember to manually re-run gulp generic before each benchmarking step (since we're now relying on a built pdf.worker.js file when running tests).

I realize that it may perhaps be difficult to find such a file, but I figured that it cannot hurt to ask.

Snuffleupagus · 2018-02-01T09:50:12Z

src/core/crypto.js

-        result[j] = (t4 ^= result[j - 16]);
-        j++;
+class AESBaseCipher {
+  constructor(key) {


I don't believe that you're using the key parameter in the base class, so let's remove it both here and at the super() callsites.
Furthermore, since this should only ever be used as an abstract base class, let's actually enforce that:

constructor() { if (this.constructor === AESBaseCipher) { unreachable('Cannot initialize AESBaseCipher.'); } // The rest of the code goes here...

Snuffleupagus · 2018-02-01T09:57:45Z

src/core/crypto.js

+  }
+
+  _expandKey(cipherKey) {
+    throw new Error('Cannot call `_expandKey` on the base class');


Let's use unreachable instead here

Snuffleupagus · 2018-02-01T09:58:32Z

src/core/crypto.js

+  decryptBlock(data, finalize, iv = null) {
+    let i, sourceLength = data.length;
+    let buffer = this.buffer, bufferLength = this.bufferPosition;
+    // if not supplied an IV wait for IV values


While you're here, mind fixing the formatting/grammar of this comment?

Snuffleupagus · 2018-02-01T10:01:15Z

src/core/crypto.js

      for (i = 0; bufferLength < 16 && i < sourceLength; ++i, ++bufferLength) {
        buffer[bufferLength] = data[i];
      }
-      if (bufferLength < 16) {
-        // need more data
+      if (bufferLength < 16) { // Need more data.
        this.bufferLength = bufferLength;
        return new Uint8Array([]);


Pre-existing nit: This looks weird, let's use new Uint8Array(0); here and elsewhere instead.

Snuffleupagus · 2018-02-01T10:06:07Z

src/core/crypto.js

-  function _decrypt(input, key) {
-    var state = new Uint8Array(16);
+  _decrypt(input, key) {
+    let t, u, v, i, j, k;


Nit: Since we're now using let rather than var, thanks for that by the way :-), it doesn't seem entirely necessary to define the loop variables (i.e. i, j, k, and so on) in this "method/function global" way any more!?

This probably applies elsewhere too...

…re similar This commit is the first step for extracting a base class for the `AES128Cipher` and the `AES256Cipher` classes. The objective here is to make code changes (not altering the logic) to make the implementations as similar as possible as found by creating a diff of both classes. In particular, we extract the key size and cycles of repetitions constants since they are different for AES-128 and AES-256. Moreover, we rename functions to be similar. In the `AES256Cipher` class, there was an additional assignment to `this` in the decryption function. However, this was unnecessary because the assignment would also be done when the loop was exited.

…ES256Cipher` classes extend it

timvandermeij · 2018-02-03T19:24:25Z

Thank you for the review! I have made all changes and I did a benchmark of the code.

I created the following benchmark.json file with two files from the test suite that use the AES-128 cipher (found using grep -r "AESV2" test/* and confirmed by placing print statements in the class and opening the files in the viewer):

[
  {
    "id": "i9",
    "file": "pdfs/i9.pdf",
    "md5": "ba7cd54fdff083bb389295bc0415f6c5",
    "rounds": 20,
    "type": "load"
  },
  {
    "id": "ichiji.pdf",
    "file": "pdfs/ichiji.pdf",
    "md5": "66b645802d33513cd598886e017392b8",
    "rounds": 20,
    "type": "load"
  }
]

Then I ran all benchmarking steps from https://github.com/mozilla/pdf.js/wiki/Benchmarking-your-changes, which I updated to be more step-by-step and to include the gulp generic step, which was a really good point from you. Doing so I obtained the following table:

-- Grouped By browser, stat --
browser | stat         | Count | Baseline(ms) | Current(ms) | +/- |    %  | Result(P<.05)
------- | ------------ | ----- | ------------ | ----------- | --- | ----- | -------------
Firefox | Overall      |   120 |           55 |          54 |  -1 | -1.69 |              
Firefox | Page Request |   120 |            3 |           3 |   0 | -4.26 |              
Firefox | Rendering    |   120 |           52 |          52 |  -1 | -1.59 |

Therefore, we can conclude that this change has no negative performance impact.

Snuffleupagus · 2018-02-03T20:24:30Z

/botio test

pdfjsbot · 2018-02-03T20:24:30Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/14515caa1b4d8aa/output.txt

pdfjsbot · 2018-02-03T20:24:30Z

From: Bot.io (Windows)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.215.176.217:8877/f1af0452c155fef/output.txt

pdfjsbot · 2018-02-03T20:48:19Z

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/f1af0452c155fef/output.txt

Total script time: 23.81 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/f1af0452c155fef/reftest-analyzer.html#web=eq.log

pdfjsbot · 2018-02-03T21:03:13Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/14515caa1b4d8aa/output.txt

Total script time: 38.71 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

Snuffleupagus

Looks good, thanks for cleaning this up!

Also, thanks's for taking the time to benchmark the changes.

Implement the `AESBaseCipher` class and let the `AES128Cipher` and `AES256Cipher` classes extend it

timvandermeij added the core label Jan 27, 2018

Snuffleupagus self-requested a review January 28, 2018 20:39

Snuffleupagus reviewed Feb 1, 2018

View reviewed changes

timvandermeij added 2 commits February 3, 2018 20:16

Implement the AESBaseCipher class and let the AES128Cipher and `A…

73436c0

…ES256Cipher` classes extend it

timvandermeij force-pushed the crypto branch from 3d2b082 to 73436c0 Compare February 3, 2018 19:24

Snuffleupagus approved these changes Feb 4, 2018

View reviewed changes

Snuffleupagus merged commit d42541d into mozilla:master Feb 4, 2018

timvandermeij deleted the crypto branch February 4, 2018 14:57

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#9412 from timvandermeij/crypto

5ce49db

Implement the `AESBaseCipher` class and let the `AES128Cipher` and `AES256Cipher` classes extend it

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the `AESBaseCipher` class and let the `AES128Cipher` and `AES256Cipher` classes extend it #9412

Implement the `AESBaseCipher` class and let the `AES128Cipher` and `AES256Cipher` classes extend it #9412

timvandermeij commented Jan 27, 2018 •

edited

Loading

timvandermeij commented Jan 27, 2018

pdfjsbot commented Jan 27, 2018

pdfjsbot commented Jan 27, 2018

timvandermeij commented Jan 27, 2018

pdfjsbot commented Jan 27, 2018

pdfjsbot commented Jan 27, 2018

pdfjsbot commented Jan 27, 2018

pdfjsbot commented Jan 27, 2018

Snuffleupagus left a comment •

edited

Loading

Snuffleupagus Feb 1, 2018 •

edited

Loading

Snuffleupagus Feb 1, 2018

Snuffleupagus Feb 1, 2018

Snuffleupagus Feb 1, 2018

Snuffleupagus Feb 1, 2018 •

edited

Loading

timvandermeij commented Feb 3, 2018

Snuffleupagus commented Feb 3, 2018

pdfjsbot commented Feb 3, 2018

pdfjsbot commented Feb 3, 2018

pdfjsbot commented Feb 3, 2018

pdfjsbot commented Feb 3, 2018

Snuffleupagus left a comment •

edited

Loading

Implement the AESBaseCipher class and let the AES128Cipher and AES256Cipher classes extend it #9412

Implement the AESBaseCipher class and let the AES128Cipher and AES256Cipher classes extend it #9412

Conversation

timvandermeij commented Jan 27, 2018 • edited Loading

timvandermeij commented Jan 27, 2018

pdfjsbot commented Jan 27, 2018

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jan 27, 2018

From: Bot.io (Linux m4)

Success

Published

timvandermeij commented Jan 27, 2018

pdfjsbot commented Jan 27, 2018

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jan 27, 2018

From: Bot.io (Windows)

Received

pdfjsbot commented Jan 27, 2018

From: Bot.io (Windows)

Failed

pdfjsbot commented Jan 27, 2018

From: Bot.io (Linux m4)

Success

Snuffleupagus left a comment • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Feb 1, 2018

Choose a reason for hiding this comment

Snuffleupagus Feb 1, 2018

Choose a reason for hiding this comment

Snuffleupagus Feb 1, 2018

Choose a reason for hiding this comment

Snuffleupagus Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

timvandermeij commented Feb 3, 2018

Snuffleupagus commented Feb 3, 2018

pdfjsbot commented Feb 3, 2018

From: Bot.io (Linux m4)

Received

pdfjsbot commented Feb 3, 2018

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 3, 2018

From: Bot.io (Windows)

Failed

pdfjsbot commented Feb 3, 2018

From: Bot.io (Linux m4)

Success

Snuffleupagus left a comment • edited Loading

Choose a reason for hiding this comment

Implement the `AESBaseCipher` class and let the `AES128Cipher` and `AES256Cipher` classes extend it #9412

Implement the `AESBaseCipher` class and let the `AES128Cipher` and `AES256Cipher` classes extend it #9412

timvandermeij commented Jan 27, 2018 •

edited

Loading

Snuffleupagus left a comment •

edited

Loading

Snuffleupagus Feb 1, 2018 •

edited

Loading

Snuffleupagus Feb 1, 2018 •

edited

Loading

Snuffleupagus left a comment •

edited

Loading