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

Ensure that PDFDocument.documentInfo doesn't fail during document load, when the entire XRef table hasn't been fetched yet (issue 8180) #8183

Merged
merged 1 commit into from
Mar 22, 2017

Conversation

Snuffleupagus
Copy link
Collaborator

Similar to other try-catch statements in /core code, we must re-throw MissingDataException to prevent issues with missing data during document loading.
Note that I'm not sure if/how we can test this, which is why the patch doesn't include any test(s).

Fixes #8180.

@Rob--W Thank you for helping with debugging this in #8180 (comment); do you have time to review the patch?

…oad, when the entire XRef table hasn't been fetched yet (issue 8180)

Similar to other `try-catch` statements in `/core` code, we must re-throw `MissingDataException` to prevent issues with missing data during document loading.
Note that I'm not sure if/how we can test this, which is why the patch doesn't include any test(s).

Fixes 8180.
@Rob--W
Copy link
Member

Rob--W commented Mar 22, 2017

That was quick, thanks for the patch!

To create a unit test for it, you could try to mock the network component and deliberately send partial data. Do you want to add the test, or should I merge the PR after manual verification?

@Snuffleupagus
Copy link
Collaborator Author

To create a unit test for it, you could try to mock the network component and deliberately send partial data. Do you want to add the test, or should I merge the PR after manual verification?

Unfortunately I won't have time to try and write a unit-test for this right now, so in the interest of getting this long-standing issue resolved it's probably better to just test manually for now (and we should at least run the unit-tests as well).

/botio unittest

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/89517c8637d8cf8/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/553f11a4354657c/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/89517c8637d8cf8/output.txt

Total script time: 2.40 mins

  • Unit Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://54.215.176.217:8877/553f11a4354657c/output.txt

Total script time: 5.78 mins

  • Unit Tests: Passed

Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified by applying this patch to the project from #8180 and confirming that the printed information is complete.

@Rob--W
Copy link
Member

Rob--W commented Mar 22, 2017

To create a unit test for it, you could try to mock the network component and deliberately send partial data. Do you want to add the test, or should I merge the PR after manual verification?

Unfortunately I won't have time to try and write a unit-test for this right now, so in the interest of getting this long-standing issue resolved it's probably better to just test manually for now (and we should at least run the unit-tests as well).

Fair enough, thanks for taking your time to write the patch in the first place.

@Rob--W Rob--W merged commit 086021b into mozilla:master Mar 22, 2017
@Snuffleupagus Snuffleupagus deleted the documentInfo-MissingDataException branch March 22, 2017 15:01
movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
…ngDataException

Ensure that `PDFDocument.documentInfo` doesn't fail during document load, when the entire XRef table hasn't been fetched yet (issue 8180)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants