Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Rework usage of FPDF_GetLastError() and PdfiumLibraryBindings::get_pdfium_last_error(). #78

Closed
ajrcarey opened this issue Mar 22, 2023 · 3 comments
Assignees

Comments

@ajrcarey
Copy link
Owner

ajrcarey commented Mar 22, 2023

Indirect follow-on from #60. As pointed out by https://github.com/mara004, FPDF_GetLastError() only applies to nominated functions related to document loading, not to the Pdfium library generally. As a result, it is actually potentially harmful to use this function to check if other API calls returned an error, as it may return a false positive by re-raising an error previously already caught during document loading (since a call to FPDF_GetLastError() does not clear the error flag).

A recent update to the inline documentation clarifies that FPDF_GetLastError() should only be checked after specific functions: https://pdfium.googlesource.com/pdfium/+/e116d6ebc253640da997d10b533dd9aed6aa1c23

In light of this, rework all usage of FPDF_GetLastError() and PdfiumLibraryBindings::get_pdfium_last_error(). Consider removing get_pdfium_last_error() entirely.

@mara004
Copy link

mara004 commented Mar 23, 2023

Thanks for the clear explanation on possible false positives.
Apart from that, calling FPDF_GetLastError() in other places is also just unnecessary and may have a slightly negative performance impact.

ajrcarey pushed a commit that referenced this issue Mar 23, 2023
ajrcarey pushed a commit that referenced this issue Mar 23, 2023
@ajrcarey
Copy link
Owner Author

Removed PdfiumLibraryBindings::get_pdfium_last_error(). Reworked all usage of get_pdfium_last_error(). Restricted usage of FPDF_GetLastError() to Pdfium::pdfium_document_handle_to_result(). Confirmed all tests pass. Updated README.md.

@ajrcarey ajrcarey self-assigned this Mar 23, 2023
@ajrcarey
Copy link
Owner Author

Scheduled for release as part of 0.8.0.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants