1477986 - "browser.helperApps.neverAsk.saveToDisk" doesn't work with "browser.download.dir" or "browser.download.folderList" (due to "\n" chars between prefs in user.js file))

Status: RESOLVED INVALID

(Core :: Preferences: Backend, defect)

Description

[paulmdavies]

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180710113641

Steps to reproduce:

1. Open firefox 60.0 with the following profile:

"browser.download.dir" => <any directory>
"browser.download.folderList" => 2
"browser.helperApps.neverAsk.saveToDisk" => "text/csv"

2. Enter "https://www.sample-videos.com/csv/Sample-Spreadsheet-10-rows.csv" in the address bar and press Enter.

Extra notes:
- This problem does not affect previous versions of Firefox (tested with 59.0.3) but does affect all from 60.0 onwards
- Setting just "browser.helperApps.neverAsk.saveToDisk" has the desired effect, but this setting doesn't seem to work in conjunction with "browser.download.dir" or "browser.download.folderList".


Actual results:

A download location popup appears, despite the fact that this was disabled in step 1 for csv files.


Expected results:

The file should be downloaded to the specified directory without a location popup appearing.

Comment 1

[Gingerbread Man]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
20180704003137

Works for me.

(In reply to paulmdavies from comment #0)
> Steps to reproduce:

Did you reproduce the issue with those steps in a brand new profile?
You don't have CSV files set to Always Ask in about:preferences?

> - This problem does not affect previous versions of Firefox (tested with
> 59.0.3) but does affect all from 60.0 onwards

The exact regression range would be helpful.
https://mozilla.github.io/mozregression/quickstart.html

Comment 2

[paulmdavies]

(In reply to Gingerbream Man from comment #1)
> Did you reproduce the issue with those steps in a brand new profile?

Yes, the profile is freshly generated before opening Firefox with only these properties set.

> You don't have CSV files set to Always Ask in about:preferences?

Not unless that's the default, as the profile is fresh.
 
> The exact regression range would be helpful.

I hoped that as 59.0.3 and 60.0 are consecutive builds, that would be enough. I'll see if I can run the mozregression tool tomorrow.

Comment 3

[paulmdavies]

Some more details, which I should probably have included to start with:

1. Firefox is running inside a CentOS 7 VM, so the User Agent string in my report isn't the one of the machine exhibiting the problem.
2. The problem has been reproduced across multiple CentOS 7 machines.
3. I investigated the combinations of settings for "browser.helperApps.neverAsk.saveToDisk" (b.h.n.s), "browser.download.folderList" (b.d.f) and 
"browser.download.dir" (b.d.d), with a fresh profile each time, in Firefox 59.0.3 and 60.0, with no other changes. The behaviour was as described in the following truth table:

| "b.h.n.s" set | "b.d.f" set | "b.d.d" set | popup appears (59.0.3) | popup appears (60.0) | discrepancy |
|             Y |           Y |           Y |                      N |                    Y |           Y |
|             Y |           Y |           N |                      N |                    Y |           Y |
|             Y |           N |           Y |                      N |                    Y |           Y |
|             Y |           N |           N |                      N |                    N |           N |
|             N |           Y |           Y |                      Y |                    Y |           N |
|             N |           Y |           N |                      Y |                    Y |           N |
|             N |           N |           Y |                      Y |                    Y |           N |
|             N |           N |           N |                      Y |                    Y |           N |

Comment 4

[paulmdavies]

I've run the mozregression tool, and it came out with the following:

INFO: No more inbound revisions, bisection finished.
INFO: Last good revision: fc36a210edd7e44ecb0e7e0c60ea3226d438ab17
INFO: First bad revision: e8b798a5205ad27d067b91210efb46df7ddab45d
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fc36a210edd7e44ecb0e7e0c60ea3226d438ab17&tochange=e8b798a5205ad27d067b91210efb46df7ddab45d

Considering that changeset e8b798a5205a	has the comment 'Rewrite the prefs parser', I think this is probably the source of the problem.

Comment 5

[paulmdavies]

I've uncovered the root cause here. The contents of my profile file is as follows:

user_pref("browser.download.dir", "/tmp/tmpesIUnd"); \nuser_pref("browser.download.folderList", 2); \nuser_pref("browser.helperApps.neverAsk.saveToDisk", "text/plain,text/csv");

If I manually edit the file, replacing the "\n" characters with new lines, the behaviour is as expected.

The author of change e8b798a5205a says 'The new parser is slightly stricter than the old parser in a few ways' - I don't know whether this rejection of "\n" is an expected change? I'm happy to accept if so!

Comment 6

[Nicholas Nethercote [inactive]]

Paul, you are right about the "\n" sequences. From the commit message (https://hg.mozilla.org/integration/mozilla-inbound/rev/e8b798a5205a):

> Disconcertingly, the old parser allowed arbitrary junk between prefs
> (including at the start and end of the prefs file) so long as that junk
> didn't include any of the following chars: '/', '#', 'u', 's', 'p'. I.e.
> lines like these:
>
>     !foo@bar&pref("prefname", true);
>     ticky_pref("prefname", true); // missing 's' at start
>     User_pref("prefname", true); // should be 'u' at start
>   
> would all be treated the same as this:
>
>     pref("prefname", true);
>   
> The new parser disallows such junk because it isn't necessary and seems like 
> an unintentional botch by the old parser.

These "\n" sequences fall into this category. Replacing them with newlines, as you have done, is the right thing to do. Thank you for bisecting and investigating what went wrong.

I will close this bug because the new parser is working as intended.

Comment 7

[paulmdavies]

Thanks, Nicholas. Sorry for erroneously reporting a bug.

Is it worth changing the title of the bug to something like "\n not accepted in profile" so that other with this problem might find this?

Comment 8

[Nicholas Nethercote [inactive]]

It can't hurt. I've updated the title accordingly.