Ensure that the response-origin of range requests match the full request (issue 12744)

[Snuffleupagus]

The following cases are excluded in the patch:

• The Firefox PDF Viewer, since it has been fixed on the platform side already; please see https://bugzilla.mozilla.org/show_bug.cgi?id=1683940

• The PDFNodeStream-implementation, used in Node.js environments, since after recent changes that code only supports file://-URLs.

Also updates the read-methods, on both PDFNetworkStreamFullRequestReader and PDFNetworkStreamRangeRequestReader, to await the headers before returning any data similarly to the implementation in src/display/fetch_stream.js.

Note: The relevant unit-tests are updated to await the headersReady Promise before dispatching range requests, since that's consistent with the actual usage in the src/-folder.

Fixes #12744

[Snuffleupagus]

/botio test

[moz-tools-bot]

From: Bot.io (Windows)

---

Received

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

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

[moz-tools-bot]

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/a250d0ad1d48219/output.txt

[moz-tools-bot]

From: Bot.io (Linux m4)

---

Failed

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

Total script time: 30.73 mins

• Unit tests: Passed
• Integration Tests: Passed
• Regression tests: FAILED

  different ref/snapshot: 17
  different first/second rendering: 2

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

[moz-tools-bot]

From: Bot.io (Windows)

---

Failed

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

Total script time: 46.16 mins

• Unit tests: Passed
• Integration Tests: FAILED
• Regression tests: Passed

[Snuffleupagus]

I created a PR with unit tests at #19074.

Thank you!

Your patch currently stores _storedError, which is used at the start of _read(). But if _read() is called immediately, then initially there is no _storedError and it will just wait for the response and pass that through. Please fix that.

Hopefully that's fixed now, by using a similar pattern of waiting for the headers as in src/display/fetch_stream.js.

[Rob--W]

The functional behavior of the patch looks good to me. I added a comment below with a suggestion to simplify, basically getting rid of the new _readCapability promise in favor of rejecting existing request promises. Consider this patch approved and ready to merge with that suggestion applied.

A note to a comment in the PR text / commit message:

Note: The relevant unit-tests are updated to await the headersReady Promise before dispatching range requests, since that's consistent with the actual usage in the src/-folder.

Note: this test fixup was needed because this patch introduces a new requirement: getRangeReader can only be invoked on streams that were preceded by a full request, at least until the point that the headers were received. Independently of whether the current code satisfies that requirement, we should document this expectation.

I confirmed your claim that the current code in src/ awaits headersReady before dispatching range requests, with the entry point being at getPdfManager in worker.js - our implementation does this because in order to know whether we need to switch to range requests, we first need to know whether the server supports range requests. Currently the only supported way to do so is to send another request.

As an example of a differing implementation: the Chrome extension has already received the headers at the time that the viewer is opened, so in theory it could forward the headers and response origin to the viewer so that it can immediately try switch to range requests faster (without doing the initial full request). I currently have no plans to implement that in the Chrome extension due to added complexity though.

[Snuffleupagus]

Consider this patch approved and ready to merge with that suggestion applied.

Fixed, thank you.

getRangeReader can only be invoked on streams that were preceded by a full request, at least until the point that the headers were received. Independently of whether the current code satisfies that requirement, we should document this expectation.

I've added a comment in src/interfaces.js, since that's what all stream implementations are based upon.

As an example of a differing implementation: the Chrome extension has already received the headers at the time that the viewer is opened, so in theory it could forward the headers and response origin to the viewer so that it can immediately try switch to range requests faster (without doing the initial full request). I currently have no plans to implement that in the Chrome extension due to added complexity though.

So basically, the Chrome extension could be changed to instead use PDFDataRangeTransport (similar to the Firefox PDF Viewer).
Although, the part about avoiding additional complexity makes sense.

[Snuffleupagus]

/botio unittest

[moz-tools-bot]

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/401c70deab6d163/output.txt

[moz-tools-bot]

From: Bot.io (Windows)

---

Received

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

Live output at: http://54.193.163.58:8877/06175bf9956bd84/output.txt

[moz-tools-bot]

From: Bot.io (Linux m4)

---

Success

Full output at http://54.241.84.105:8877/401c70deab6d163/output.txt

Total script time: 2.59 mins

• Unit Tests: Passed

[moz-tools-bot]

From: Bot.io (Windows)

---

Success

Full output at http://54.193.163.58:8877/06175bf9956bd84/output.txt

Total script time: 6.81 mins

• Unit Tests: Passed