1851968 - PDF opened in Firefox not added to tab history with browser.download.open_pdf_attachments_inline to true

Status: NEW

(Core :: DOM: Navigation, defect, P3)

Description

[jscher2000]

Firefox 103 (bug 1772569) introduced the browser.download.open_pdf_attachments_inline preference to allow overriding Content-Disposition: attachment to inline handling. However, there is a peculiarity:

When Firefox opens the PDF in a new tab, the PDF is somehow not added to the tab history. Therefore, if the user navigates in that tab using a link or bookmark (intentionally or by accident), it is not possible to go Back to the PDF. This can make it difficult to return to the PDF (as it may have been opened days or weeks earlier, and is in browsing history rather than downloads).

This page illustrates the problem: https://www.jeffersonscher.com/res/attach-to-inline.html

Ideally, the PDF should be added to tab history like any other PDF.

Comment 1

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:Gijs, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[:Gijs (back August 25; he/him)]

Nika or Calixte, any idea what's going on here?

Comment 3

[Calixte Denizet (:calixte)]

I'm inclined to think that it doesn't happen on the pdf side, but probably before we get the stream.
I don't really see anything in:
https://searchfox.org/mozilla-central/rev/077fc34d03b85b09add26b5f99f1a3a3a72c8720/toolkit/components/pdfjs/content/PdfStreamConverter.sys.mjs#995
which could indicate that we make a difference between attachment vs not attachment.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

The situation where the document doesn't properly enter history appears to be the one which I added in bug 1756980. I think that when I added the logic to switch the load into a new window in response to the document turning out to be an attachment, I didn't correctly hook up session history for the newly loaded document.

If we want to keep this functionality working, we probably need to change the logic which we do after a successful SwitchToNewTab call (https://searchfox.org/mozilla-central/rev/49d63b61acbc86a8eccc07e53cedc9ba82cf8c2a/netwerk/ipc/DocumentLoadListener.cpp#1942-1945), and somehow switch out which history object we're committing the changes to, as the current logic always assumes the session history the load started with is the same as the one it'll end with. I'm not 100% sure what that would involve, but it would likely require us to re-create things like the LoadingSessionHistoryInfo with the correct BrowsingContext (https://searchfox.org/mozilla-central/rev/49d63b61acbc86a8eccc07e53cedc9ba82cf8c2a/netwerk/ipc/DocumentLoadListener.cpp#722-730), and/or other things like that.

Given that this is disabled by default right now, is "opening an attachment in a new tab" a feature which we still depend on and want to support? It's a solid amount of complexity, and fairly unique, so if we can drop support for it that might be simpler than trying to fix up session history to support it. If we want to keep it working though, ni? peterv who might know more about the session history side.

Comment 5

[:Gijs (back August 25; he/him)]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #4)

Given that this is disabled by default right now, is "opening an attachment in a new tab" a feature which we still depend on and want to support?

"It's complicated."

Honestly, ISTM that different people want pretty different things from the file handling features, depending on how the websites they use regularly serve them pdf/webp/whatever files ("the website is wrong and Firefox should 'just' do what I want" vs "the website is right, why is Firefox overriding its specified intent"). We just added yet another about:config option that goes the other way (ie allows you to set up Firefox such that inline PDFs are shown inline, and ones with CD: attachment and/or download attributes cause a download). (bug 1811830 for reference)

I expect the "last word" on what is "right" here has not yet been said. Unfortunately, I don't believe we currently have telemetry for any of these features (and inasmuch as they also get used by enterprises, telemetry being turned off by them may mask usage), and as a result it's very hard to judge how important the different usecases are. Basing it on how much people complain on bugzilla is... a questionable methodology. Speaking of which - @jscher, is this something you run into yourself, or did you report on behalf of someone on SUMO or similar?

It's a solid amount of complexity, and fairly unique, so if we can drop support for it that might be simpler than trying to fix up session history to support it. If we want to keep it working though, ni? peterv who might know more about the session history side.

I mean, if it takes half a year to fix this I'd argue not to prioritize it right now.

It does make me wonder, is there an alternative way to implement this that wouldn't make the setup so "unique"? In my head, it feels like there would be other usecases (e.g. what happens to loads where the main page redirects to an external protocol handler? Cf. bug 590993 attempted to change behaviour for loads originating directly in pinned tabs (actually unsure off-hand if it would work for redirects), with the question of what to do in other tabs left for another day...) -- but perhaps I'm wrong and most of the time we know what tab/BC we want and don't need to retarget the load once we've already got headers?

(the other thing that makes me think that is that the implementation for 1811830 mentioned above decided to add the feature in a different part of the codepath, not in the same place where we do the inline handling... which was somewhat surprising to me.)

Comment 6

[Nika Layzell [:nika] (ni? for response)]

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

I mean, if it takes half a year to fix this I'd argue not to prioritize it right now.

I somewhat doubt it'd take half a year to fix this, but I don't quite know how long it would take. I've unfortunately not got this part of the new session history in my head at the moment (hence the ni? for :peterv). If it's fairly straightforward we can fix it, though I wouldn't be too surprised if we find more quirks in the future :-).

It does make me wonder, is there an alternative way to implement this that wouldn't make the setup so "unique"? In my head, it feels like there would be other usecases (e.g. what happens to loads where the main page redirects to an external protocol handler? Cf. bug 590993 attempted to change behaviour for loads originating directly in pinned tabs (actually unsure off-hand if it would work for redirects), with the question of what to do in other tabs left for another day...) -- but perhaps I'm wrong and most of the time we know what tab/BC we want and don't need to retarget the load once we've already got headers?

Something like external protocol handler redirects I believe are handled a bit differently. Unlike in this case, where we need to preserve the active channel in it's entirety (incl. headers, potentially a partial response, redirect history etc.), with an web handler app we instead handle the request by starting a completely new navigation. You can see that in the WebHandlerApp handler here: https://searchfox.org/mozilla-central/rev/95ec620fda8ad6e4096cac1f1a9db8653669b31d/uriloader/exthandler/WebHandlerApp.sys.mjs#130,167-173. The load in this case is even triggered with the system principal :-).

Something like that is fairly easy to retarget, as it's a brand-new load and retargeting a load before it's started is straightforward, but retargeting a load when it's partially complete is (to my knowledge) something new, and my approach to it apparently didn't handle session history properly.

Comment 7

[jscher2000]

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

Speaking of which - @jscher, is this something you run into yourself, or did you report on behalf of someone on SUMO or similar?

I started noticing it in early September, possibly because I was following more links in PDFs in Firefox and forgetting to direct them to open in a new tab. I have no idea whether anyone else has noticed it.

What is strange to me about this behavior is that it seems unique to browser.download.open_pdf_attachments_inline being set to true.

What do you think about the possibility of allowing the attachment => inline PDF to open in the same tab -- that is, following the same rules for PDFs served with Content-Disposition: inline in the first place? I know for downloads saved to disk, users were upset about Firefox 98 navigating their tab to the file:/// URL of the download, and that was changed to launch the download in a new tab a version or two later. But for users preferring inline disposition, is that necessary/beneficial?

Comment 8

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Please feel free to let me know if this should be a higher priority.

Comment 9

[Pier Angelo Vendrame]

I noticed that also other features are broken as a result of this Bug (e.g., reloading).

(In reply to jscher2000 from comment #7)

What do you think about the possibility of allowing the attachment => inline PDF to open in the same tab -- that is, following the same rules for PDFs served with Content-Disposition: inline in the first place? I know for downloads saved to disk, users were upset about Firefox 98 navigating their tab to the file:/// URL of the download, and that was changed to launch the download in a new tab a version or two later. But for users preferring inline disposition, is that necessary/beneficial?

I'd be very interested in this.
I'm extending browser.download.open_pdf_attachments_inline to other mime types to avoid disk leaks in Tor Browser when the automatic download happens, and I noticed the UX seems kinda strange (new tab also when you were coming from about:blank, lacking history and refresh 😅).

In case anyone needs a server that forces Content-disposition: attachment (adapted from this SO answer):

#!/usr/bin/env python3
from http import server

class MyHTTPRequestHandler(server.SimpleHTTPRequestHandler):
    def end_headers(self):
        self.send_my_headers()

        server.SimpleHTTPRequestHandler.end_headers(self)

    def send_my_headers(self):
        if self.path != "/":
            self.send_header("Content-disposition", "attachment")


if __name__ == "__main__":
    server.test(HandlerClass=MyHTTPRequestHandler)

Maybe I will investigate a little bit about this Bug (but I don't promise anything) 😄.

Comment 10

[:Gijs (back August 25; he/him)]

(In reply to Pier Angelo Vendrame from comment #9)

I noticed that also other features are broken as a result of this Bug (e.g., reloading).

This isn't really enough information to do anything. Can you file a new bug with more specifics? It may want to depend on this bug as I expect that fixing session restore might fix reloads, depending on if/how they're broken (not super clear to me off-hand).

Comment 11

[Pier Angelo Vendrame]

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

(In reply to Pier Angelo Vendrame from comment #9)

I noticed that also other features are broken as a result of this Bug (e.g., reloading).

This isn't really enough information to do anything. Can you file a new bug with more specifics? It may want to depend on this bug as I expect that fixing session restore might fix reloads, depending on if/how they're broken (not super clear to me off-hand).

Sorry, I meant I found that reloading doesn't work, but I'm not sure it's the only broken functionality.
Anyway, I've opened Bug 1917085 for it.
I also opened Bug 1917088 for the attachments always opened in new tabs (even when you open them from an already new tab), but I wasn't sure how related it is to this one.