1766030 - Downloading from internal PDF viewer produces the wrong download link

Status: VERIFIED FIXED

(Firefox :: PDF Viewer, defect, P1)

Description

[Eric Rescorla (:ekr)]

STR

1. Set Firefox to internally handle PDFs
2. Navigate to a PDF
3. Press the download button in the PDF UI
4. Confirm the download.
5. Open the downloads menu
6. Right-click on the item you downloaded and select "Copy Download Link"

Expected Result:
Get the actual URL

Actual Result:
Get a URL of the form blob:resource://pdf.js/...

Comment 1

[Eric Rescorla (:ekr)]

Relatedly, clicking on the download re-opens it in Firefox, and it's not clear how to open it in Preview from inside Fx.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Firefox::Downloads Panel' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 3

[Micah [:mtigley] (she/her)]

This seems to be pdf.js bug since it uses a different method of saving vs. the context menu's "Save Link As" option.

For pdf.js, we use the the file data's blob URL get the download link, whereas we take a different route via the context menu using _saveURL.

I'll move this issue over to the pdf.js component, but feel free to move it back to downloads panel if that's not the case.

Comment 4

[Calixte Denizet (:calixte)]

The way pdfs are saved should be the same as other files in FIrefox.
There is an exception for pdfs containing a form filled by the user and then saved: in this case we're creating a new pdf and as far as I know there is no other way to save the file except in using a Blob.
The json viewer does something similar when a json is pretty-printed and then saved, the url corresponds to Blob.

Comment 5

[Calixte Denizet (:calixte)]

:mtigley, do you know if it's possible to save a file in using a blob and in specifying the url which will be used in the download panel ?
The easy fix would be to pass a null instead of a blob:
https://github.com/mozilla/pdf.js/blob/b10b8dad7dd232318375d6a4d7e2e57042a4c3f0/web/app.js#L973
but I guess that in this case the file will be downloaded a second time.

Comment 6

[Jonas Jenwald [:Snuffleupagus]]

Please note that there's two reasons for always downloading using a Blob:

• For the general PDF.js library data can be opened from e.g. a TypedArray, in which case no URL exists to download from. (That might be less relevant here, but just a FYI.)
• To avoid having to re-download already existing data, since it's theoretically possible that a URL is only e.g. valid once or for a limited time (e.g. if the PDF document is protected with a login or similar).

Possibly a stupid idea:
Initially I wasn't actually able to reproduce this bug, until I realized that it's because I had set the browser.download.improvements_to_download_panel = false preference manually.
Hence it seems that things "just work" if we take the following code-path, and my question is thus why we cannot just do that unconditionally?

https://searchfox.org/mozilla-central/rev/ecd91b104714a8b2584a4c03175be50ccb3a7c67/toolkit/components/pdfjs/content/PdfStreamConverter.jsm#369-374

Comment 7

[Marco Castelluccio [:marco]]

(In reply to Jonas Jenwald [:Snuffleupagus] from comment #6)

Initially I wasn't actually able to reproduce this bug, until I realized that it's because I had set the browser.download.improvements_to_download_panel = false preference manually.
Hence it seems that things "just work" if we take the following code-path, and my question is thus why we cannot just do that unconditionally?

https://searchfox.org/mozilla-central/rev/ecd91b104714a8b2584a4c03175be50ccb3a7c67/toolkit/components/pdfjs/content/PdfStreamConverter.jsm#369-374

When browser.download.improvements_to_download_panel is false we do not follow that code path.
browser.download.improvements_to_download_panel is true by default since 98 (see bug 1733587).

Comment 8

[Jonas Jenwald [:Snuffleupagus]]

Sorry, I copied the wrong code!
What I should have linked is the following code, which we could run for everything that's not a "save": https://searchfox.org/mozilla-central/rev/ecd91b104714a8b2584a4c03175be50ccb3a7c67/toolkit/components/pdfjs/content/PdfStreamConverter.jsm#377-461

Comment 9

[:Gijs (out for now; he/him)]

(In reply to Eric Rescorla (:ekr) from comment #1)

Relatedly, clicking on the download re-opens it in Firefox, and it's not clear how to open it in Preview from inside Fx.

Right click the download entry, and instead of copying the URL, click "open in Preview".

(In reply to Jonas Jenwald [:Snuffleupagus] from comment #8)

What I should have linked is the following code, which we could run for everything that's not a "save": https://searchfox.org/mozilla-central/rev/ecd91b104714a8b2584a4c03175be50ccb3a7c67/toolkit/components/pdfjs/content/PdfStreamConverter.jsm#377-461

We're trying to get rid of that path (see bug 1757771 and the many, many see also bugs, also including the various bugs caused by that code path existing at all and needing to be accounted for in the "what do you want to do with this file" dialog); it also opens an additional dialog that isn't useful for most cases, and it would re-download the PDF (which is wasteful).

It looks like Marco has identified the "regressing" bug, which explains why we take the codepath we take.

(In reply to Calixte Denizet (:calixte) from comment #5)

:mtigley, do you know if it's possible to save a file in using a blob and in specifying the url which will be used in the download panel ?

No. The link that gets copied is the source.url property of the download: https://searchfox.org/mozilla-central/rev/ecd91b104714a8b2584a4c03175be50ccb3a7c67/browser/components/downloads/DownloadsCommon.jsm#445 .

If we wanted something else, we'd need to add additional properties (originalSource or w/e) to the download to "pretend" that it came from source A when it actually came from source B, and prefer those if present with a fallback to source.url.

Comment 10

[:Gijs (out for now; he/him)]

(In reply to :Gijs (he/him) from comment #9)

No. The link that gets copied is the source.url property of the download: https://searchfox.org/mozilla-central/rev/ecd91b104714a8b2584a4c03175be50ccb3a7c67/browser/components/downloads/DownloadsCommon.jsm#445 .

If we wanted something else, we'd need to add additional properties (originalSource or w/e) to the download to "pretend" that it came from source A when it actually came from source B, and prefer those if present with a fallback to source.url.

... I guess alternatively, you could hand-roll the physical file-saving and "manually" add the completed download object to the list with the "right" URL? Feels like cheating, and now you're on your own for error handling and such, and you'd probably bypass safe browsing checks and such...

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1740135

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1740135

Comment 13

[Calixte Denizet (:calixte)]

Attachment: Bug 1766030 - Add an optional source URL when saving an URL. r=gijs

In pdf.js, files are saved thanks to a blob but the original URL is lost.
Consequently, the download panel doesn't contain any information about the
origins of a saved pdf.
The saveURL, internalSave and nsITransfer.init functions has now a parameter for this originalURL.

Comment 14

[Calixte Denizet (:calixte)]

A try:
https://treeherder.mozilla.org/jobs?repo=try&revision=d2d50a9e4a7d90f430b0b4591abef44add9cb03a

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a20f51b3289
Add an optional source URL when saving an URL. r=Gijs

Comment 16

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/3a20f51b3289

Comment 17

[Lasana Murray]

Attachment: Port Bug 1766030 - Add missing parameter to internalSave() call. r=#thunderbird-reviewers

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party.

Comment 19

[Phabricator Automation]

Comment on attachment 9283470 [details]
Port Bug 1766030 - Add missing parameter to internalSave() call. r=#thunderbird-reviewers

Revision D150626 was moved to bug 1777323. Setting attachment 9283470 [details] to obsolete.

Comment 20

[Monica Chiorean]

Issue is still reproducing on Win10/Ubuntu 20.4 using build Beta 103.0b6 (20220707185904), 'Copy Download link =blob:resource://pdf.js/ab54bb36-4213-446f-93af-18ee56e71484'. Same behavior on Nightly 104.0a1 (20220708093332).
Is this expected? Thank you.

Comment 21

[Calixte Denizet (:calixte)]

@Monica, could you add a str please ?
I just tried with the pdf (2nd attachment) in this https://bugzilla.mozilla.org/show_bug.cgi?id=1778692 and it wfm.

Comment 22

[Monica Chiorean]

Attachment: Screenplay.gif

1. Open FF.
2. Open pdf file. (I used "http://www.pdf995.com/samples/pdf.pdf ").
3. Download pdf.
4. Open download file (Library).
5. Right click on downloaded pdf 'Copy Download link'.
6. Paste the copied link to a new tab.

Actual result: Copied file is "blob:resource://pdf.js/9f79bca1-df79-4060-b207-24dd3f6d5374".

Attached steps and screenplay. Please let me know if there is something else that I need to set. Thank you.

Comment 23

[Calixte Denizet (:calixte)]

Oh, I got it.
I fixed the issue for the downloads panel reachable from the toolbar button.
Could file a new bug please and I'll fix it ? Thank you.

Comment 24

[Monica Chiorean]

Verified issue is fixed only if downloads panel is reachable from the toolbar button on Beta 103.0b6 and Nightly 104.0a1 for Win10/Ubuntu20.4/Mac 10.13.
Logged bug 1778714 for the part where the download panel is reached from Library.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

What do you think about taking this on ESR102? It grafts cleanly and it might be nice to get some of these download fixes landed there before ESR91 users get migrated over to ESR102 in a couple weeks.

Comment 27

[Calixte Denizet (:calixte)]

I think it'd be nice to have this patch and the one from bug 1778714 in ESR: they're simple, tested and they didn't cause any regressions in Firefox.
Do I have something to do to have them in ESR102 ?

Comment 28

[:Gijs (out for now; he/him)]

(In reply to Calixte Denizet (:calixte) from comment #27)

I think it'd be nice to have this patch and the one from bug 1778714 in ESR: they're simple, tested and they didn't cause any regressions in Firefox.
Do I have something to do to have them in ESR102 ?

Go to the attachment details for your patch and request esr approval, then fill in the form that appears.

Comment 29

[Calixte Denizet (:calixte)]

Comment on attachment 9278747 [details]
Bug 1766030 - Add an optional source URL when saving an URL. r=gijs

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Ergonomics fix for PDF downloads
• User impact if declined: User will get a blob url when they will copy the url from the download panel.
• Fix Landed on Version: 103
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small changes and it has an automated test.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9278747 [details]
Bug 1766030 - Add an optional source URL when saving an URL. r=gijs

Approved for 102.3esr.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/778f4b2173c6

Comment 32

[Monica Chiorean]

Verified issue as fixed on Win10, Ubuntu20.04, Mac10.13 using FF build 102.3.0esr( 20220912135840).