1411865 - PDFjs should stop using XHR.responseType="moz-chunked-arraybuffer"

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, enhancement, P3)

Description

[Andrea Marchesini [:baku]]

This is a mozilla-only response type and we are planning to remove it.
This seems to be used only when onProgressiveData is passed as argument in PdfJsNetwork.request(). But I don't see it used in our codebase. Can we maybe remove it easily?

http://searchfox.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfJsNetwork.jsm#101-105

Comment 1

[Yury Delendik (:yury)]

It's also used here https://github.com/mozilla/pdf.js/blob/master/src/display/network.js#L120 -- notice that the PDF.js library maybe be used my multiple document hosting vendors (e.g. OneDrive, DropBox, etc.) The PDF.js just implemented support for fetch() and will use it instead of moz-chunked-arraybuffer. I recommend to wait until bug 1389628 will reach the Firefox release channel and https://github.com/mozilla/pdf.js/pull/8768 reaches PDF.js release version (expected 1.10), to not regress to many people.

Comment 2

[Thomas Wisniewski [:twisniewski]]

yury, are we ready to proceed here? I'm not sure what still needs to be done, given that PDF.js 2 has shipped with Firefox for a while (and so has ReadableStream).

Comment 3

[Thomas Wisniewski [:twisniewski]]

:Snuffleupagus, would you happen to know if we still need -moz-chunked-arraybuffer for pdf.js in central? If not, maybe we can try to remove it.

Comment 4

[Jonas Jenwald [:Snuffleupagus]]

As outlined in comment 1 the issue isn't so much the mozilla-central version of PDF.js, but rather the generic library using moz-chunked-arraybuffer. Personally I'd advise to postpone the removal until ReadableStream, which was enabled by default in bug 1505122, is shipping in a Firefox ESR version.

Comment 5

[Jonas Jenwald [:Snuffleupagus]]

Looking at comment 3 again, I cannot help feeling that there's a few different questions being asked. Hence I'll obsolete my previous comment and try break this up into a hopefully more helpful response:

1. Can the platform support, i.e. https://searchfox.org/mozilla-central/rev/486b64e4bff86b7988af8c8b80845404ad197533/dom/webidl/XMLHttpRequest.webidl#25 and any related code, for moz-chunked-arraybuffer be removed now?

   Yes, as far as I'm concerned. (I'd guess that this is the most important part of the question, from a wider Firefox perspective.)

2. Can the moz-chunked-arraybuffer support be removed from the Firefox-specific version of PDF.js now?

   Yes, because as far as I can tell that particular branch is actually dead code; please see the attached patch for additional information.

3. Can the moz-chunked-arraybuffer support be removed from the generic PDF.js library?

   No, not until both Fetch and ReadableStream support is available in Firefox ESR. However, as can be seen in https://github.com/mozilla/pdf.js/blob/bce9ff7347266e18326fff52c201faba04a96bd7/src/display/network.js#L56-L73 feature detection is used, hence I cannot see the harm in leaving this intact for the time being.
   Unless I'm missing something here, this last point should be irrelevant for the platform removal of moz-chunked-arraybuffer though.

The above is my take, as an occasional PDF.js contributor, on the state of this bug.
Thomas, does this help clarify the current situation?

Comment 6

[Jonas Jenwald [:Snuffleupagus]]

Attachment: bug1411865.patch

Comment 7

[Thomas Wisniewski [:twisniewski]]

Ah, thank you for the details, that does clarify things.

I was asking as I'd like to try removing platform support for -moz-chunked-arraybuffer in bug 1120171, and this seemed to be the only obvious blocker I could find.

Comment 8

[Rob Wu [:robwu]]

Reviewing has moved off Bugzilla to Phabricator.

Jonas, can you and do you want to upload your patch to Phabricator? (see https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html)
If not, then I am willing to submit a patch to remove the unused code to get the ball rolling.

Comment 9

[Jonas Jenwald [:Snuffleupagus]]

(In reply to Rob Wu [:robwu] from comment #8)

Reviewing has moved off Bugzilla to Phabricator.

I'm aware of that, and find it to be a very unfortunate decision.

Jonas, can you and do you want to upload your patch to Phabricator? (see https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html)

It seems that you need to enable 2FA in order to use Phabricator, and I don't have a device that I'm able to use for that purpose.

If not, then I am willing to submit a patch to remove the unused code to get the ball rolling.

It would be great if you could submit my patch from https://bugzilla.mozilla.org/show_bug.cgi?id=1411865#c6, since I'm not able to myself (unless Mozilla is willing to send me a 2FA device); thanks for offering to help!

Comment 10

[Rob Wu [:robwu]]

Attachment: Bug 1411865 - Remove dead code from PdfJsNetwork.jsm

• onProgressiveData callback is never set, so everything that is
  conditional on it can be removed.
• loadedRequests has never been used, it can be removed.
• Several other methods are unused and not part of any interface;
  These can also be removed.

Comment 11

[Thomas Wisniewski [:twisniewski]]

Rob, shall I help get the patch here across the finish line, given Jonas cannot access Phabricator?

Comment 12

[Rob Wu [:robwu]]

I have already submitted the patch (with more cleanups besides Jonas' changes - see https://phabricator.services.mozilla.com/D24745).

The only blocker on this bug is a review from Brendan.

Comment 13

[Rob Wu [:robwu]]

Comment on attachment 9052823 [details] [diff] [review]
bug1411865.patch

Obsoleted by https://phabricator.services.mozilla.com/D24745

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1611d7c06c13
Remove dead code from PdfJsNetwork.jsm r=bdahl

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/1611d7c06c13