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

[api-minor] Introduce a PrintAnnotationStorage with *frozen* serializable data #15043

Merged
merged 1 commit into from
Jun 24, 2022
Merged

[api-minor] Introduce a PrintAnnotationStorage with *frozen* serializable data #15043

merged 1 commit into from
Jun 24, 2022

Conversation

Snuffleupagus
Copy link
Collaborator

Given that printing is triggered synchronously in browsers, it's thus possible for scripting (in PDF documents) to modify the Annotation-data while printing is currently ongoing.
To work-around that we add a new printing-specific AnnotationStorage, where the serializable data is frozen upon initialization, which the viewer can thus create/utilize during printing.

@Snuffleupagus Snuffleupagus force-pushed the PrintAnnotationStorage branch 2 times, most recently from f4664ff to 94617b1 Compare June 15, 2022 09:46
@Snuffleupagus
Copy link
Collaborator Author

@calixteman This is my suggestion for how we could handle stored Annotation-data during printing; for PR #15032.

The idea here is that we don't (in any way) modify or freeze the regular AnnotationStorage-instance, since that feels error-prone, but rather create a new printing-specific AnnotationStorage where the serializable data cannot change.

Obviously this requires extending the API a little bit, but with the added checks (of intent, annotationMode, and instanceof PrintAnnotationStorage) it shouldn't really be possible for users to "abuse" this in unintended ways during e.g. normal rendering.

@Snuffleupagus Snuffleupagus force-pushed the PrintAnnotationStorage branch from 94617b1 to c4d8f3d Compare June 15, 2022 14:08
@calixteman
Copy link
Contributor

I read the patch and overall it looks like mine but in better ;)
Can imagine a way to test that stuff ?
I thought about that the last week-end and I didn't find anything... (except in using an integration test, print and save, open the generated pdf, dump it as a png and then compare to a ref... and I wasn't super excited by this idea)

@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Jun 15, 2022 •
edited
Loading

I read the patch and overall it looks like mine but in better ;)

Indeed the overall approach of your patch seemed very reasonable, hence my idea was only to try and improve the AnnotationStorage-handling to avoid problems and to ensure that we can't accidentally "block" the main AnnotationStorage-instance.

Can imagine a way to test that stuff ?
I thought about that the last week-end and I didn't find anything... (except in using an integration test, print and save, open the generated pdf, dump it as a png and then compare to a ref... and I wasn't super excited by this idea)

For an integration-test I also cannot imagine anything simple.

The only thing that'd probably not be too difficult would be to add a unit-test that, from an API point of view, simulate what happens in the viewer during printing.

@Snuffleupagus
Copy link
Collaborator Author

@calixteman Would a simple unit-test, as mentioned above, be of (any) value here since that's probably the only really easy way to test that AnnotationStorage-freezing works during printing?

@calixteman
Copy link
Contributor

Yes I think that adding a unit test is better than nothing.
I'll try to figure out a way to correctly test that stuff either here with puppeteer or eventually in m-c.

@Snuffleupagus Snuffleupagus force-pushed the PrintAnnotationStorage branch from c4d8f3d to 0d60225 Compare June 23, 2022 15:05
@Snuffleupagus
[api-minor] Introduce a PrintAnnotationStorage with *frozen* serial…
1cc7cec 
…izable data

Given that printing is triggered *synchronously* in browsers, it's thus possible for scripting (in PDF documents) to modify the Annotation-data while printing is currently ongoing.
To work-around that we add a new printing-specific `AnnotationStorage`, where the serializable data is *frozen* upon initialization, which the viewer can thus create/utilize during printing.
@Snuffleupagus Snuffleupagus force-pushed the PrintAnnotationStorage branch from 0d60225 to 1cc7cec Compare June 23, 2022 15:06
@Snuffleupagus
Copy link
Collaborator Author

/botio unittest

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/37e31a539e41348/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/284e69888fe76d2/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/284e69888fe76d2/output.txt

Total script time: 3.34 mins

  • Unit Tests: FAILED

@pdfjsbot
Copy link

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/37e31a539e41348/output.txt

Total script time: 8.20 mins

  • Unit Tests: Passed

@Snuffleupagus Snuffleupagus marked this pull request as ready for review June 23, 2022 15:22
calixteman
calixteman approved these changes Jun 23, 2022
View reviewed changes
Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

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

Thank you very much.

@Snuffleupagus
Copy link
Collaborator Author

/botio integrationtest

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/fb7a82c2e3350e4/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/a87666d0b5ca2e0/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/fb7a82c2e3350e4/output.txt

Total script time: 4.54 mins

  • Integration Tests: FAILED

@pdfjsbot
Copy link

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/a87666d0b5ca2e0/output.txt

Total script time: 7.77 mins

  • Integration Tests: Passed

@Snuffleupagus Snuffleupagus merged commit 3fab4af into mozilla:master Jun 24, 2022
@Snuffleupagus Snuffleupagus deleted the PrintAnnotationStorage branch June 24, 2022 07:30
@ZeroXClem ZeroXClem mentioned this pull request Aug 12, 2024
Closed
@earthywh earthywh mentioned this pull request Sep 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calixteman calixteman calixteman approved these changes

Assignees
No one assigned
Labels
core viewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Snuffleupagus @calixteman @pdfjsbot