Skip to content

Ensure that getDocument handles Node.js Buffers more gracefully (issue 13075) #13079

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
Merged
merged 1 commit into from
Mar 13, 2021
Merged

Ensure that getDocument handles Node.js Buffers more gracefully (issue 13075) #13079

merged 1 commit into from
Mar 13, 2021

Conversation

Snuffleupagus
Copy link
Collaborator

Please refer to the individual commit messages for detailed information.

Fixes #13075

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/73dfba02d036831/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

Live output at: http://3.101.106.178:8877/933e3bb32080b0e/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/73dfba02d036831/output.txt

Total script time: 3.56 mins

  • Unit Tests: FAILED

@pdfjsbot
Copy link

From: Bot.io (Windows)

Success

Full output at http://3.101.106.178:8877/933e3bb32080b0e/output.txt

Total script time: 5.60 mins

  • Unit Tests: Passed

@Snuffleupagus Snuffleupagus marked this pull request as draft March 12, 2021 19:57
@Snuffleupagus Snuffleupagus force-pushed the issue-13075 branch 3 times, most recently from 525b460 to a6e572c Compare March 12, 2021 20:36
@Snuffleupagus Snuffleupagus changed the title Ensure that the LoopbackPort actually copies TypedArrays as intended, and handle Node.js Buffers more gracefully in getDocument (issue 13075) Prevent issues, on the worker-thread, when calling getDocument with TypedArrays that have a non-zero byteOffset (issue 13075) Mar 12, 2021
@Snuffleupagus Snuffleupagus marked this pull request as ready for review March 12, 2021 20:37
@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/8e4a46c3b2eff5d/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

Live output at: http://3.101.106.178:8877/ac60089a023ffda/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/8e4a46c3b2eff5d/output.txt

Total script time: 3.65 mins

  • Unit Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)

Success

Full output at http://3.101.106.178:8877/ac60089a023ffda/output.txt

Total script time: 6.73 mins

  • Unit Tests: Passed

@Snuffleupagus
Ensure that getDocument handles Node.js Buffers more gracefully (…
50681d7 
…issue 13075)

While the JSDocs have never advertised `getDocument` as supporting Node.js `Buffer`s, that apparently doesn't stop users from passing such data structures to `getDocument`.
In theory the existing `instanceof Uint8Array` check ought to have caught Node.js `Buffer`s, however for reasons that I don't even pretend to understand that check actually passes. Hence this patch which, *only* in Node.js environments, will special-case `Buffer`s to hopefully provide a slightly better out-of-the-box behaviour in Node.js environments[1].

---
[1] Although I'm not sure that we necessarily want to advertise this in the JSDocs, given the specialized use-case.
@Snuffleupagus Snuffleupagus changed the title Prevent issues, on the worker-thread, when calling getDocument with TypedArrays that have a non-zero byteOffset (issue 13075) Ensure that getDocument handles Node.js Buffers more gracefully (issue 13075) Mar 13, 2021
@timvandermeij
Copy link
Contributor

/botio unittest

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/078a58bb73762e0/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

Live output at: http://3.101.106.178:8877/27d8cc9a49ac3c6/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/078a58bb73762e0/output.txt

Total script time: 3.55 mins

  • Unit Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)

Success

Full output at http://3.101.106.178:8877/27d8cc9a49ac3c6/output.txt

Total script time: 5.66 mins

  • Unit Tests: Passed

@timvandermeij timvandermeij merged commit ba56732 into mozilla:master Mar 13, 2021
@timvandermeij
Copy link
Contributor

Thank you!

@timvandermeij timvandermeij mentioned this pull request Mar 13, 2021
Closed
@Snuffleupagus Snuffleupagus deleted the issue-13075 branch March 13, 2021 13:38
@Snuffleupagus Snuffleupagus mentioned this pull request Jul 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timvandermeij timvandermeij timvandermeij approved these changes

Assignees
No one assigned
Labels
core node-specific
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

pdfjs fails to read a Buffer.from or buffer from fs.readFileSync
3 participants
@Snuffleupagus @pdfjsbot @timvandermeij