Adds fetch stream logic for networking part of PDF.js #8768

mukulmishra18 · 2017-08-10T07:57:46Z

This PR adds Fetch support in networking part of PDF.js project. This PR implements IPDFStream interfaces using Fetch API to stream PDF data. Using Fetch can give us streamable response, that streams PDF data in small chunks.

P.S. fixes #6126

timvandermeij · 2017-08-10T20:37:42Z

Does this fix #6126?

mukulmishra18 · 2017-08-11T15:33:32Z

Does this fix #6126?

Yes, this patch is intended to add support for fetch stream. Currently it is using fetch if browser has native implementation, although I am thinking if we can add github polyfill for fetch, if browser doesn't have support.

While testing this patch I get to know that we don't have any property named body in Response Object in firefox(but defined in chrome), so 8370f1d#diff-ffa21364c93bc3328ab1eb76226f8a71R84 might throw error. I think we have to detect it and use response.arrayBuffer()(?) instead.

mukulmishra18 · 2017-08-16T19:16:20Z

While testing this PR locally, I find out that this._headers.get(name) is not returning value for Accept-Ranges http response header from 63b4286#diff-ffa21364c93bc3328ab1eb76226f8a71R87, although it is present in the response header(confirmed from the network tab in web console).

Due to this, test for range requests are failing. I am getting Error: 405(Method not allowed) in rangeReader.

yurydelendik · 2017-08-24T16:39:34Z

src/pdf.js

+  } else if (typeof Response !== 'undefined' && 'body' in Response.prototype) {
+    var PDFFetchStream = require('./display/fetch_stream.js').PDFFetchStream;
+    pdfjsDisplayAPI.setPDFNetworkStreamClass(PDFFetchStream);
+   } else {


@Rob--W do you know if we can enable only fetch_stream.js for Chrome extension?

What do you mean?

Whether fetch_stream is disabled for every target except for the Chrome extension build, or

Whether fetch_stream can be unconditionally be used in the Chrome extension, and display/network be removed?

I can't answer 1, but if you meant 2:

The streaming feature requires Chrome 43 - Implement Fetch API for streaming PDF loads on Chrome #6126 (comment)

I looked at the telemetry of the Chrome extension for August 2017 (this month), filtered by users with the latest version of the extension (1.8.557, released July 2017, 17th), with a user agent that contains Chrome/digits.

Based on 1.4M unique data points (i.e. pings where the client cannot be distinguished from another client in the same data set; this already happens when the user upgrades their browser version), the usage statistics are as follows:

Chrome 62 0.91% Chrome 61 0.87% Chrome 60 56.85% <-- Current stable (officially released July 2017, 25th) Chrome 59 32.11% Chrome 58 2.08% Chrome 57 1.17% Chrome 56 0.77% Chrome 55 0.97% Chrome 54 0.20% Chrome 53 0.34% Chrome 52 0.60% Chrome 51 0.33% Chrome 50 0.19% Chrome 49 0.91% <-- last supported Chrome for Windows XP Chrome 48 0.07% Chrome 47 0.17% Chrome 46 0.08% Chrome 45 0.07% Chrome 44 0.04% Chrome 43 0.13% <-- First version where streams API is supported Chrome 42 0.04% Chrome 41 0.06% Chrome 40 0.05% Chrome 39 0.15% Chrome 38 0.01% Chrome 37 0.02% Chrome 36 0.01% Chrome 35 0.38% <-- mostly 35.0.1878.0, win7. Doesn't look like an official build. Chrome 34 0.37% <-- Ubuntu https://apps.ubuntu.com/cat/applications/chromium-browser/ Chrome 33 0.02% Chrome 32 0.01% Chrome 31 0.01% Chrome 100 0.01% <-- I don't know what this is...

1.13% of the users on the latest version of the extension (16k pings) use a Chrome version that does not support the streams API (Chrome 42 and earlier). For now, I'd like to keep supporting these users.

Thanks for response and analysis. I was asking about (2) -- fetch_stream can be unconditionally be used in the Chrome extension. You answered my question, so we will handle Chrome extension no different than generic viewer.

yurydelendik · 2017-08-24T18:46:04Z

@mukulmishra18 Can you rebase the commit to the current master?

yurydelendik · 2017-08-28T21:28:11Z

We are trying to "serialize" the reason we are not suppose too. Add makeReasonSerializable:

diff --git a/src/shared/util.js b/src/shared/util.js
index 9bd0ff1b..15b3f8ff 100644
--- a/src/shared/util.js
+++ b/src/shared/util.js
@@ -1247,6 +1247,17 @@ function wrapReason(reason) {
   }
 }
 
+function makeReasonSerializable(reason) {
+  if (!(reason instanceof Error) ||
+      reason instanceof AbortException ||
+      reason instanceof MissingPDFException ||
+      reason instanceof UnexpectedResponseException ||
+      reason instanceof UnknownErrorException) {
+    return reason;
+  }
+  return new UnknownErrorException(reason.message, reason.toString());
+}
+
 function resolveOrReject(capability, success, reason) {
   if (success) {
     capability.resolve();
@@ -1307,16 +1318,12 @@ function MessageHandler(sourceName, targetName, comObj) {
             data: result,
           });
         }, (reason) => {
-          if (reason instanceof Error) {
-            // Serialize error to avoid "DataCloneError"
-            reason = reason + '';
-          }
           comObj.postMessage({
             sourceName,
             targetName,
             isReply: true,
             callbackId: data.callbackId,
-            error: reason,
+            error: makeReasonSerializable(reason),
           });
         });
       } else if (data.streamId) {

yurydelendik · 2017-08-28T21:30:56Z

src/display/fetch_stream.js

+      redirect: 'follow',
+    }).then((response) => {
+      if (response.status !== 200) {
+        throw validateResponseStatus(response.status, url);


Change name to createResponseStatusError.

Done. Thanks.

yurydelendik · 2017-08-28T21:33:18Z

src/display/fetch_stream.js

+      credentials: this._withCredentials ? 'omit' : 'include',
+      redirect: 'follow',
+    }).then((response) => {
+      if (response.status !== 200) {


you need to validate status based on protocol, for http it is 200 for other protocols it's 0. I recommend to add/have/change validateResponseStatus(status, isHttp) so it will return boolean that will tell is status is valid. Use this function in the range reader as well.

Done with new commit. Thanks.

yurydelendik · 2017-08-28T22:50:42Z

/botio test

pdfjsbot · 2017-08-28T22:50:43Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/b5dc6fa707adeb3/output.txt

pdfjsbot · 2017-08-28T22:50:43Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/d2c468ba2000d41/output.txt

pdfjsbot · 2017-08-28T23:07:14Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/d2c468ba2000d41/output.txt

Total script time: 16.50 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2017-08-28T23:20:14Z

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/b5dc6fa707adeb3/output.txt

Total script time: 29.51 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

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

yurydelendik · 2017-08-29T13:22:21Z

src/display/network_utils.js

+    return false;
+  }
+  return true;
+}


if (!isHttp) { return status === 0; } return status === 200 || status === 206;

Done. Thanks.

yurydelendik · 2017-08-29T13:23:38Z

src/display/fetch_stream.js

+    fetch(url, {
+      method: 'GET',
+      headers: this._headers,
+    }).then((response) => {


define the same options as in full reader. extract the createFetchOptions(headers) method?

created createFetchOptions with new commit. Thanks.

yurydelendik

Looks good, with the validate and options changes.

yurydelendik · 2017-08-30T13:50:21Z

/botio test

pdfjsbot · 2017-08-30T13:50:22Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/9cc359156484d66/output.txt

pdfjsbot · 2017-08-30T13:50:22Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/cad322ff23abd42/output.txt

pdfjsbot · 2017-08-30T14:06:53Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/9cc359156484d66/output.txt

Total script time: 16.50 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2017-08-30T14:19:36Z

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/cad322ff23abd42/output.txt

Total script time: 29.23 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

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

yurydelendik · 2017-08-30T16:27:36Z

filed https://bugzilla.mozilla.org/show_bug.cgi?id=1395220

yurydelendik · 2017-08-30T16:27:57Z

Thank you for the patch.

Adds fetch stream logic for networking part of PDF.js

timvandermeij added the core label Aug 10, 2017

mukulmishra18 force-pushed the fetch_stream branch 5 times, most recently from 688ecd7 to 63b4286 Compare August 16, 2017 18:20

mukulmishra18 force-pushed the fetch_stream branch from 63b4286 to 84d492d Compare August 18, 2017 21:36

yurydelendik reviewed Aug 24, 2017

View reviewed changes

mukulmishra18 force-pushed the fetch_stream branch 2 times, most recently from d242f17 to 4451a97 Compare August 28, 2017 18:19

yurydelendik reviewed Aug 28, 2017

View reviewed changes

mukulmishra18 force-pushed the fetch_stream branch from 4451a97 to e52c0f3 Compare August 28, 2017 22:12

yurydelendik reviewed Aug 29, 2017

View reviewed changes

yurydelendik approved these changes Aug 29, 2017

View reviewed changes

Adds fetch stream logic for networking part of PDF.js

3516a59

mukulmishra18 force-pushed the fetch_stream branch from e52c0f3 to 3516a59 Compare August 29, 2017 17:30

yurydelendik mentioned this pull request Aug 30, 2017

Fixes autofetch and firefox nightly fetch streams #8845

Merged

yurydelendik merged commit bad3203 into mozilla:master Aug 30, 2017

yurydelendik mentioned this pull request Aug 30, 2017

"The provided value 'moz-chunked-arraybuffer' is not a valid enum value... " #6306

Closed

Rob--W mentioned this pull request Aug 31, 2017

Enable "read with streaming" test in test/unit/network_spec.js #8851

Closed

mukulmishra18 mentioned this pull request Sep 1, 2017

Headers are no longer being added to requests #8734

Closed

Snuffleupagus mentioned this pull request Sep 2, 2017

Correctly validate the response status for non-HTTP fetch requests (PR 8768 follow-up) #8866

Merged

This was referenced Jan 23, 2018

Evaluation of support for old Chrome versions #9397

Closed

Implement streaming using responseType=stream (Streams API) #5319

Closed

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#8768 from mukulmishra18/fetch_stream

585ee80

Adds fetch stream logic for networking part of PDF.js

The-Compiler mentioned this pull request Sep 21, 2018

PDF.js v1.10 and later fails with "Failed to fetch" qutebrowser/qutebrowser#4235

Closed

Snuffleupagus mentioned this pull request Sep 19, 2019

first request of range request download whole pdf file？ #11161

Closed

timvandermeij mentioned this pull request Sep 15, 2024

rangeChunkSize Doesnt Work With Version 12.1.2 stephanrauh/ngx-extended-pdf-viewer#2566

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds fetch stream logic for networking part of PDF.js #8768

Adds fetch stream logic for networking part of PDF.js #8768

mukulmishra18 commented Aug 10, 2017 •

edited by yurydelendik

Loading

timvandermeij commented Aug 10, 2017

mukulmishra18 commented Aug 11, 2017

mukulmishra18 commented Aug 16, 2017 •

edited

Loading

yurydelendik Aug 24, 2017

Rob--W Aug 26, 2017 •

edited

Loading

yurydelendik Aug 28, 2017

yurydelendik commented Aug 24, 2017

yurydelendik commented Aug 28, 2017

yurydelendik Aug 28, 2017

mukulmishra18 Aug 28, 2017

yurydelendik Aug 28, 2017

mukulmishra18 Aug 28, 2017

yurydelendik commented Aug 28, 2017

pdfjsbot commented Aug 28, 2017

pdfjsbot commented Aug 28, 2017

pdfjsbot commented Aug 28, 2017

pdfjsbot commented Aug 28, 2017

yurydelendik Aug 29, 2017

mukulmishra18 Aug 29, 2017

yurydelendik Aug 29, 2017

mukulmishra18 Aug 29, 2017

yurydelendik left a comment

yurydelendik commented Aug 30, 2017

pdfjsbot commented Aug 30, 2017

pdfjsbot commented Aug 30, 2017

pdfjsbot commented Aug 30, 2017

pdfjsbot commented Aug 30, 2017

yurydelendik commented Aug 30, 2017

yurydelendik commented Aug 30, 2017

Adds fetch stream logic for networking part of PDF.js #8768

Adds fetch stream logic for networking part of PDF.js #8768

Conversation

mukulmishra18 commented Aug 10, 2017 • edited by yurydelendik Loading

timvandermeij commented Aug 10, 2017

mukulmishra18 commented Aug 11, 2017

mukulmishra18 commented Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Rob--W Aug 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik commented Aug 24, 2017

yurydelendik commented Aug 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik commented Aug 28, 2017

pdfjsbot commented Aug 28, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Aug 28, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented Aug 28, 2017

From: Bot.io (Linux m4)

Success

pdfjsbot commented Aug 28, 2017

From: Bot.io (Windows)

Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik left a comment

Choose a reason for hiding this comment

yurydelendik commented Aug 30, 2017

pdfjsbot commented Aug 30, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented Aug 30, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Aug 30, 2017

From: Bot.io (Linux m4)

Success

pdfjsbot commented Aug 30, 2017

From: Bot.io (Windows)

Failed

yurydelendik commented Aug 30, 2017

yurydelendik commented Aug 30, 2017

mukulmishra18 commented Aug 10, 2017 •

edited by yurydelendik

Loading

mukulmishra18 commented Aug 16, 2017 •

edited

Loading

Rob--W Aug 26, 2017 •

edited

Loading