Skip to content

Ensure that pending requests are resolved when calling PDFDataTransportStreamReader.prototype.progressiveDone#20634

Merged
timvandermeij merged 3 commits intomozilla:masterfrom
Snuffleupagus:progressiveDone-resolve-requests
Feb 8, 2026
Merged

Ensure that pending requests are resolved when calling PDFDataTransportStreamReader.prototype.progressiveDone#20634
timvandermeij merged 3 commits intomozilla:masterfrom
Snuffleupagus:progressiveDone-resolve-requests

Conversation

@Snuffleupagus
Copy link
Collaborator

Doing skip-cache reloading of https://opensource.adobe.com/dc-acrobat-sdk-docs/pdfstandards/PDF32000_2008.pdf#disableRange=true in the latest Firefox Nightly version I noticed an intermittent bug, where the loadingBar would fill up but without the PDF ever rendering.
Initially I was quite worried that the changes in PR #20602 had somehow caused this, however after a bunch of testing in a slightly older Nightly I was able to reproduce the problem there as well.[1]

Instead I believe that this problem goes all the back to PR #10675, so still my fault, since the progressiveDone handling there was incomplete.
Note how we only set the this._done property, but don't actually resolve any still pending requests. For reference, compare with the handling in the PDFDataTransportStreamRangeReader.prototype._enqueue method.
Depending on the exact order in which the PDFDataTransportStreamReader.prototype.{_enqueue, read, progressiveDone} methods are invoked, which depends on how/when the PDF data arrives, the bug might occur.

The reason that this bug hasn't been caught before now is likely that disableRange=true isn't used by default and Firefox users are unlikely to manually set that in e.g. about:config.


[1] Possibly some timings changed to make it slightly more common, but given the intermittent nature of this it's difficult to tell.

@Snuffleupagus Snuffleupagus force-pushed the progressiveDone-resolve-requests branch from 09796ab to 6f2bb9e Compare February 7, 2026 12:05
@Snuffleupagus Snuffleupagus marked this pull request as draft February 7, 2026 19:50
…ortStreamReader.prototype.progressiveDone`

Doing skip-cache reloading of https://opensource.adobe.com/dc-acrobat-sdk-docs/pdfstandards/PDF32000_2008.pdf#disableRange=true in the latest Firefox Nightly version I noticed an *intermittent* bug, where the loadingBar would fill up but without the PDF ever rendering.
Initially I was quite worried that the changes in PR 20602 had somehow caused this, however after a bunch of testing in a slightly older Nightly I was able to reproduce the problem there as well.[1]

Instead I believe that this problem goes all the back to PR 10675, so still my fault, since the `progressiveDone` handling there was incomplete.
Note how we only set the `this._done` property, but don't actually resolve any still pending requests. For reference, compare with the handling in the `PDFDataTransportStreamRangeReader.prototype._enqueue` method.
Depending on the exact order in which the `PDFDataTransportStreamReader.prototype.{_enqueue, read, progressiveDone}` methods are invoked, which depends on how/when the PDF data arrives, the bug might occur.

The reason that this bug hasn't been caught before now is likely that `disableRange=true` isn't used by default and Firefox users are unlikely to manually set that in e.g. `about:config`.

---
[1] Possibly some timings changed to make it slightly more common, but given the intermittent nature of this it's difficult to tell.
@Snuffleupagus Snuffleupagus force-pushed the progressiveDone-resolve-requests branch from 6f2bb9e to 2d643ef Compare February 7, 2026 21:26
@mozilla mozilla deleted a comment from moz-tools-bot Feb 7, 2026
@mozilla mozilla deleted a comment from moz-tools-bot Feb 7, 2026
@mozilla mozilla deleted a comment from moz-tools-bot Feb 7, 2026
@mozilla mozilla deleted a comment from moz-tools-bot Feb 7, 2026
@Snuffleupagus Snuffleupagus marked this pull request as ready for review February 7, 2026 21:38
…nsport_stream.js` and `src/display/network.js`

Currently the same identical code is duplicated four times per file, which seems completely unnecessary.
Note that the function isn't placed in `src/display/network_utils.js`, since that file isn't included in MOZCENTRAL builds.
… with `PDFNodeStreamReader` and `PDFNodeStreamRangeReader`

Given that the Node.js code uses standard `ReadableStream`s now, see PR 20594, it can use the same `getArrayBuffer` as the Fetch API implementation.

Also, change the `getArrayBuffer` fallback case to an Error (rather than a warning) since that should never actually happen.
@mozilla mozilla deleted a comment from moz-tools-bot Feb 8, 2026
@mozilla mozilla deleted a comment from moz-tools-bot Feb 8, 2026
@mozilla mozilla deleted a comment from moz-tools-bot Feb 8, 2026
@mozilla mozilla deleted a comment from moz-tools-bot Feb 8, 2026
@Snuffleupagus
Copy link
Collaborator Author

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/76d0720fc56399c/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/87c9cd5e32c519d/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/76d0720fc56399c/output.txt

Total script time: 42.15 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 1

Image differences available at: http://54.241.84.105:8877/76d0720fc56399c/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/87c9cd5e32c519d/output.txt

Total script time: 84.09 mins

  • Unit tests: FAILED
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 1

Image differences available at: http://54.193.163.58:8877/87c9cd5e32c519d/reftest-analyzer.html#web=eq.log

@timvandermeij timvandermeij removed the request for review from calixteman February 8, 2026 13:56
@timvandermeij timvandermeij merged commit 2b95a8e into mozilla:master Feb 8, 2026
11 checks passed
@timvandermeij
Copy link
Contributor

Thank you!

@Snuffleupagus Snuffleupagus deleted the progressiveDone-resolve-requests branch February 8, 2026 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants