1741431 - Context menu "Save Link As..." for plain text files opens the selected helper application with browser.download.improvements_to_download_panel=true

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: handlers.json

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:96.0) Gecko/20100101 Firefox/96.0 ID:20211115093917

Similar to bug 1738129 but for normal plain text files. Selecting Save link as... for such a file doesn't only save the file to disk, but at the same time also opens the selected helper application.

Steps to reproduce:

1. Create a new profile and close Firefox
2. Save the attachment (handlers.json) to this folder and tweak the helper application for plain/text to mach a locally installed application
3. Start Firefox and navigate to Treeherder
4. Select the Artifacts and Debugging Tools pane at the bottom of the page
5. Right click live_backing.log and select Save link as...
6. Save the file

With steps 6 the helper application as selected in the handler.json file gets inappropriately opened. The only action to do here is to save the file, and nothing more.

Comment 1

[:Gijs (he/him)]

The handlers.json file is set to open with Sublime Text, but also to ask first. We shouldn't be ignoring the "ask" part (though soon it'll be hard to end up in this situation, and we'll migrate existing cases to just save to disk by default.

Comment 2

[kpatenio]

I can also reproduce this with a brand new profile without copying the attached handlers.json.

I did the following:

1. Create a new profile and open it in new browser window
2. Downloaded a text file (ex. here)
3. Selected the Always Open Similar Files to ensure Text File content type exists in pref table in about:preferences
4. Went to about:preferences and ensured Text File content type exists in table (otherwise, refresh page)
5. Changed Action to Sublime for text file via Use other
6. Went to Treeherder link and followed steps 4-6

As a result, the download resumes and sublime opens the file. I took a look at DownloadLegacy.jsm where launchWhenSucceeded is being set, and it seems that when the pref browser.download.improvements_to_download_panel is true, the preferredAction is being read as useHelperApp instead of saveToDisk. Thus, launchWhenSucceeded is true and results in sublime opening the file.

When the pref is disabled, the preferredAction is properly set to saveToDisk, hence why Sublime never launches.

Comment 3

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

(In reply to kpatenio from comment #2)

When the pref is disabled, the preferredAction is properly set to saveToDisk, hence why Sublime never launches.

This might be where the preferredAction action is being set when the pref is disabled: https://searchfox.org/mozilla-central/rev/3881c4ca80d1b4b2f43be695438ecaf90ee4f86c/uriloader/exthandler/nsExternalHelperAppService.cpp#2668-2670. Perhaps we need to set the preferredAction to save to disk if mForceSave is true?

EDIT
Although it seems mForceSave will always be true when saving through the context menu. Perhaps we can just remove the check altogether.

Comment 4

[:Gijs (he/him)]

(In reply to kpatenio from comment #2)

I can also reproduce this with a brand new profile without copying the attached handlers.json.

I did the following:

1. Create a new profile and open it in new browser window
2. Downloaded a text file (ex. here)
3. Selected the Always Open Similar Files to ensure Text File content type exists in pref table in about:preferences
4. Went to about:preferences and ensured Text File content type exists in table (otherwise, refresh page)
5. Changed Action to Sublime for text file via Use other

I'm confused - these steps mean that you're actually telling Firefox "please open the file with sublime text", and then we do that. That sounds like expected behaviour to me. The issue in comment 0 is that the action is set to "always ask", not "always open this file with sublime text". Am I missing something?

Comment 5

[kpatenio]

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

I'm confused - these steps mean that you're actually telling Firefox "please open the file with sublime text", and then we do that. That sounds like expected behaviour to me. The issue in comment 0 is that the action is set to "always ask", not "always open this file with sublime text". Am I missing something?

Looking back, I completely misunderstood your first comment and the expected downloads behaviour when the pref is enabled. Opening with Sublime after saving a file makes sense here with the pref enabled. Sorry about that!

The handlers.json file is set to open with Sublime Text, but also to ask first. We shouldn't be ignoring the "ask" part (though soon it'll be hard to > end up in this situation, and we'll migrate existing cases to just save to disk by default.

1. Given that ask in handlers.json is true, should we then be expecting the UCT window to appear after selecting Save Link As... ? I'm aware that we open the file with any default or external apps after saving a file, but I'm not sure what the case is when we choose alwaysAsk in the preferences table. It seems we intentionally set alwaysAsk to false whenever we try to save a file.
2. From your comment, to me it sounds like we're expecting the migration from Bug 1736924 to fix the situation where action is set to open with Sublime but ask is true. Am I understanding correctly?

Comment 6

[:Gijs (he/him)]

(In reply to kpatenio from comment #5)

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

I'm confused - these steps mean that you're actually telling Firefox "please open the file with sublime text", and then we do that. That sounds like expected behaviour to me. The issue in comment 0 is that the action is set to "always ask", not "always open this file with sublime text". Am I missing something?

Looking back, I completely misunderstood your first comment and the expected downloads behaviour when the pref is enabled. Opening with Sublime after saving a file makes sense here with the pref enabled. Sorry about that!

No worries!

1. Given that ask in handlers.json is true, should we then be expecting the UCT window to appear after selecting Save Link As... ?

No, fair point, we should not, because the user explicitly picked "save link as". I think in Firefox release/beta we also don't show the dialog in this case (would be good to doublecheck just to be safe, but I'm like 90% sure...).

I'm aware that we open the file with any default or external apps after saving a file, but I'm not sure what the case is when we choose alwaysAsk in the preferences table. It seems we intentionally set alwaysAsk to false whenever we try to save a file.

Right, but we also set the action to saveToDisk (line 1944), so why do we still end up opening the file?

2. From your comment, to me it sounds like we're expecting the migration from Bug 1736924 to fix the situation where action is set to open with Sublime but ask is true. Am I understanding correctly?

Yes... however ;-)
As you noted in the test in that bug, in some cases we default to the useHelperApp value. So I think it might still be valuable to ensure that we're doing the right thing if the state of the preferences has changed. Also, even after the migration it will be possible for users to set Firefox to "always ask", and then to configure a helper app in the dialog, after which AIUI the preferences state would be the same as the one in comment 0. So that'd be these steps (I haven't tested):

1. clean profile on current nightly
2. download a text/plain txt file
3. use the context menu to "always open similar files" to create an entry in about:preferences for text files
4. change the preference for text files to "always ask"
5. find a text file sent with Content-Disposition: attachment. This will show the dialog (I think - because preferredAction will be alwaysAsk at this point) - https://mime.ty.ax/ might be helpful.
6. choose to open with a helper app and find sublime text
7. accept the dialog
8. use "save link as" per comment 0

I think that'd produce this same bug? Or something close to these steps, anyway.

Comment 7

[kpatenio]

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

I can confirm that the example you noted works. Thanks!

Right, but we also set the action to saveToDisk (line 1944), so why do we still end up opening the file?

I'm curious about this too and would like to look into it. I assumed that the action is getting overwritten, but after talking with @mtigley, it's possible that the action itself never updates to saveToDisk.

Comment 8

[kpatenio]

Took some time to set up, but I finally managed to debug nsExternalHelperAppService.cpp, and I can confirm that action does in fact get set to save and ask gets eventually set to false (confirming that the UCT window should never appear when selecting "Save Link As...").

But mimeInfo is different once we hit DownloadIntegration. (I mentioned DownloadLegacy earlier, but I think DownloadIntegration is the right place to look at). We read mimeInfo using getFromTypeAndExtension, and from there we get preferredAction = 2. So it's possible that the action either gets overwritten, or the update is never transferred (?).

@gijs, given this information, do you have an idea why or where this could be happening? I'm not sure what happens in between going from nsExternalHelperAppService to DownloadIntegration, and it would help to know what other files I should look at. The earliest I could look back at in my JS debugger was HelperAppDlg.jsm.

Comment 9

[:Gijs (he/him)]

(In reply to kpatenio from comment #8)

Took some time to set up, but I finally managed to debug nsExternalHelperAppService.cpp, and I can confirm that action does in fact get set to save and ask gets eventually set to false (confirming that the UCT window should never appear when selecting "Save Link As...").

OK, so this bit sounds like everything works as expected.

But mimeInfo is different once we hit DownloadIntegration. (I mentioned DownloadLegacy earlier, but I think DownloadIntegration is the right place to look at). We read mimeInfo using getFromTypeAndExtension, and from there we get preferredAction = 2. So it's possible that the action either gets overwritten, or the update is never transferred (?).

We don't (and don't want to) write changes to the handler service (HandlerService.js) when downloading and evaluating user preferences in the external helper app service - we're just reading the contents of those preferences and using them to make a decision. I think the external helper app service gets the same info as the DownloadIntegration code, but it overwrites the default action to "save to disk" because from the "save link as" code we'll pass true for the forceSave/aForceSave parameter.

The code you linked in DownloadIntegration will only run if something calls launchDownload. That in turn only happens if something calls download.launch which I think in this case would only happen if something set launchWhenSucceeded to true: https://searchfox.org/mozilla-central/rev/633345116df55e2d37be9be6555aa739656c5a7d/toolkit/components/downloads/DownloadCore.jsm#598 .

So I think the next step would be checking if my guess above is right, and if so, what's setting that to true. I expect the "force a save" information is not propagating from the C++ code to the JS code but I don't know why that normally works and doesn't work here, off-hand.

If something's unclear or I missed something please do ask more questions here or ping me on matrix/slack etc. :-)

Comment 10

[kpatenio]

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

So I think the next step would be checking if my guess above is right, and if so, what's setting that to true. I expect the "force a save" information is not propagating from the C++ code to the JS code but I don't know why that normally works and doesn't work here, off-hand.

If something's unclear or I missed something please do ask more questions here or ping me on matrix/slack etc. :-)

Thanks! So yes, I can confirm that's correct. I took a look once again and here is my analysis:

1. We select "Save Link As..."
2. nsExternalAppHandler::OnStartRequest() starts and we update action
3. We call PromptForSaveDestination()
4. However, we never actually update the preferredAction to saveToDisk, unless the improvements pref is disabled.
5. We hit PromptForSaveToFileAsync
6. File picker prompt happens. Continuing from HelperAppDlg.jsm up until DownloadIntegration, we would've continued to read the original preferredAction that was never updated to saveToDisk.
7. We press save in the dialog. This leads to ContinueSave, where we read the preferredAction there. Again up to this point, we never updated to saveToDisk.
8. We finally proceed with transfer.

If we really want to update the behaviour such that we only ever download the file after Save Link As..., we will need to set the preferredAction to saveToDisk somewhere before we actually download the file. Perhaps we could do something like this?

// PromptForSaveDestination
  if (!StaticPrefs::browser_download_improvements_to_download_panel() || mForceSave) {
    mMimeInfo->SetPreferredAction(nsIMIMEInfo::saveToDisk);
  }

Does this seem right?

Comment 11

[:Gijs (he/him)]

(In reply to kpatenio from comment #10)

If we really want to update the behaviour such that we only ever download the file after Save Link As...

To be clear, I think this was/is the behaviour on release/beta right? We don't offer the user even an option to always open, and even if the user has configured filetype X to always open, if they right click a link to an X and pick "save link as...", I don't think we honour that, ever - we just save the file.

, we will need to set the preferredAction to saveToDisk somewhere before we actually download the file. Perhaps we could do something like this?

// PromptForSaveDestination
  if (!StaticPrefs::browser_download_improvements_to_download_panel() || mForceSave) {
    mMimeInfo->SetPreferredAction(nsIMIMEInfo::saveToDisk);
  }

Does this seem right?

Yes, that seems very much right. We'll just need an automated test then (and check doing that doesn't break any other tests, I guess). :-)

Comment 12

[kpatenio]

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

To be clear, I think this was/is the behaviour on release/beta right? We don't offer the user even an option to always open, and even if the user has configured filetype X to always open, if they right click a link to an X and pick "save link as...", I don't think we honour that, ever - we just save the file.

I can confirm that the current behaviour in release is that we save the file after selecting "Save Link As..." (considering that the pref isn't enabled in the first place anyways).

, we will need to set the preferredAction to saveToDisk somewhere before we actually download the file. Perhaps we could do something like this?

// PromptForSaveDestination
  if (!StaticPrefs::browser_download_improvements_to_download_panel() || mForceSave) {
    mMimeInfo->SetPreferredAction(nsIMIMEInfo::saveToDisk);
  }

Does this seem right?

Yes, that seems very much right. We'll just need an automated test then (and check doing that doesn't break any other tests, I guess). :-)

Thanks for verifying that. I'll work on it then :)

Comment 13

[kpatenio]

Attachment: Bug 1741431 - fix Save Link As... for plain text files when download improvements pref is enabled

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c703e6d35d50
fix Save Link As... for plain text files when download improvements pref is enabled r=mtigley

Comment 17

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/c703e6d35d50