Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Font renderer logic violates the Content security policy (no unsafe-eval) #4660

Closed
Rob--W opened this issue Apr 22, 2014 · 9 comments · Fixed by #4738
Closed

Font renderer logic violates the Content security policy (no unsafe-eval) #4660

Rob--W opened this issue Apr 22, 2014 · 9 comments · Fixed by #4738

Comments

@Rob--W
Copy link
Member

Rob--W commented Apr 22, 2014

The font renderer uses a form of eval for rendering fonts:
display/font_loader.js: this.compiledGlyphs[character] = new Function('c', 'size', js);

This does not play well with the Chrome extension. One way to solve the problem is to add unsafe-eval to the CSP of the Chromium extension.
Another way is to get rid of eval. It seems that the code string follows a strict format;

core/font_renderer.js:      js.push('c.moveTo(' + x + ',' + y + ');');
core/font_renderer.js:      js.push('c.lineTo(' + x + ',' + y + ');');
core/font_renderer.js:      js.push('c.quadraticCurveTo(' + xa + ',' + ya + ',' +
core/font_renderer.js:          js.push('c.save();');
core/font_renderer.js:          js.push('c.transform(' + scaleX + ',' + scale01 + ',' +
core/font_renderer.js:          js.push('c.restore();');
core/font_renderer.js:      js.push('c.moveTo(' + x + ',' + y + ');');
core/font_renderer.js:      js.push('c.lineTo(' + x + ',' + y + ');');
core/font_renderer.js:      js.push('c.bezierCurveTo(' + x1 + ',' + y1 + ',' + x2 + ',' + y2 + ',' +
core/font_renderer.js:              js.push('c.save();');
core/font_renderer.js:              js.push('c.translate('+ x + ',' + y + ');');
core/font_renderer.js:              js.push('c.restore();');
core/font_renderer.js:      js.push('c.save();');
core/font_renderer.js:      js.push('c.transform(' + this.fontMatrix.join(',') + ');');
core/font_renderer.js:      js.push('c.scale(size, -size);');
core/font_renderer.js:      js.push('c.restore();');

Instead of generating a string of code, we could create a list of commands.
This could be implemented as a array of commands, names and arguments (at generation in the worker) and a loop (in the viewer).

@yurydelendik Do you see any issues with the proposed solution?

(Here is a PDF to test, http://www.adambarth.com/papers/2008/barth-jackson-mitchell.pdf#page=11)

Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'self' chrome-extension-resource:".

@yurydelendik
Copy link
Contributor

Maybe, can we look at the effect on the performance? We choose this method since apply() with array of arguments was slower. I'm okay with the proposed solution if it will not affect the Firefox and Chrome's performance.

@Rob--W
Copy link
Member Author

Rob--W commented May 4, 2014

.apply has a terrible performance, but it can be sped up by x3 by using the fact that the number of arguments is limited (e.g. say, max 8), and use switch-case.

The test is posted at http://jsperf.com/pdf-js-function-or-compile; I'm adding 1000 commands, then run the compiled function 1000x. Do you think that the test case is an accurate representation of PDF.js?

The test results are surprising.

  • On Firefox, my proposed method is 4-5x faster than the current method.
  • On Chrome, my method is 3x slower than the current method.

@yurydelendik
Copy link
Contributor

You did not try per command type interpretation, similar thing I'm trying to do at https://github.com/mozilla/pdf.js/pull/4683/files#diff-1ae2243ab84e246d9a06a6c22b5e5ad1R966 -- I think, this shall allow JIT to bind to native canvas function (vs generic apply call) and don't create extra arrays during slice.

@waddlesplash
Copy link
Contributor

Still, why is Chrome slower? Is this a bug in V8's optimizer?

@Rob--W
Copy link
Member Author

Rob--W commented May 7, 2014

@waddlesplash There is no bug in V8's optimizer, I guess. On the contrary, the eval method (generate a function once from a string and cache the result) is highly optimized (it's 2x faster than Firefox). The alternative methods, on the other hand, are a bit slower. Together, the result is amplified and gives the (misleading?) appearance that Chrome "is slower".

Note: An optimization technique for Firefox is not necessarily 1:1 appicable to Chrome and vice versa. I think that this area has not been explored in PDF.js before (mainly because it's a Mozilla project), but it might be interesting to consider: Could changing the code in hotspots lead to significant performance improvements in V8/Blink-powered browsers such as Chrome and Opera?

@waddlesplash
Copy link
Contributor

Yes, I see that V8 handles eval better than SpiderMonkey; but why is ArrayImpl 11 ops/sec on Firefox but 2 ops/sec on Chrome?

Or am I missing something here?

@Rob--W
Copy link
Member Author

Rob--W commented May 7, 2014

@waddlesplash I'm not an expert with V8-internals, so I don't know. You could create the assembly code using the v8 shell and inspect the result if you're interested.

@yurydelendik
Copy link
Contributor

An optimization technique for Firefox is not necessarily 1:1 appicable to Chrome and vice versa. I think that this area has not been explored in PDF.js before (mainly because it's a Mozilla project)

Just to be clear on "Mozilla project" statement: during initial development of this project we tried not use any of SpiderMonkey optimization techniques -- only intuitive techniques that could make faster any JavaScript JIT. (See conversation from #jsapi channel http://logs.glob.uno/?c=mozilla%23jsapi&s=29+Jan+2013&e=29+Jan+2013&h=pdf.js#c110308)

@timvandermeij
Copy link
Contributor

#4738 provided an initial fix, but we need to find a better solution for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants