1679675 - nsILocalFile::MoveTo() (and therefore IOUtils.move) fails when run in the root of a RamDisk mountpoint (was: Download -> Open with failed when using RamDisk as temp folder)

Status: NEW

(Core :: XPCOM, defect, P2)

Description

[thanhminh.mr]

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

Steps to reproduce:

0. Environment: Using ImDisk RamDisk in mount to folder mode, set both system and user %temp% and %tmp% to the ramdisk folder.

1. Download any file that Firefox can open directly (any file that aren't executable file, for example: a docx file)

2. The download dialog shows up, choose to open it with appropiate program (Libre Office for example) and start the download

Actual results:

The download immediately fail with no further warning or error.

Expected results:

The file download successfully to a temp folder and then open with the selected program.

Comment 1

[thanhminh.mr]

Additional Info: This behavior started recently (about a month or two?), I have this temp folder setup for 3 years and it works fine.

Comment 2

[Andrei Purice]

Setting a component for this issue in order to get the dev team involved.
If you feel it's an incorrect one please feel free to change it to a more appropriate one.

Comment 3

[Marco Bonardo [:mak]]

Could you please help us finding the culprit change? We have a util called mozregression that allows to download and test various Firefox builds to find when a bug started. You just give it a time bracket where the bug started happening, then test each build and mark it as good (no bug) or bad (has bug). In the end it gives you a link to a list of bugs that contains the problematic change. That would help a lot speeding up our investigation.
You can find more details here: https://mozilla.github.io/mozregression/

Comment 4

[thanhminh.mr]

After running the tool, here is the link of the bug that introduce the behaviour: https://phabricator.services.mozilla.com/D91069
More over, I can replicate this problem on another Windows 10 x64 machine.
After taking a closer look, the downloading file seem to successfully download to the *.part file and also create a empty file with correct name (afaik this is the normal behaviour), but failed to rename the *.part file to the correct name.

Comment 5

[Marco Bonardo [:mak]]

Thank you, it may be some not well handled behavior in IOUtils.

Comment 6

[:Gijs (he/him)]

(In reply to thanhminh.mr from comment #4)

After running the tool, here is the link of the bug that introduce the behaviour: https://phabricator.services.mozilla.com/D91069
More over, I can replicate this problem on another Windows 10 x64 machine.
After taking a closer look, the downloading file seem to successfully download to the *.part file and also create a empty file with correct name (afaik this is the normal behaviour), but failed to rename the *.part file to the correct name.

Is there an error in the browser console (not the regular devtools console; you can open the browser one with ctrl-shift-j) when this happens ?

Comment 7

[thanhminh.mr]

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

(In reply to thanhminh.mr from comment #4)

After running the tool, here is the link of the bug that introduce the behaviour: https://phabricator.services.mozilla.com/D91069
More over, I can replicate this problem on another Windows 10 x64 machine.
After taking a closer look, the downloading file seem to successfully download to the *.part file and also create a empty file with correct name (afaik this is the normal behaviour), but failed to rename the *.part file to the correct name.

Is there an error in the browser console (not the regular devtools console; you can open the browser one with ctrl-shift-j) when this happens ?

There is none. The download just failed with a yellow dot on the download icon. I tested on both machine I have with the same setup.

Comment 8

[:Gijs (he/him)]

I installed ImToolkit on a Windows 7 VM, set up a RAM disk as R:\ and made the temp dir R:\Temp and set all the environment variables, then logged out and back in again, verified that Firefox puts files there when downloading and opening them - but I cannot reproduce the issue, the download works fine.

If it matters, I opened a google doc, used the gdoc "File > Download as" menu to download it as a docx, I get a download prompt suggesting opening it with WordPad, accept this, and the document opens with WordPad just fine.

Can you point to a specific download with which you have this problem? Are there any other circumstances that might be relevant (add-ons you have installed, preferences you've changed, other software on the machine)? Can you try a clean Firefox profile ( https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles ) just to see if that makes a difference?

Comment 9

[thanhminh.mr]

I config my ram disk a little bit different from the default setting:
I set the ram disk to be mounted to an empty folder in a NTFS partition, and then set the environment variables to that folder.
You could look at these screencaps for more info: https://gist.github.com/thanhminhmr/8cfea039c6f84d1df488b15ac26bf0c4

I retest the ramdisk with the default setting, indeed it works fine, but I can not use this setup because some other older software doesn't know how to deal with a temp folder that are not in the system drive.

Comment 10

[:Gijs (he/him)]

OK, with the details from comment #9 I can reproduce.

I don't really understand what's going on though. The thing that fails is moving the already-downloaded file from its .part file to the target (the real name we want the file to have). That is, this hunk in the regressing patches is what fails.

This is because the following snippet also fails:

x = new FileUtils.File("C:\\Temp\\partfilename.part")
x.moveTo(x.parent, "myfancydocument.docx");

with:

Exception { name: "NS_ERROR_FILE_ALREADY_EXISTS", message: "Component returned failure code: 0x80520008 (NS_ERROR_FILE_ALREADY_EXISTS) [nsIFile.moveTo]", result: 2152857608, ... }

This obviously doesn't make much sense - the destination file does not in fact exist in this folder.

I've also noticed that for the source file, x.exists() returns true, but x.parent.exists() returns false (that is, we don't think C:\Temp as a folder exists...). So I think what's happening is that we try to create C:\Temp folder, because we were asked to move something into it and we didn't think it existed, but Windows then tells us "hey, this is already here" when we try to create the parent folder to which we're trying to move the file, and then things blow up.

The confusion over whether C:\Temp\ exists appears to be related to the fact that it's a symlink / mount point type thing, because e.g. x.parent.lastModifiedTime returns a date stamp correctly, but the file.exists() implementation appears to depend on ResolveAndStat() returning success. I don't know why it doesn't here; because I ran all this stuff in a VM I don't have a C++ debugger to hand, but even if I did, I wouldn't necessarily be sure what the right fix would be. Kagami, it looks like you worked on this code this past year, can you take a look when you're back after the holiday break, please?

However, I'm also confused about this RAM disk thing. The configuration you linked suggests that it should basically have C:\Temp be a ram disk, right (and be the same as the contents of R:\)? But as far as I can tell, temp files Firefox creates show up under C:\Temp, but not under R:\, which is very strange if one is meant to point to the other. Reporter, have you noticed this? Do you know how RamDisk works under the hood?

As a workaround, if your setup has ended up looking like mine, with C:\Temp pointing to R:\, and R:\ having another Temp folder in it, I think things will go back to working if you set your temp dir environment variables to point to C:\Temp\Temp. Which is dumb, I know, but if it helps while we get to the bottom of this I figured it was worth pointing out.

Comment 11

[:Gijs (he/him)]

Barret, Emma, CC'ing you for visibility. This will be something to keep an eye on when continuing with OS.File-to-IOUtils conversions; AFAICT this will affects moves and copies into junction/mount/symlink/ramdisk thingies, whatever they are...

Comment 12

[thanhminh.mr]

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

However, I'm also confused about this RAM disk thing. The configuration you linked suggests that it should basically have C:\Temp be a ram disk, right (and be the same as the contents of R:\)? But as far as I can tell, temp files Firefox creates show up under C:\Temp, but not under R:\, which is very strange if one is meant to point to the other. Reporter, have you noticed this? Do you know how RamDisk works under the hood?

This is because the ram disk that mounted to R: and to C:\Temp are actually different and you can unmount it separately (Look at my screencaps, I actually don't even mount and use the R: ram disk).

As a workaround, if your setup has ended up looking like mine, with C:\Temp pointing to R:\, and R:\ having another Temp folder in it, I think things will go back to working if you set your temp dir environment variables to point to C:\Temp\Temp. Which is dumb, I know, but if it helps while we get to the bottom of this I figured it was worth pointing out.

Thanks for your suggestion, it's work as a temporary solution.

Comment 13

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

It's GetFinalPathNameByHandleW inside WinUtils::ResolveJunctionPointsAndSymLinks that is failing. The path has no drive letter although ImDisk says it's mounted as R:\:

> ls c:/

    Directory: C:\

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
l----          05/01/2021  5:39 PM                Temp -> \Device\ImDisk0\

Replacing VOLUME_NAME_DOS with VOLUME_NAME_NT gives the exact string \Device\ImDisk0\ but this syntax is not supported by GetFileAttributesExW() in GetFileInfo(), so 🤷‍♀️

diff --git a/widget/windows/WinUtils.cpp b/widget/windows/WinUtils.cpp
index 14706d86a4e41..731da8ccb4ee4 100644
--- a/widget/windows/WinUtils.cpp
+++ b/widget/windows/WinUtils.cpp
@@ -1958,7 +1958,13 @@ bool WinUtils::ResolveJunctionPointsAndSymLinks(std::wstring& aPath) {
       handle, path, MAX_PATH, FILE_NAME_NORMALIZED | VOLUME_NAME_DOS);
   if (pathLen == 0 || pathLen >= MAX_PATH) {
     LOG_E("GetFinalPathNameByHandleW failed. GetLastError=%d", GetLastError());
-    return false;
+
+    pathLen = GetFinalPathNameByHandleW(
+      handle, path, MAX_PATH, FILE_NAME_NORMALIZED | VOLUME_NAME_NT);
+    if (pathLen == 0 || pathLen >= MAX_PATH) {
+      LOG_E("GetFinalPathNameByHandleW failed. GetLastError=%d", GetLastError());
+      return false;
+    }
   }
   aPath = path;

This passes the resolver but fails at GetFileInfo().

Comment 14

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

Attachment: Bug 1679675 - Fallback to VOLUME_NAME_NT

Comment 15

[spodi2290]

Downloads directly stored at the location of symlinks still fail in 89.0.2

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

As a workaround, if your setup has ended up looking like mine, with C:\Temp pointing to R:\, and R:\ having another Temp folder in it, I think things will go back to working if you set your temp dir environment variables to point to C:\Temp\Temp. Which is dumb, I know, but if it helps while we get to the bottom of this I figured it was worth pointing out.

That actually let me directly open files with firefox again.
Sadly this workaround is a no-go for me, because then some windows-store apps like Whatsapp-Desktop are starting to fail (crash at start) for whatever reason...

Comment 16

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

Unassigning me as I haven't been looking at this for a good time.

Comment 17

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

Has bug 1763978 fixed it?