-
Notifications
You must be signed in to change notification settings - Fork 6
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
VxAdmin: Disable PDF preview/export for large reports #5343
Conversation
56f307a
to
cbe3104
Compare
8d20bfc
to
d4567b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a few places you are added a potential error condition after a "success" log occurs so it would be better to move the log below there and have it log either a "success" or "failure" as appropriate. Otherwise, lgtm.
@@ -88,14 +88,19 @@ export async function generateBallotCountReportPreview({ | |||
|
|||
...reportProps | |||
}: BallotCountReportPreviewProps): Promise<BallotCountReportPreview> { | |||
const report = buildBallotCountReport(reportProps); | |||
await logger.logAsCurrentRole(LogEventId.ElectionReportPreviewed, { | |||
message: `User previewed a ballot count report.`, | |||
disposition: 'success', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to make this log as a failure if there is an error in getting the report preview?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call, thanks 2bdba80
@@ -121,12 +120,18 @@ export async function generateTallyReportPreview({ | |||
message: `User previewed a tally report.`, | |||
disposition: 'success', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same logging comment
/** | ||
* Returns a PDF preview of the write-in adjudication report, as a buffer. | ||
*/ | ||
export async function generateWriteInAdjudicationReportPreview({ | ||
logger, | ||
|
||
...reportProps | ||
}: WriteInAdjudicationReportPreviewProps): Promise<Buffer> { | ||
}: WriteInAdjudicationReportPreviewProps): Promise<WriteInAdjudicationReportPreview> { | ||
const report = buildWriteInAdjudicationReport(reportProps); | ||
await logger.logAsCurrentRole(LogEventId.ElectionReportPreviewed, { | ||
message: `User previewed the write-in adjudication report.`, | ||
disposition: 'success', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
failure logging
Chromium crashes with an out of memory error when trying to generate a PDF for very large documents. Set a limit well below that point to prevent crashes and return a usable error code.
We'll use the preview error to disable the buttons to print/export PDF reports, so we don't need to propagate those errors.
At around 100 pages, the browser starts crashing occasionally during scrolling. To prevent this, set a conservative limit of 50 pages.
Previously, reports with no data showed up as a blank page. Instead, we don't render them at all.
In the report builder, changing the parameters would remove the current PDF preview but leave the page count.
We don't expect any of these PDFs to reach the length limit. If they do, it's an unexpected error, so we can crash explicitly, since that's an improvement over hanging or crashing with a Chromium error.
d4567b6
to
40d199c
Compare
40d199c
to
2bdba80
Compare
Overview
Fixes: #5278
When report PDFs are very very long (~1000 pages), Chromium will crash when trying to render them. In addition, the frontend PDF preview can crash at ~100 pages.
To prevent this, we set two limits:
This PR also fixes two small bugs:
Demo Video or Screenshot
Screen.Recording.2024-08-29.at.4.49.19.PM.mov
Testing Plan
To set the limits, I tested a variety of content lengths and chose conservative numbers. I also updated automated tests
Checklist