1714583 - UNC shared folder as default download location not working on Windows 10 (because nsIFile::exists() returns false for network server names on Windows)

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Ralph]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:89.0) Gecko/20100101 Firefox/89.0

Steps to reproduce:

Use "settings->Downloads" and browse to a folder shared from my NAS. Attempt to download a file.

Actual results:

File is not saved in the shared folder, it is saved in my Windows user profile's local "Downloads" folder.

Expected results:

File should have been saved to shared location. This was last working for me as expected in Firefox 86.

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Firefox::File Handling' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[Andrei Purice]

Hey Ralph,
I tried reproducing this issue on the latest versions of Firefox Nightly 91.0a1 (2021-06-09), beta 90.0b5 and release 89.0 but the downloaded files are saved in their specified locations (in my case it was the desktop). You can also go to Settings > Downloads and check the "Always ask you where to save files" or change the destination folder.

If that doesn't work can you test the issue while in Safe Mode? You can find helpful info here : https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode .
Also a fresh new profile could help. You can find more about creating a new profile here : https://support.mozilla.org/en-US/kb/troubleshoot-and-diagnose-firefox-problems#w_6-create-a-new-firefox-profile .
If possible, you can test this issue on the nightly build as well. Download the build from : https://www.mozilla.org/en-US/firefox/nightly/all/ .

Comment 3

[Ralph]

Thanks for taking a look at the issue. To clarify, the destination folder setting generally works for me so long as the location I am saving to is on my local PC. Saving files to the desktop worked fine, as it did for you. This issue only occurs if I configure Firefox to use an SMB path like \servername\share as the download location. I'm sure that my account has access to the location because if I use either Firefox 86 or Chrome files are downloaded to my share with no problem. I did try the "Always ask you where to save files" option but even though I was able to select the share location for downloading, things still downloaded to the "Downloads" folder.

I did test the issue in Safe Mode but I still experienced the same trouble. I also have tried a new profile since for a while I had downgraded to Firefox 86(this worked to fix the issue) and I needed to create a new profile to make that work.

Comment 4

[Andrei Purice]

Thank you for trying the suggestions.
Since this issue already has a component the devs are aware of it and working on it.

Comment 5

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

It might be good to get a regression range on this if we can reproduce the issue shown in Comment 0

Comment 6

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

Would you be able to provide an attached log from about:support too?

Comment 7

[Ralph]

Attachment: about-support.json

about:support data

Comment 8

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

I suspect the file going straight to the local Downloads folder might have to do with something going wrong when the DownloadIntegration module is creating the file at the specified download directory. But I don't think we log anything if it errors out here, so it's hard to know why that is, or if that's even the case.

Gijs, do you know if IOUtils.makeDirectory here might be causing the file to fall back to the local downloads directory?

Comment 9

[:Gijs (he/him)]

(In reply to Micah [:mtigley] (she/her) from comment #8)

Apologies for the slow response here; I've been busy. :-(

I suspect the file going straight to the local Downloads folder might have to do with something going wrong when the DownloadIntegration module is creating the file at the specified download directory. But I don't think we log anything if it errors out here, so it's hard to know why that is, or if that's even the case.

Gijs, do you know if IOUtils.makeDirectory here might be causing the file to fall back to the local downloads directory?

That's plausible, but I don't know why off-hand. I filed bug 1721224 to at least log exceptions.

Ralph, we tried to reproduce this issue locally by setting up a UNC share etc., but were unsuccessful. Would you mind running mozregression to find out when exactly this broke? It should be able to narrow down the gap between Firefox 86 and 89 to only 1 day of our development cycle, or even a specific patch, by doing a binary search which will help us in figuring out what the issue is and addressing it.

My other thought here is that this may be related to the switch from OS.File to IOUtils (these are internal IO libraries). Looking at the Windows implementation, it has a specific block that appears to deal with trying to create a root folder that I suspect might be dealing with this situation, that I believe IOUtils lacks. This is specifically about trying to create a root folder that already exists. Ralph, is the shared folder you're trying to use as a default a network drive, ie is it something like "Z:\", or is it a full network path, like "\\someserver\blah" ?

Comment 10

[Ralph]

Sorry that you were not able to reproduce - hopefully there is not something strange in my setup. I suppose if what we see here doesn't help I can try to get a fresh machine together and give that a shot. Below I'm posting the tail end of the log from the run of mozregression. I think you hit the nail on the head - the commit being referenced does indeed appear to be a switch from OS.File to IOUtils. When I experience the issue I am also using a \someserver\blah style path. As a test I mapped a drive to my UNC and sure enough, I was able to download there by default with no trouble. So, I suppose that could be a workaround for me if need be. Interestingly dom/system/PathUtils.cpp has a special case for UNC paths, but unfortunately I'm not good enough in the codebase to really say whether or not that's had an effect here.

---

2021-07-19T19:08:18.249000: INFO : Narrowed integration regression window from [917f823a, 7cdfc29e] (6 builds) to [ce5e2f5f, 7cdfc29e] (3 builds) (~1 steps left)
2021-07-19T19:08:18.251000: INFO : Running autoland build built on 2021-02-04 09:15:18.544000, revision aadba769
2021-07-19T19:08:19.417000: INFO : Launching C:\Users\micro\AppData\Local\Temp\tmp_ix5z4po\firefox\firefox.exe
2021-07-19T19:08:19.417000: INFO : Application command: C:\Users\micro\AppData\Local\Temp\tmp_ix5z4po\firefox\firefox.exe --wait-for-browser -profile C:\Users\micro\AppData\Local\Temp\tmp_gudq188.mozrunner
2021-07-19T19:08:19.448000: INFO : application_buildid: 20210201041942
2021-07-19T19:08:19.448000: INFO : application_changeset: aadba76932ea3f20967a69813277a637e3bfa025
2021-07-19T19:08:19.448000: INFO : application_display_name: Firefox Nightly
2021-07-19T19:08:19.448000: INFO : application_id: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
2021-07-19T19:08:19.448000: INFO : application_name: Firefox
2021-07-19T19:08:19.448000: INFO : application_remotingname: firefox
2021-07-19T19:08:19.448000: INFO : application_repository: https://hg.mozilla.org/integration/autoland
2021-07-19T19:08:19.448000: INFO : application_vendor: Mozilla
2021-07-19T19:08:19.448000: INFO : application_version: 87.0a1
2021-07-19T19:08:19.448000: INFO : platform_buildid: 20210201041942
2021-07-19T19:08:19.448000: INFO : platform_changeset: aadba76932ea3f20967a69813277a637e3bfa025
2021-07-19T19:08:19.448000: INFO : platform_repository: https://hg.mozilla.org/integration/autoland
2021-07-19T19:08:19.448000: INFO : platform_version: 87.0a1
2021-07-19T19:08:41.592000: INFO : b'ConvertWinError received an unrecognized WinError: 0xa1'
2021-07-19T19:08:49.835000: INFO : Narrowed integration regression window from [ce5e2f5f, 7cdfc29e] (3 builds) to [ce5e2f5f, aadba769] (2 builds) (~1 steps left)
2021-07-19T19:08:49.840000: DEBUG : Starting merge handling...
2021-07-19T19:08:49.841000: DEBUG : Using url: https://hg.mozilla.org/integration/autoland/json-pushes?changeset=aadba76932ea3f20967a69813277a637e3bfa025&full=1
2021-07-19T19:08:49.841000: DEBUG : redo: attempt 1/3
2021-07-19T19:08:49.841000: DEBUG : redo: retry: calling _default_get with args: ('https://hg.mozilla.org/integration/autoland/json-pushes?changeset=aadba76932ea3f20967a69813277a637e3bfa025&full=1',), kwargs: {}, attempt #1
2021-07-19T19:08:49.842000: DEBUG : urllib3.connectionpool: Resetting dropped connection: hg.mozilla.org
2021-07-19T19:08:50.671000: DEBUG : urllib3.connectionpool: https://hg.mozilla.org:443 "GET /integration/autoland/json-pushes?changeset=aadba76932ea3f20967a69813277a637e3bfa025&full=1 HTTP/1.1" 200 None
2021-07-19T19:08:50.749000: DEBUG : Found commit message:
Bug 1649611: Remove OS.File usage from DownloadIntegration.jsm and respect umask in IOUtils::SetPermissions r=barret

Depends on D103520

Differential Revision: https://phabricator.services.mozilla.com/D99729

2021-07-19T19:08:50.749000: DEBUG : Did not find a branch, checking all integration branches
2021-07-19T19:08:50.749000: INFO : The bisection is done.
2021-07-19T19:08:50.749000: INFO : Stopped

Comment 11

[:Gijs (he/him)]

Thanks very much for the bisection, and additional details, Ralph! That really helped.

I was able to reproduce by sharing from my mac to a Windows machine. When adding the patch from bug 1721224, logging says "Could not create directory at \\SERVER\folder because the path has missing ancestor components." It appears that the problem is that our even-older file handling framework code in xpcom/io returns false if you ask it if \\\\SERVER exists (and I expected this would already be on file, but I didn't find any related bugs...). It returns the correct result (ie true) if you ask about \\\\SERVER\\folder. So if you use a subfolder of a shared folder as the downloads target, it also works...

Moving to XPCOM because the "right" fix is presumably to make nsIFile::Exists not lie when asked about server paths.

Barret, pinging you to check if, based on this, and comment #9, you think we need a separate/additional patch to IOUtils itself...

Comment 12

[:Gijs (he/him)]

Root cause inside XPCOM is that Exists() calls ResolveAndStat() which calls GetFileInfo which calls the Windows API GetFileAttributes, which promptly returns an error, which we don't understand.

It's even documented on MSDN that this will happen, though:

If you call GetFileAttributes for a network share, the function fails, and GetLastError returns ERROR_BAD_NETPATH. You must specify a path to a subfolder on that share.

Which is all well and good but doesn't really help in terms of saying what you should do to check if the server name is valid.

The OS.File code in makeDir does not bother checking if the parent dir of the dir it's trying to create exists, which is why this worked before. It directly called the Windows CreateDirectory API which does not create ancestor directories if necessary.

The IOUtils code is platform-agnostic and relies on xpcom/io code to do it works, and that abstraction layer always creates any necessary intermediate directories - but the downloads code explicitly specifies it does not want that behaviour. To accommodate this, the IOUtils layer does an explicit "does the parent exist?" check, which fails here.

I suspect the best solution is pushing the behaviour we want (ie not creating ancestors) down into the XPCOM layer.

Comment 13

[:Gijs (he/him)]

I have a WIP patch, just need to check it compiles on Linux/macOS etc.

https://treeherder.mozilla.org/jobs?repo=try&revision=2365709d75affac56e0ac39a41f1d5af2c66ecd9

Comment 14

[:Gijs (he/him)]

Attachment: Bug 1714583 - allow nsIFile::Create to skip creating ancestor directories, to fix IOUtils::makeDirectory, to fix UNC default download folders, r?barret!,nika!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/097173dd4f93
allow nsIFile::Create to skip creating ancestor directories, to fix IOUtils::makeDirectory, to fix UNC default download folders, r=barret,xpcom-reviewers,mccr8

Comment 16

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/097173dd4f93

Comment 17

[:Gijs (he/him)]

Hey Ralph, I believe this should now be fixed in our nightly builds ( https://nightly.mozilla.org/ ). Could you confirm if it works for you with that build? Thanks!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

I could see this affecting enterprise users, so leaving this on the radar for a possible ESR91 uplift next cycle.

Comment 19

[Ralph]

I finally had a chance to test the nightly and it appears this patch resolved the issue. I'd like to thank everyone who worked this bug and to just generally express how much of a fan I am of Firefox - it's a fantastic accomplishment.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Speaking of which, shall we uplift this to ESR91, Gijs? :)

Comment 21

[:Gijs (he/him)]

Comment on attachment 9232147 [details]
Bug 1714583 - allow nsIFile::Create to skip creating ancestor directories, to fix IOUtils::makeDirectory, to fix UNC default download folders, r?barret!,nika!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Broken behaviour with download folders that are immediate children of a UNC share; probably more likely to affect ESR users as they'll be more likely to have network shares
• User impact if declined: See above
• Fix Landed on Version: 92
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Low-ish; Though this involves XPCOM API changes (is that OK on esr?), this has shipped with 92 now and we haven't heard of ill effects.
• String or UUID changes made by this patch: Nope

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9232147 [details]
Bug 1714583 - allow nsIFile::Create to skip creating ancestor directories, to fix IOUtils::makeDirectory, to fix UNC default download folders, r?barret!,nika!

Approved for 91.1esr.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/e3a57c25bbdf