Ensure that the response-origin of range requests match the full request (issue 12744) #19028
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
8b48921
to
b407dbb
Compare
This comment was marked as outdated.
This comment was marked as outdated.
b407dbb
to
9c38dd5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
src/display/fetch_stream.js
Outdated
| @@ -121,6 +124,8 @@ class PDFFetchStreamReader { | |||
| createFetchOptions(headers, this._withCredentials, this._abortController) | |||
| ) | |||
| .then(response => { | |||
| stream._responseOrigin.resolve(getResponseOrigin(response.url)); | |||
There was a problem hiding this comment.
This part may not be reached when the network request fails. Is it desirable to indefinitely postpone the resolution of the other responses?
There was a problem hiding this comment.
Hopefully I've not misunderstood your comment, but please see below for why I don't believe this to be an issue in practice.
Ignoring the unit-tests here, please note that if the initial (i.e. full) request fails then we'll never actually get to a point where we dispatch any range requests.
More specifically, if the full request fails we'll end up here:
pdf.js/src/display/fetch_stream.js
Line 152 in 9bf9bbd
Lines 2654 to 2655 in 9bf9bbd
That message is then handled on the worker-thread:
pdf.js/src/core/worker_stream.js
Lines 64 to 70 in 9bf9bbd
Lines 262 to 265 in 9bf9bbd
There was a problem hiding this comment.
The latest version of the patch updates the unit-tests to agree with the "actual" usage, since that removes the need for the additional Promises in the code.
src/display/network.js
Outdated
| @@ -420,6 +430,14 @@ class PDFNetworkStreamRangeRequestReader { | |||
| this.> | |||
| } | |||
|
|
|||
| _onHeadersReceived() { | |||
| this.#responseOrigin.resolve( | |||
There was a problem hiding this comment.
Again this stage may never be reached. Shouldn't we resolve (or reject) earlier to avoid requests getting stuck?
There was a problem hiding this comment.
This would be similar to the situation outlined for src/display/fetch_stream.js above.
9c38dd5
to
84a81f2
Compare
…est (issue 12744) 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. *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.
84a81f2
to
0b4cf74
Compare
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 supportsfile://-URLs.Note: The relevant unit-tests are updated to await the
headersReadyPromise before dispatching range requests, since that's consistent with the actual usage in thesrc/-folder.Fixes #12744