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

Stop caching Streams in XRef.fetchCompressed #11370

Merged
merged 2 commits into from
Nov 30, 2019
Merged

Stop caching Streams in XRef.fetchCompressed #11370

merged 2 commits into from
Nov 30, 2019

Commits on Nov 30, 2019

  1. Slighthly re-factor XRef.fetchCompressed 

     - Change all occurences of `var` to `let`/`const`.

 - Initialize the (temporary) Arrays with the correct sizes upfront.

 - Inline the `isCmd` check. Obviously this won't make a huge difference, but given that the check is only relevant for corrupt documents it cannot hurt.
    @Snuffleupagus
    Snuffleupagus committed Nov 30, 2019
    Configuration menu
    Copy the full SHA
    06412a5 View commit details
    Browse the repository at this point in the history

  2. Stop caching Streams in XRef.fetchCompressed 

    I'm slightly surprised that this hasn't actually caused any (known) bugs, but that may be more luck than anything else since it fortunately doesn't seem common for Streams to be defined inside of an 'ObjStm'.[1]

Note that in the `XRef.fetchUncompressed` method we're *not* caching Streams, and that for very good reasons too.

 - Streams, especially the `DecodeStream` ones, can become *very* large once read. Hence caching them really isn't a good idea simply because of the (potential) memory impact of doing so.

 - Attempting to read from the *same* Stream more than once won't work, unless it's `reset` in between, since using any method such as e.g. `getBytes` always starts at the current data position.

 - Given that even the `src/core/` code is now fairly asynchronous, see e.g. the `PartialEvaluator`, it's generally impossible to assert that any one Stream isn't being accessed "concurrently" by e.g. different `getOperatorList` calls. Hence `reset`-ing a cached Streams isn't going to work in the general case.

All in all, I cannot understand why it'd ever be correct to cache Streams in the `XRef.fetchCompressed` method.

---
[1] One example where that happens is the `issue3115r.pdf` file in the test-suite, where the streams in question are not actually used for anything within the PDF.js code.
    @Snuffleupagus
    Snuffleupagus committed Nov 30, 2019
    Configuration menu
    Copy the full SHA
    168c6ae View commit details
    Browse the repository at this point in the history