1731049 - Downloading files onto mapped network drives fails

Status: VERIFIED FIXED

(Core :: XPCOM, defect, P1)

Description

[Lukas]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.159 Safari/537.36 OPR/78.0.4093.184

Steps to reproduce:

1. Open Firefox as RemoteApp over RDP
   1a. Tested with:

• Local system: Windows 10, connection through mstsc
• Remote system: Windows Server 2019, Windows 10

2. Download a file
3. Save in a folder on a mapped drive from the local machine

Actual results:

The file appears to be downloading, but fails after the progress bar is finished. A file does appear in the selected folder, but it has a size of 0 bytes.

Expected results:

The downloaded file is successfully saved into the selected folder. This worked well up until version 82.0.3. The problem first occurred in version 83 and is still present in version 90.

Comment 1

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

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

Comment 2

[Lukas]

Description edit: Tested up until Firefox version 92 instead of 90. The bug was still present.

Comment 3

[Kershaw Chang [:kershaw]]

Hi Lukas,

Could you try to use mozregresssion to find out which patch between 82 and 83 that causes this issue?

Thanks.

Comment 4

[Lukas]

Hi Kershaw,

According to mozregression:

Depends on D90639
Differential Revision: https://phabricator.services.mozilla.com/D90640

There were some versions during the regression in which it would work one time, and fail another time. This occurred both when downloading the same file several times, and when downloading several different files. I downloaded the same set of files for each iteration of the regression.

Comment 5

[Lukas]

app_name: firefox
build_date: 2020-09-28
build_file: C:\Users\Lukas\.mozilla\mozregression\persist\2020-09-28--mozilla-central--firefox-83.0a1.en-US.win32.zip
build_type: nightly
build_url: https://archive.mozilla.org/pub/firefox/nightly/2020/09/2020-09-28-21-28-24-mozilla-central/firefox-83.0a1.en-US.win32.zip
changeset: b3ea886b0ae7c2f056fead164116774dab00239b
pushlog_url: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2c3121f695b6b40944b93fdd3bc20d6b94991828&tochange=b3ea886b0ae7c2f056fead164116774dab00239b
repo_name: mozilla-central
repo_url: https://hg.mozilla.org/mozilla-central

Comment 6

[Kershaw Chang [:kershaw]]

(In reply to Lukas from comment #5)

app_name: firefox
build_date: 2020-09-28
build_file: C:\Users\Lukas\.mozilla\mozregression\persist\2020-09-28--mozilla-central--firefox-83.0a1.en-US.win32.zip
build_type: nightly
build_url: https://archive.mozilla.org/pub/firefox/nightly/2020/09/2020-09-28-21-28-24-mozilla-central/firefox-83.0a1.en-US.win32.zip
changeset: b3ea886b0ae7c2f056fead164116774dab00239b
pushlog_url: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2c3121f695b6b40944b93fdd3bc20d6b94991828&tochange=b3ea886b0ae7c2f056fead164116774dab00239b
repo_name: mozilla-central
repo_url: https://hg.mozilla.org/mozilla-central

Thanks for the information, but it's still unclear to me which patch causes this issue.

Since there is no networking related patches in the push log, I'd like to switch the component to Downloads API and see if anyone has an idea here.

Comment 7

[Lukas]

I performed it again, this time only marking a version as bad when downloading failed consistently, giving me this result:

app_name: firefox
build_date: 2020-09-29 00:35:29.842000
build_file: C:\Users\Lukas\.mozilla\mozregression\persist\e67a68917a43-debug--autoland--target.zip
build_type: integration
build_url: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/R-uIqKPHSSOclcaLcnHmyw/runs/0/artifacts/public%2Fbuild%2Ftarget.zip
changeset: e67a68917a431566af23ba26ee58edbc23347080
pushlog_url: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=08f8f6b292ec804e3510f93ffe1974a03af3dbd0&tochange=e67a68917a431566af23ba26ee58edbc23347080
repo_name: autoland
repo_url: https://hg.mozilla.org/integration/autoland
task_id: R-uIqKPHSSOclcaLcnHmyw

I already see more commits with changes to the Downloads API in this range, so we're probably closer to the cause.

Comment 8

[:Gijs (he/him)]

(In reply to Lukas from comment #0)

Thanks for all the research and the mozregression checks, that really helps! I've marked the regressing bug. This looks related to bug 1679675, but that's about a much more uncommon scenario and/or ramdisk tool, and this is more generic Windows networking stuff...

3. Save in a folder on a mapped drive from the local machine

Can you provide some more precise detail? That is, are you saving directly into Z:\\ which is a mapped network drive, or something? But it's mapped to a folder on the local machine from which you're RDP'ing into the machine where you're running Firefox?

And, if so, can you try saving to a subfolder, or (and yes I know this sounds dumb, but x-ref bug 1714583 where this would have made a difference) to a subfolder of a subfolder (so Z:\\foo\\bar\\file.txt) and confirming if any of those work?

Are there any errors in the browser console when this breaks?

Unfortunately I don't have a setup where I can reproduce this... Mike, I don't suppose you know if you/we have access to RDP systems anywhere internal to Mozilla that I'm unaware of?

Comment 9

[Lukas]

Hi Gijs, thanks for the response. I've tried the operations you've described. Saved a file several times, first on the root folder (D:\\), then a subfolder, and a sub-subfolder. Failed to save every time, the file on the disk contained 0 bytes. I've tried this with a few different setups: connecting from desktop to laptop (as described below), from desktop to server, and from laptop to server. The Firefox version I used this time was 92.

As for reproduction, the way I do it is by connecting from my desktop to my laptop through mstsc. The drive I am trying to save to is a local drive from my desktop, so the D:\\ in this case. This is done by expanding the options, opening the local sources tab, pressing more under Local Devices and sources and selecting a drive listed under stations.

I hope this will help you further.

Comment 10

[Marco Castelluccio [:marco]]

Set release status flags based on info from the regressing bug 827010

Comment 11

[:Gijs (he/him)]

OK, so it turns out this is reproducible with remote desktop, so all you need is 2 windows machines. More detailed STR:

(Optionally, on machine A, ensure you have a debugging/build environment (I used MSVS, I'm sure windbg or similar would also work))

1. enable remote desktop on Windows 10 machine A
2. log in to machine A from another Windows machine B. Before connecting, expand the advanced options, and under "local resources", click "more" and share a local drive
3. open Firefox inside the remote desktop connection
4. in Firefox, enable "ask me every time" for where to save downloads
   4a. if testing with Nightly, ensure browser.download.improvements_to_download_panel is disabled
5. go to e.g. https://filesamples.com
6. click csv, and download the first csv sample
7. in the "what do you want to do with this file" dialog, select "save to disk"
8. in the file picker, next to the drives local to machine A, you should see something like "X on machine B" where "X" is the drive letter of the drive you shared. Save the file there.

ER:
it works

AR:
it does not work.

This appears to be either an XPCOM or a Windows issue. Specifically, we hit this code path in nsLocalFileWin: https://searchfox.org/mozilla-central/rev/5e7976773895d72e47a76a9c912fbf3d0171e610/xpcom/io/nsLocalFileWin.cpp#1788,1810-1811,1818-1819 .

That is, we try to move the file from the temp dir to the destination, at which point Windows returns ERROR_NOT_SAME_DEVICE. This is somewhat expected for moving a file with MoveFileEx. The internet-documented workaround is to pass a flag to allow MoveFileEx to do a copy instead (cf https://stackoverflow.com/questions/21977090/movefileex-returning-getlasterror-17 ). However... in bug 545650 we found that some versions of Windows 7 corrupt files when doing this, so the workaround was to do the copy manually. This copy happens on line 1810/1811. In my case the arguments looked like this:

filePath: "C:\Users\Blah\AppData\Local\Temp\Eq5fZtK7.csv.part"
destPath: "\\tsclient\X\sample4.csv"
dwCopyFlags: 4104

This produces a GetLastError() result of 87, ie ERROR_INVALID_PARAMETER.

At this point, I'm not sure what Windows means by that or how that should be addressed... I expect it should be reproducible with a trivial commandline program that makes an analogous CopyFileExW call.

Kagami, I don't suppose you know?

(This file move worked when we used osfile, presumably because that does pass the "copy allowed" flag, cf https://searchfox.org/mozilla-central/rev/5e7976773895d72e47a76a9c912fbf3d0171e610/toolkit/components/osfile/modules/osfile_win_front.jsm#672 - I guess we could detect whether we're running on Win7 and only fall back to CopyFileEx in that case? That could reintroduce corruption in a newer client connecting to win7 or windows-server-equivalent buggy terminal/remote-desktop servers, but that seems like an edgecase, and with win7 being effectively on life support as far as MS is concerned, you'd hope people move away...)

Comment 12

[:Gijs (he/him)]

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

That could reintroduce corruption in a newer client connecting to win7 or windows-server-equivalent buggy terminal/remote-desktop servers

Err, this is a bit muddled - what I meant was, we wouldn't detect the case where Firefox is running on a newer Windows version, and the tsclient share is on Win7. I don't know if there's anything we can do about that, because actually that doesn't sound as implausible (note share of Win7 Firefox clients on e.g. https://data.firefox.com/dashboard/hardware#operating-system-metric-overview-1 ).

Comment 13

[Kagami Rosylight [:saschanaz] (they/them)]

Sorry, nothing immediately from my head, but the fallback idea sounds good to me.

in bug 545650 we found that some versions of Windows 7 corrupt files when doing this

Is there any chance that Windows 7 fixed it after that?

Comment 14

[Kagami Rosylight [:saschanaz] (they/them)]

It seems COPY_FILE_NO_BUFFERING is the culprit there, omitting it allows actual copy. A quick search shows someone complaining that Windows 8 caused a regression related to COPY_FILE_NO_BUFFERING, it could be that it's still relevant here.

Interestingly, using CopyFile2 with COPY_FILE_NO_BUFFERING gives me ERROR_ADAP_HDW_ERR instead of just ERROR_INVALID_PARAMETER, and I guess that's the real error?

COPYFILE2_EXTENDED_PARAMETERS params{};
params.dwSize = sizeof(params);
params.dwCopyFlags = dwCopyFlags;
copyOK = SUCCEEDED(::CopyFile2(filePath.get(), destPath.get(), &params));

Comment 15

[Masatoshi Kimura [:emk]]

How about falling back to CopyFileExW without COPY_FILE_NO_BUFFERING when CopyFileExW with COPY_FILE_NO_BUFFERING failed with ERROR_INVALID_PARAMETER?

Comment 16

[:Gijs (he/him)]

(In reply to Masatoshi Kimura [:emk] from comment #15)

How about falling back to CopyFileExW without COPY_FILE_NO_BUFFERING when CopyFileExW with COPY_FILE_NO_BUFFERING failed with ERROR_INVALID_PARAMETER?

WFM, though it does feel like we're setting ourselves up for failure again if Windows 11/12/... decides to "fix" the error code per comment #14, or just changes behaviour again. :-(

Anyway, this definitely seems like the least risky option, so I've prepared and tested a patch, and confirmed this works.

Comment 17

[:Gijs (he/him)]

Attachment: Bug 1731049 - fix copy/move behaviour for remote paths on Win10 when copying without buffering returns errors, r?masayuki

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Filed a bug report into Feedback Hub.
https://aka.ms/AAeqe0l

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe397267a03f
fix copy/move behaviour for remote paths on Win10 when copying without buffering returns errors, r=masayuki

Comment 20

[Marian-Vasile Laza]

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

Comment 21

[Lukas]

Just tested the nightly build on our Development Environment and it works! I'll keep testing for a bit to make sure it works consistently, but I'm very optimistic. Thanks a lot! As it's a Nightly build, there's a disclaimer not all features may make it to the final release. Will there be some sort of confirmation when the release is near? Or will it only be dropped if it appears to be breaking other functionality?
Once again, thanks for all your efforts!

Comment 22

[:Gijs (he/him)]

(In reply to Lukas from comment #21)

Just tested the nightly build on our Development Environment and it works! I'll keep testing for a bit to make sure it works consistently, but I'm very optimistic. Thanks a lot! As it's a Nightly build, there's a disclaimer not all features may make it to the final release.

This disclaimer is primarily about new features (rather than bugfixes) that we're explicitly testing out on Nightly only. I don't think it applies here, although...

Or will it only be dropped if it appears to be breaking other functionality?

Yes, there is a chance that this would get backed out if it broke something. That would be clearly noted on this bug if it happened (so "no news is good news", effectively).

However, I also suspect the majority of people likely to run into this issue (ie using terminal server or using other windows sharing functionality for downloads / file saving, if that has the same issue) will run Firefox's release branch rather than Nightly, so I'm not sure we'd find breakage unless it broke stuff for folks not using terminal server / windows sharing.

Given we won't learn much on Nightly, and given that we're relatively early in the beta cycle, I am inclined to suggest we uplift to beta 95. I'll fill out a request for this shortly.

Once again, thanks for all your efforts!

No problem, thank you for reporting this!

Comment 23

[:Gijs (he/him)]

Comment on attachment 9248522 [details]
Bug 1731049 - fix copy/move behaviour for remote paths on Win10 when copying without buffering returns errors, r?masayuki

Beta/Release Uplift Approval Request

• User impact if declined: Broken downloads functionality when using terminal server / windows sharing
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 11
• List of other uplifts needed: n/a
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We're fixing this specific broken case with what is a relatively very small patch, and the reporter has confirmed it fixes the issue for them. Although I've selected "no" for automated tests because we don't have one that uses windows terminal server, the change is in nsIFile which is used for the majority of our file operations on Windows, so if we broke something catastrophically, other automated tests would have complained, and I'm fairly confident that issues with it affecting larger user groups would be spotted quickly.

Issues with smaller user groups (like users of terminal server or windows sharing) are unlikely to be shaken out in Nightly, so I don't think riding the 96 train (ie waiting a number of weeks before this goes to beta) buys us much in terms of guarantees about the quality of the patch or otherwise, so we might as well uplift now and get this out with 95, to speed up the feedback cycle and the time it takes to shake out any more / follow-up issues with the patch

• String changes made/needed: None

Comment 24

[Mike Kaply [:mkaply]]

I think we should put this on the ESR as well.

Comment 25

[:Gijs (he/him)]

(In reply to Mike Kaply [:mkaply] from comment #24)

I think we should put this on the ESR as well.

Probably, but I'd like this to bake on release for at least a cycle before doing that; we wouldn't want to make things worse on ESR...

(I'm kind of surprised this didn't first surface as part of ESR, it's been crickets so far...)

Comment 26

[Alexandru Trif, Desktop QA [:atrif]]

Hello! Reproduced the initial issue with Firefox 95.0a1 (20211023211445) using a remote desktop connection with two Windows 10x64 machines and STR from comment 11. The download fails on the affected build creating only a 0KB .csv file.
The issue is no longer reproducible with Firefox 96.0a1 (20211110092453) on the same remote desktop Windows 10x64 machines. The file is successfully downloaded after following STR from comment 11.

Comment 27

[Pascal Chevrel:pascalc]

Comment on attachment 9248522 [details]
Bug 1731049 - fix copy/move behaviour for remote paths on Win10 when copying without buffering returns errors, r?masayuki

Verified by QA on nightly, approved for 95 beta 6, thanks.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/13a689d7b338

Comment 29

[Alexandru Trif, Desktop QA [:atrif]]

Verified fixed with 95.0b6 (20211110162506) from comment 28. The file is correctly downloaded after following STR from comment 11. Removing qe+ flag based on comment 25.

Comment 30

[:Gijs (he/him)]

Comment on attachment 9248522 [details]
Bug 1731049 - fix copy/move behaviour for remote paths on Win10 when copying without buffering returns errors, r?masayuki

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Reliability of file moves/copies improves for enterprise users who use network shares
• User impact if declined: Such file moves/copies break
• Fix Landed on Version: 96, uplifted to 95
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): We're adding additional error handling code for a specific scenario, so the patch size is limited and pretty low risk. It'll also have baked a little while by the time we ship on esr in conjunction with the 96 cycle, so all that is low risk. However, there is no automated test coverage and we are unlikely to get bugreports from users using network shares until this hits release, and the roll-out cycle of release means it'll be a few weeks yet before that has happened, and then we're in end-of-year PTO territory.
• String or UUID changes made by this patch: Nope

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9248522 [details]
Bug 1731049 - fix copy/move behaviour for remote paths on Win10 when copying without buffering returns errors, r?masayuki

95 has been out for a couple weeks now without any known issues related to this change. Approved for 91.5esr.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/01850e97e9de

Comment 34

[Alexandru Trif, Desktop QA [:atrif]]

Verified fixed with 91.5.0esr (20220105212146) on Windows 10x64 with a remote connection to another Windows 10x64.