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.

Sign up for GitHub

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

Jump to bottom

Slightly refactor the fontRef handling in PartialEvaluator_loadFont (issue 7403 and issue 7402) #7347

Merged
merged 3 commits into from
Jul 23, 2016
Merged

Slightly refactor the fontRef handling in PartialEvaluator_loadFont (issue 7403 and issue 7402) #7347

merged 3 commits into from
Jul 23, 2016

Commits on Jul 21, 2016

  1. Slightly refactor the fontRef handling in `PartialEvaluator_loadFon… 

    …t` (issue 7403 and issue 7402)

Originally, I was just going to change this code to use `Ref_toString` in a couple more places. When I started reading the code, I figured that it wouldn't hurt to clean up a couple of comments. While doing this, I noticed that the logic for the (rare) `isDict(fontRef)` case could do with a few improvements.

There should be no functional changes with this patch, but given the added reference checks, we will now avoid bogus `Ref`s when resolving font aliases. In practice, as issue 7403 shows, the current code can break certain PDF files even if it's very rare.

Note that the only thing that this patch will change, is the `font.loadedName` in the case where a `fontRef` is a reference *and* the font doesn't have a descriptor. Previously for `fontRef = Ref(4, 0)` we'd get `font.loadedName = 'g_d0_f4_0'`, and with this patch `font.loadedName = g_d0_f4R`, which is actually one character shorted in most cases. (Given that `Ref_toString` contains an optimization for the `gen === 0` case, which is by far the most common `gen` value.)

In the already existing fallback case, where the `fontName` is used to when creating the `font.loadedName`, we allow any alphanumeric character. Hence I don't see how (as mentioned above) e.g. `font.loadedName = g_d0_f4R` would be an issue here.
    @Snuffleupagus
    Snuffleupagus committed Jul 21, 2016
    Configuration menu
    Copy the full SHA
    2e9cd3e View commit details
    Browse the repository at this point in the history

  2. Add a reduced test-case for issue 7403

    @Snuffleupagus
    Snuffleupagus committed Jul 21, 2016
    Configuration menu
    Copy the full SHA
    4fe891c View commit details
    Browse the repository at this point in the history

  3. Attempt to cache fonts that are direct objects (i.e. Dicts), as opp… 

    …osed to `Ref`s, to prevent re-rendering after `cleanup` from breaking (issue 7403 and issue 7402)

Fonts that are not referenced by `Ref`s are very uncommon in practice, but it can unfortunately happen. In this case, we're currently not caching them in the usual way, i.e. by `Ref`, which leads to failures when a page is rendered after `cleanup` has run.
The simplest solution would have been to remove the `font.translated` workaround, but since this would have meant loading these kind of fonts over and over, the patch attempts to be a bit clever about this situation.

Note that if we instead loaded fonts per *page*, instead of per document, this issue wouldn't have existed.
    @Snuffleupagus
    Snuffleupagus committed Jul 21, 2016
    Configuration menu
    Copy the full SHA
    390c02a View commit details
    Browse the repository at this point in the history