Provide a fallback for font rendering when not allowed to use `eval` #6141

skalnik · 2015-06-24T22:09:32Z

This provides a fallback for font rendering if PDF.js is used in a place where it is not allowed to use eval, such as an environment with a CSP header with unsafe-eval.

It does so by changing the code emitted from the font_renderer to be more generic commands, and then when they're run in the font_loader, they're compiled into JS and used to construct a function like before. However if we catch an EvalError, we instead use Function.prototype.apply to run the commands.

Fixes #4660

yurydelendik · 2015-06-24T22:49:58Z

Thanks for working on the issue. Could you address jshint issues and squash commits? Make sure you are friendly to the https://github.com/mozilla/pdf.js/wiki/Style-Guide.

Few points:

feature test ability to eval once and perform the transform for compiler or interpreter later
generate few less arrays to reduce amount of created object, e.g. use single array and add some smarts on the compiler/interpreter to scan it

skalnik · 2015-06-25T22:13:29Z

@yurydelendik Thanks for the kind words! Its my pleasure to work on this 😁

I cleaned up much of the excess array creation, ensured I passed the linter, and applied the style guide rules where I saw I violated them. I'd love another review on this when you get a chance 👀

I'm unsure how to test this feature. I'm not the most familiar with this code base and I'm having trouble finding any tests around this. I also am not sure if there is a nice way to force the Function constructor to throw an EvalError instead of executing. If you had any pointers here they would be greatly appreciated 🙇

After this is resolved, I'm happy to squash my commits to prepare this for merging ✨

timvandermeij · 2015-06-26T09:33:05Z

@skalnik It is easier for us to review your code if you squash the commits into one commit. Could you please do that? Thanks!

skalnik · 2015-06-26T15:26:00Z

@timvandermeij D'oh, I always just look at the PR diff on GitHub, so I didn't even think about that. I've gone ahead and squashed the commits 👍

yurydelendik · 2015-06-29T13:23:14Z

src/display/font_loader.js

+          }
+          /* jshint -W054 */
+          this.compiledGlyphs[character] = new Function('c', 'size', js);
+        } catch(e) {


Use lazy initialized property (e.g. isEvalSupported) at FontLoader that will use shadow() where you will testnew Function capabilities and branch based on that, e.g. var result; try { new Function(''); result = true; } catch (e) { result = false }; return shadow(....

See example at https://github.com/skalnik/pdf.js/blob/fix-font-csp-issues/src/display/font_loader.js#L54

yurydelendik · 2015-06-29T13:28:03Z

Reword commit comment "[PATCH] Add fallback for font loading when eval disabled
🔮" to "Add fallback for font loading when eval disabled"

yurydelendik · 2015-06-29T13:31:28Z

I'm unsure how to test this feature. I'm not the most familiar with this code base and I'm having trouble finding any tests around this.

If you introduce FontLoader.isEvalSupported property, you will be able to fake different compile methods by returning true or false. The internal font rendering is working when PDFJS.disableFontFace is set to true.

yurydelendik · 2015-06-30T15:48:44Z

Could you add isEvalSupported to the FontLoader?

  get isEvalSupported() {
    var result = false;
    try {
       new Function(''); // trying to compile a simple function
       result = true;
    } catch (e) {
      // eval is not supported
    }
   return shadow(this, 'isEvalSupported', result);
  },

And use it to branch the logic in the FontLoader.getPathGenerator. PDFJS.isEvalSupported check can be included in the function above as well.

skalnik · 2015-06-30T18:33:24Z

Ah, after looking at your example, I think I understand what you were saying now. Sorry, a lot of these JS patterns are new to me

timvandermeij · 2015-06-30T22:02:18Z

There are some lint errors. Tip: you can catch lint errors locally by running node make lint from a terminal. Please also keep the commits squashed for review. Thanks!

Rob--W · 2015-07-10T12:22:48Z

Could you optimize your code, by using objects instead of array & slice? Your current method involves a array copy operation at every glyph creation call. Here are some suggestions for optimization:

Use an object, not an array

I have created some alternatives and a benchmark, my proposed method ("usingObject") is 2-4x faster than your method (Safari 8. Chrome 43, Firefox 40,

  cmds.push({ name: 'lineTo',  args: [x, y]});  // usingObject
// instead of cmds.push(['lineTo', x, y]);      // usingSlice
// or even cmds.push(['lineTo', [x, y]]);       // usingArray

For a benchmark of the alternatives, see http://jsfiddle.net/5co62ohk/1/. My top suggestion is 3-4x faster than my proposed alternative in Chrome 43, Safari 8 and IE 11 and 1.3-1.5x faster in Firefox 40.

Special-case parameter-less arguments

The generic for loop in if (cmd === 'scale') is not needed, because scale always uses the same constant parameters. Instead of the for loop in this.compiledGlyphs[character], use the following. You could also put a comment at cmds.push({name: 'scale', args: []}); to clarify that scale is treated as a special case.

if (cmd === 'scale') {
    args = [size, -size];
}

skalnik · 2015-07-28T23:48:52Z

Could you optimize your code, by using objects instead of array & slice? Your current method involves a array copy operation at every glyph creation call. Here are some suggestions for optimization:

I've implemented these changes 😁

Oh wait! I lied, I didn't do the second one. Doing that now.

skalnik · 2015-08-07T16:50:05Z

@Rob--W Any more thoughts on this?

Rob--W · 2015-08-07T17:02:05Z

@skalnik Two small notes, and then it looks fine to me.

skalnik · 2015-08-10T21:27:36Z

All fixed up 🔧

Rob--W · 2015-08-10T22:17:19Z

src/display/font_loader.js

-        this.compiledGlyphs[character] = new Function('c', 'size', js);
+        var cmds = objs.get(this.loadedName + '_path_' + character);
+        var js = '';
+        var current, i, len, j, alen;


j and alen don't seem to be used, so remove them.

And could you move var js = ''; inside if (FontLoader.isEvalSupported) {?

Rob--W · 2015-08-10T22:18:34Z

@skalnik I found another two minor issues. I guess that that's really it.

skalnik · 2015-08-12T16:47:54Z

Patched that up as well!

Rob--W · 2015-08-12T17:07:12Z

In terms of code, LGTM.

I'll defer the merge to @yurydelendik.

yurydelendik · 2015-08-12T18:08:59Z

src/display/font_loader.js

+    if (PDFJS.isEvalSupported) {
+      try {
+        /* jshint evil: true */
+        new Function('console.log("test");');


Make function empty.

yurydelendik · 2015-08-12T18:10:31Z

Looks good with comments above addressed.

skalnik · 2015-08-12T21:14:46Z

Updated!

timvandermeij · 2015-08-12T21:39:33Z

The bots time out when processing test/pdfs/glyph_accent.pdf with the following console message:

TypeError: current.args is undefined

Snuffleupagus · 2015-08-13T07:45:57Z

src/display/font_loader.js

+          for (i = 0, len = cmds.length; i < len; i++) {
+            current = cmds[i];
+
+            if(current.args !== undefined) {


Nit: please add one space between if and (.

In some cases, such as in use with a CSP header, constructing a function with a string of javascript is not allowed. However, compiling the various commands that need to be done on the canvas element is faster than interpreting them. This patch changes the font renderer to instead emit commands that are compiled by the font loader. If, during compilation, we receive an EvalError, we instead interpret them.

skalnik · 2015-08-13T21:33:54Z

@timvandermeij That should be resolved. @Snuffleupagus tiny nits fixed up

timvandermeij · 2015-08-13T21:39:32Z

/botio-linux preview

pdfjsbot · 2015-08-13T21:39:33Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/a50cdef429e0d69/output.txt

pdfjsbot · 2015-08-13T21:40:12Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/a50cdef429e0d69/output.txt

Total script time: 0.64 mins

Published

timvandermeij · 2015-08-13T21:40:17Z

/botio test

pdfjsbot · 2015-08-13T21:40:18Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/f72d3db853ecabb/output.txt

pdfjsbot · 2015-08-13T21:40:18Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/539e862e2ff05cb/output.txt

pdfjsbot · 2015-08-13T21:58:46Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/539e862e2ff05cb/output.txt

Total script time: 18.47 mins

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

pdfjsbot · 2015-08-13T21:59:19Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/f72d3db853ecabb/output.txt

Total script time: 19.02 mins

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

timvandermeij · 2015-08-13T22:43:53Z

This patch looks really good and much cleaner than before to me. The tests also pass now. @yurydelendik would like to test this once more and after that this should be good to merge.

skalnik · 2015-08-18T20:06:59Z

@yurydelendik any updates here since you wanted to test one more time?

yurydelendik · 2015-08-18T23:50:09Z

Looks good. Thank you for the patch.

Provide a fallback for font rendering when not allowed to use `eval`

timvandermeij added the core label Jun 25, 2015

skalnik force-pushed the fix-font-csp-issues branch from 6681cea to 2f7dd63 Compare June 26, 2015 15:23

yurydelendik reviewed Jun 29, 2015
View reviewed changes

skalnik force-pushed the fix-font-csp-issues branch from e596313 to 3442ecd Compare July 1, 2015 00:07

timvandermeij assigned yurydelendik Jul 5, 2015

skalnik force-pushed the fix-font-csp-issues branch from 11aed83 to 8f044d6 Compare July 28, 2015 23:48

skalnik force-pushed the fix-font-csp-issues branch 2 times, most recently from ac13648 to bf4544f Compare July 29, 2015 00:12

skalnik force-pushed the fix-font-csp-issues branch from bf4544f to ae25b9c Compare August 7, 2015 20:10

Rob--W reviewed Aug 10, 2015
View reviewed changes

skalnik force-pushed the fix-font-csp-issues branch 2 times, most recently from 81c395d to 88ba476 Compare August 12, 2015 16:24

yurydelendik reviewed Aug 12, 2015
View reviewed changes

skalnik force-pushed the fix-font-csp-issues branch from 88ba476 to 04cbddc Compare August 12, 2015 21:10

skalnik force-pushed the fix-font-csp-issues branch from 04cbddc to 8daceca Compare August 12, 2015 23:56

Snuffleupagus reviewed Aug 13, 2015
View reviewed changes

skalnik force-pushed the fix-font-csp-issues branch from 8daceca to 341c5e9 Compare August 13, 2015 21:33

yurydelendik added a commit that referenced this pull request Aug 18, 2015

Merge pull request #6141 from skalnik/fix-font-csp-issues

c56dc9a

Provide a fallback for font rendering when not allowed to use `eval`

yurydelendik merged commit c56dc9a into mozilla:master Aug 18, 2015

skalnik deleted the fix-font-csp-issues branch July 26, 2016 19:01

Provide a fallback for font rendering when not allowed to use eval #6141

Provide a fallback for font rendering when not allowed to use eval #6141

Conversation

skalnik commented Jun 24, 2015

yurydelendik commented Jun 24, 2015

skalnik commented Jun 25, 2015

timvandermeij commented Jun 26, 2015

skalnik commented Jun 26, 2015

yurydelendik Jun 29, 2015

Choose a reason for hiding this comment

yurydelendik commented Jun 29, 2015

yurydelendik commented Jun 29, 2015

yurydelendik commented Jun 30, 2015

skalnik commented Jun 30, 2015

timvandermeij commented Jun 30, 2015

Rob--W commented Jul 10, 2015

Use an object, not an array

Special-case parameter-less arguments

skalnik commented Jul 28, 2015

skalnik commented Aug 7, 2015

Rob--W commented Aug 7, 2015

skalnik commented Aug 10, 2015

Rob--W Aug 10, 2015

Choose a reason for hiding this comment

Rob--W commented Aug 10, 2015

skalnik commented Aug 12, 2015

Rob--W commented Aug 12, 2015

yurydelendik Aug 12, 2015

Choose a reason for hiding this comment

yurydelendik commented Aug 12, 2015

skalnik commented Aug 12, 2015

timvandermeij commented Aug 12, 2015

Snuffleupagus Aug 13, 2015

Choose a reason for hiding this comment

skalnik commented Aug 13, 2015

timvandermeij commented Aug 13, 2015

pdfjsbot commented Aug 13, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Aug 13, 2015

From: Bot.io (Linux)

Success

Published

timvandermeij commented Aug 13, 2015

pdfjsbot commented Aug 13, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Aug 13, 2015

From: Bot.io (Windows)

Received

pdfjsbot commented Aug 13, 2015

From: Bot.io (Windows)

Success

pdfjsbot commented Aug 13, 2015

From: Bot.io (Linux)

Success

timvandermeij commented Aug 13, 2015

skalnik commented Aug 18, 2015

yurydelendik commented Aug 18, 2015

Provide a fallback for font rendering when not allowed to use `eval` #6141

Provide a fallback for font rendering when not allowed to use `eval` #6141