Skip to content

Update the signatures-helpers to actually handle MissingDataExceptions - #21652

Merged
timvandermeij merged 3 commits into
mozilla:masterfrom
Snuffleupagus:signatures-async-helpers
Jul 28, 2026
Merged

Update the signatures-helpers to actually handle MissingDataExceptions#21652
timvandermeij merged 3 commits into
mozilla:masterfrom
Snuffleupagus:signatures-async-helpers

Conversation

@Snuffleupagus

Copy link
Copy Markdown
Collaborator

The PDFDocument.prototype.signatures getter returns a (shadowed) Promise, however the way that it invokes various helper-methods can lead to intermittent failures to parse the signature data.
These helper-methods will lookup a fair amount of Dictionary data, however any one of those cases could throw MissingDataException during document loading.

To avoid having to re-factor those methods a lot, and adding a bunch more pdfManager.ensureDoc() calls, they are instead made asynchronous and the Dictionary lookups changed to use Dict.prototype.getAsync (similar to the existing fieldObjects handling).
Technically this additional asynchronicity may be ever so slightly slower, however I don't think it matters in practice since: most PDFs don't have any signatures, the signature-UI is initialized lazily in the viewer, and finally fetching/parsing of signatures do not block rendering.
Also, note how multiple values are being fetched in parallel in order to attempt to reduce overall asynchronicity.

Note: The unit-test changes are essentially fixing pre-existing bugs, that this patch exposed, since the test-only Dict instances weren't able to fetch indirect objects.

The `PDFDocument.prototype.signatures` getter returns a (shadowed) Promise, however the way that it invokes various helper-methods can lead to *intermittent* failures to parse the signature data.
These helper-methods will lookup a fair amount of Dictionary data, however any one of those cases could throw `MissingDataException` during document loading.

To avoid having to re-factor those methods a lot, and adding a bunch more `pdfManager.ensureDoc()` calls, they are instead made asynchronous and the Dictionary lookups changed to use `Dict.prototype.getAsync` (similar to the existing `fieldObjects` handling).
Technically this additional asynchronicity may be ever so slightly slower, however I don't think it matters in practice since: most PDFs don't have any signatures, the signature-UI is initialized lazily in the viewer, and finally fetching/parsing of signatures do not block rendering.
Also, note how multiple values are being fetched in parallel in order to attempt to reduce overall asynchronicity.

*Note:* The unit-test changes are essentially fixing pre-existing bugs, that this patch exposed, since the test-only `Dict` instances weren't able to fetch indirect objects.
Given that these checks are synchronous, we can avoid a little bit of unnecessary data-fetching if the `ByteRange` is invalid.
@Snuffleupagus
Snuffleupagus requested a review from calixteman July 28, 2026 14:20
@Snuffleupagus Snuffleupagus added core release-blocker Blocker for the upcoming release labels Jul 28, 2026
@codecov-commenter

codecov-commenter commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.83%. Comparing base (1609bd8) to head (c9eaf13).

Files with missing lines Patch % Lines
src/core/document.js 94.73% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #21652      +/-   ##
==========================================
- Coverage   89.83%   89.83%   -0.01%     
==========================================
  Files         263      263              
  Lines       66899    66888      -11     
==========================================
- Hits        60100    60086      -14     
- Misses       6799     6802       +3     
Flag Coverage Δ
browsertest 66.54% <0.00%> (-0.01%) ⬇️
fonttest 9.03% <ø> (ø)
integrationtest 69.23% <89.47%> (-0.01%) ⬇️
unittest 57.52% <94.73%> (-0.02%) ⬇️
unittestcli 56.46% <94.73%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@timvandermeij timvandermeij left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

r=me, with passing browser tests. Thank you for improving this!

@timvandermeij

Copy link
Copy Markdown
Collaborator

/botio browsertest

@moz-tools-bot

Copy link
Copy Markdown
Collaborator

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/7e55c3e407e5661/output.txt

@moz-tools-bot

Copy link
Copy Markdown
Collaborator

From: Bot.io (Windows)


Received

Command cmd_browsertest from @timvandermeij received. Current queue size: 1

Live output at: http://54.193.163.58:8877/26d25e84d922709/output.txt

@moz-tools-bot

Copy link
Copy Markdown
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/7e55c3e407e5661/output.txt

Total script time: 17.96 mins

  • Regression tests: Passed

@moz-tools-bot

Copy link
Copy Markdown
Collaborator

From: Bot.io (Windows)


Success

Full output at http://54.193.163.58:8877/26d25e84d922709/output.txt

Total script time: 23.40 mins

  • Regression tests: Passed

@timvandermeij
timvandermeij merged commit f704f2e into mozilla:master Jul 28, 2026
21 checks passed
@timvandermeij
timvandermeij removed the request for review from calixteman July 28, 2026 19:34
@Snuffleupagus
Snuffleupagus deleted the signatures-async-helpers branch July 28, 2026 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core release-blocker Blocker for the upcoming release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants