Closed Bug 1719892 Opened 3 years ago Closed 3 years ago

Opening a PDF sending CD: attachment should load directly from internet

Categories

(Firefox :: Downloads Panel, defect, P3)

defect
Points:
5

Tracking

()

RESOLVED FIXED
96 Branch
Tracking Status
firefox96 --- fixed

People

(Reporter: mtigley, Assigned: enndeakin)

References

Details

(Whiteboard: [fidefe-Outreachy2021])

Attachments

(4 files)

Steps to Reproduce

  1. Ensure browser.download.improvements_to_download_panel pref is set to true.
  2. Ensure "Save files to <path>" option is selected from about:preferences and that PDFs are configured to open in Firefox
  3. Try opening a PDF that sends CD: attachment. (eg. open any google doc and use File > Print inside the gdocs UI)

Expected Results
File should be loaded directly from internet instead of local disk, so that the address bar is correct

Actual Results
File is saved to local disk and instead shows the local file path from the address bar

Blocks: 1719895
Severity: -- → S3
Priority: -- → P3
Whiteboard: [fidefe-Outreachy2021]
See Also: → 1726902
Points: --- → 5
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED

As part of the scope of this issue, could we add telemetry that captures if the PDF opened originates from opening a PDF sending CD: attachment?
This would help validate that downloads lost on 'downloads.added' are compensated by pdf opens added to 'pdf.viewer.used'.

(In reply to Romain Testard [:RT] from comment #1)

As part of the scope of this issue, could we add telemetry that captures if the PDF opened originates from opening a PDF sending CD: attachment?
This would help validate that downloads lost on 'downloads.added' are compensated by pdf opens added to 'pdf.viewer.used'.

We can easily add an extra bit of information that indicates this, but I think it would be as part of 1736864.

Blocks: 1738574

I first attempted to modify nsDocumentOpenInfo::DispatchContent such that it changes 'forceExternalHandling' to false when a pdf is opened and the user has configured 'internal' as the action to take. This way it instead goes directly to the stream conversion and uses the pdf viewer instead. However, this did not work.

A link to a pdf file first gets handled in the parent process via nsDocumentOpenInfo::DispatchContent to decide whether it should be opened internally or not. It then gets handled in the child process in the same function. However the mime info for the child process is not set to the expected values, so I cannot detect the settings here.

If I hard-code 'forceExternalHandling' to false in both processes for pdfs, then it gets past this stage and the pdfjs viewer gets invoked. It doesn't work if only one process does this.

The second step was to manually set the content-disposition for the request to 'inline' in the pdf viewer before it is processed, around at this point: https://searchfox.org/mozilla-central/rev/1e7f7235cf822e79cd79ba9e200329ede3d37925/toolkit/components/pdfjs/content/PdfStreamConverter.jsm#1331

This two steps make a link to a pdf open remotely directly without a download occuring. I'm not entirely sure why it works though. It also fixes bug 1719895.

However, while it works for most pdfs, it does not work for when you print a google sheet page. This is sent as a pdf inside of another parent document frame. I'm not sure if it being in a frame is the issue or not as a simple test shows that a pdf loaded in a frame does display correctly. The page is sent with caching headers set to not cache or store it, which might also be relevant.

Anyway, that is the progress I have made and am not sure if I can proceed further. It is difficult to understand the code flow when urls are loaded in both parent and child processes and multiple listeners are involved.

The pdf seems to be handled only in the parent and content process when the action is set to 'internal'. Otherwise, it is downloaded only in the parent.

I discovered that the child process mime handler is asking the parent process for the pdf-related handler info but is not copying the action over correctly. When I fix that, I can get the right handler info and display most remote pdf files.

An update about progress here. There are several not particularly related fixes needed:

  1. The fix to copy the action over to the mime handler in the child process.
  2. Changing the content disposition to inline in the pdf stream converter works for some pdf files. However, the http channel (at https://searchfox.org/mozilla-central/rev/6c8d325e61b0b445ed2e04899da38c3a4c266cba/netwerk/protocol/http/HttpBaseChannel.cpp#656) ignores that you've set it to inline, so it doesn't work in some tests. I'm not sure what exactly to do here. One option is to just add an extra value that means use the header value, and have a separate inline value.
  3. There are a number of tests that expect pdfs to download and open in a new tab. I changed them to use svgs instead since I don't think it affects what is actually being tested.
  4. Still not sure why printing (or exporting) a google sheet/document needs to be special cased.

I also can't seem to verify the telemetry for 'pdf.viewer.is_attachment' to work in a test. This is set in the content process, and only one other test (browser_creditCard_telemetry.js) does this so not sure if there is something not working there or not. I verified that it is set manually however.

Attachment #9250418 - Attachment description: WIP: Bug 1719892, copy mime handler action over to child process properly → Bug 1719892, copy mime handler action over to child process properly, r=nika
Attachment #9250419 - Attachment description: WIP: Bug 1719892, add a default value for content disposition, so that it can be explicitly set to inline → Bug 1719892, add a default value for content disposition, so that it can be explicitly set to inline, r=kershaw
Attachment #9250420 - Attachment description: WIP: Bug 1719892, have some download tests use svg instead of pdf as pdf will open the pdf viewer instead of downloading → Bug 1719892, have some download tests use svg instead of pdf as otherwise the pdf viewer will open instead of downloading, r=mtigley
Attachment #9250421 - Attachment description: WIP: Bug 1719892, when pdfs are set to open internally, open downloads or responses marked as content-disposition: attachment directly using the pdf viewer instead of downloading them locally first → Bug 1719892, when pdfs are set to open internally, open downloads or responses marked as content-disposition: attachment directly using the pdf viewer instead of downloading them locally first, r=mtigley
Pushed by neil@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/78fae9550f1b
copy mime handler action over to child process properly, r=nika
https://hg.mozilla.org/integration/autoland/rev/5a1d9f67ead2
add a default value for content disposition, so that it can be explicitly set to inline, r=kershaw
https://hg.mozilla.org/integration/autoland/rev/c7daacf05682
have some download tests use svg instead of pdf as otherwise the pdf viewer will open instead of downloading, r=mtigley
https://hg.mozilla.org/integration/autoland/rev/451328d7a3d3
when pdfs are set to open internally, open downloads or responses marked as content-disposition: attachment directly using the pdf viewer instead of downloading them locally first, r=mtigley
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dc5619b3e7e9
Fix lint failure on browser_shows_where_to_save_dialog.js. a=lint-fix CLOSED TREE
Blocks: 1742648
Regressions: 1742823
QA Whiteboard: [qa-96b-p2]

This behavior doesn't apply to Google Doc. See bug 1742648

Blocks: 1756980
Blocks: 1762868

The default behaviour here was reverted, and we now download these PDFs. If you want them to show inline, you need the about:config option browser.download.open_pdf_attachments_inline set to true.

Awesome, enabling this about:config option resolves this issue AFAIK!

(In reply to megamorphg from comment #16)

Awesome, enabling this about:config option resolves this issue AFAIK!

Correction: some PDF files do still download do not stream even with this option enabled... perhaps only binary/octet-stream type PDFs.
Test case is outlined here where I enabled the above about:config option and it still downloads instead of streaming:
https://bugzilla.mozilla.org/show_bug.cgi?id=1659008#c21

This was the normal behavior previously though anyway and the binary/octet-stream never had the Action to "Open in Firefox."

Anyway, I am happy enough having the PDF files I frequently use on educational sites not auto-download thanks to the about:config.

None of the above worked for 105.0.1 (64 bit)
I had to set browser.download.start_downloads_in_tmp_dir to true to make FF download temporary files to %TEMP% instead of my download folder. When files are downloaded to %TEMP%, I can use the builtin windows app "Disk Cleanup" to manage garbage in %TEMP%, like I do from time to time. This is also what windows suggests to run if drive gets full. It may appear like this "always download" decision was made without knowledge about this garbage management..?
Also, ideally (imo), when opening links in an app, FF could/should either pass on the URL for opening in the associated app, or delete the file after the opening app has been shut down, or even make us choose our preferred way.

(In reply to mleikvol from comment #18)

FF could/should either pass on the URL for opening in the associated app,

This doesn't generally work, and isn't on-topic for this report anyway.

or delete the file after the opening app has been shut down

If you flip the tmp folder pref, the file will be deleted when Firefox closes.

Files opened this way in private browsing are deleted when the last private window closes.

This bug is closed, please do not post further comments here - if you think there are still bugs or you have feature requests, file new tickets.

You need to log in before you can comment on or make changes to this bug.