1555644 - [*nix / macOS / Linux] OS.File should fall back to copy+delete if moving a file fails

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect, P2)

Description

[:Gijs (out for now; he/him)]

In nsILocalFile, file moves fall back to using copy+delete if the move fails (see https://searchfox.org/mozilla-central/rev/7556a400affa9eb99e522d2d17c40689fa23a729/xpcom/io/nsLocalFileWin.cpp#1683-1719 and https://searchfox.org/mozilla-central/rev/7556a400affa9eb99e522d2d17c40689fa23a729/xpcom/io/nsLocalFileUnix.cpp#1000-1010 ), which can happen due to a variety of permission issues (e.g. bug 1549259, bug 1539881 - I believe there are others but they're tricky to find in bugzilla).

OS.File does not do this, and so it causes problems with downloads in a variety of circumstances. We should fix this.

Yoric, do you have cycles to work on this?

Comment 1

[David Teller [:Yoric] - still alive but not very active]

I agree that it's needed but I doubt I have any cycles to work on this.

Willing to mentor and/or review, though.

Comment 2

[:Gijs (out for now; he/him)]

Looks like the *nix implementation of nsILocalFile actually also needs to allow for copying in more situations than just EXDEV.

Comment 3

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

The priority flag is not set for this bug.
:Yoric, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[Amit Raj]

Hi,
New to the community. I would be more than happy to contribute. Just set me with the necessary links and i will look into the bug.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Hi Amit,
Thanks for offering to help! Have you already downloaded and built the source code of Firefox?

Comment 6

[Hamzah Akhtar]

Hey! what are the files associated with this bug. Can you please specify them here so that I can give them a look and try to solve the issue:)

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Hi Hamzah, there are two files associated with this bug:

• Unix version: https://searchfox.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_unix_front.jsm#697-734
• Windows version: https://searchfox.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_win_front.jsm#669-723

Comment 8

[Hamzah Akhtar]

Hi David!
In the files that you mentioned above, you want to fall back to copy+delete only if the move operation fails or is there something else that you are looking for?

Comment 9

[David Teller [:Yoric] - still alive but not very active]

(In reply to Hamzah Akhtar from comment #8)

Hi David!
In the files that you mentioned above, you want to fall back to copy+delete only if the move operation fails or is there something else that you are looking for?

That's pretty much it. If you look at the code, you'll see that we already do it in some handpicked cases. It seems we actually need to do it in all cases.

Comment 10

[Priyank Singh]

Willing to work on this issue.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Hey, Hamzah, what's your status?

Comment 12

[Hamzah Akhtar]

I'm currently caught up working on some other bug. Priyank, feel free to take up the issue.

Comment 13

[Priyank Singh]

Should I call the copy function and then File.remove function if move fails to fix the issue?

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Hey, Priyank, if you look at the source code I've linked in comment 7, you'll see the existing code. You'll see that we already have a fallback that pretty much calls copy then remove. We just need to make sure that it's always called in case of error, not just in a few cases.

Comment 15

[Priyank Singh]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #14)

Hey, Priyank, if you look at the source code I've linked in comment 7, you'll see the existing code. You'll see that we already have a fallback that pretty much calls copy then remove. We just need to make sure that it's always called in case of error, not just in a few cases.

Can you please highlight the part where its already implement,
also how would I know whether and error has occurred does the file throw some kind of exception or something?

Comment 16

[Priyank Singh]

(In reply to Priyank Singh from comment #15)

Can you please highlight the part where its already implement,
also how would I know whether and error has occurred does the file throw some kind of exception or something?

*implemented

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Priyank, have you already looked at the lines I've linked in comment 7?

Comment 18

[Priyank Singh]

Yes.
Saw the complete File.Move function, unable to determine where the copy then remove procedure was implemented.
I guess WinFile.MoveFileEx is responsible for moving the file.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

So, let's start with the Unix version.

This code is the fallback. It is used to ensure that we copy & remove in case the move failed. For this version, we just need to make sure that we always use that fallback code if there was an error with the main code, not just in case of EXDEV.

For the Windows version, there's no fallback yet. We need to detect whether this call succeeded. If it failed, call File.copy and File.remove.

Comment 20

[Priyank Singh]

Okay, saw the File.Move implementation for both unix and windows,
unix implementation always copies and then removes the file, and we need to invoke that functionality if there is some sort of error in main code, but what if there is no error in main code, how can we move the file without using File.copy and File.remove.

Also, we checked for EXDEV error apart from that what all errors main code could get which needs to be checked before moving the file.

Comment 21

[David Teller [:Yoric] - still alive but not very active]

(In reply to Priyank Singh from comment #20)

Okay, saw the File.Move implementation for both unix and windows,
unix implementation always copies and then removes the file, and we need to invoke that functionality if there is some sort of error in main code, but what if there is no error in main code, how can we move the file without using File.copy and File.remove.

No quite. The Unix implementation calls UnixFile.rename. If it succeeds (i.e. if it doesn't return -1), we stop there.

What we need now is basically to change line 726 to not check whether the error is EXDEV.

Also, we checked for EXDEV error apart from that what all errors main code could get which needs to be checked before moving the file.

I don't understand that sentence.

Comment 22

[Priyank Singh]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #21)

Also, we checked for EXDEV error apart from that what all errors main code could get which needs to be checked before moving the file.

I don't understand that sentence.
I meant how could we possibly detect that an error has occurred in main code, does it throws some exception?

What we need now is basically to change line 726 to not check whether the error is EXDEV.
Why do we need to do this?

Comment 23

[Priyank Singh]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #21)

Also, we checked for EXDEV error apart from that what all errors main code could get which needs to be checked before moving the file.

I don't understand that sentence.

I meant how could we possibly detect that an error has occurred in main code, does it throws some exception?

What we need now is basically to change line 726 to not check whether the error is EXDEV.

Why do we need to do this?

Comment 24

[David Teller [:Yoric] - still alive but not very active]

(In reply to Priyank Singh from comment #23)

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #21)

Also, we checked for EXDEV error apart from that what all errors main code could get which needs to be checked before moving the file.

I don't understand that sentence.

I meant how could we possibly detect that an error has occurred in main code, does it throws some exception?

Functions from UnixFile follow the Unix conventions. It returns -1 in case of error, then we need to check the value of ctypes.errno to find out which error.

Functions from WinFile follow the Windows conventions. It returns 0 in case of error, then we need to check the value of ctypes.winLastError.

What we need now is basically to change line 726 to not check whether the error is EXDEV.

Why do we need to do this?

EXDEV is one specific Unix error that can show up in ctypes.errono. See http://man7.org/linux/man-pages/man3/errno.3.html for an (unfortunately incomplete) list of Unix errors.

Comment 25

[Priyank Singh]

Okay, so I must remove the check for EXDEV error so that our File.Move and File.remove would be called for all the cases whenever Unix.rename returns -1.

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Well, it's File.copy and File.remove, but yeah, that's the idea.

Comment 27

[Priyank Singh]

I had a look at windows implementation also, looks like throw_on_zero() function throws an error if the function WinFile.MoveFileEx returns zero.
So to have a fallback we must simply add File.copy and File.remove at the end of File.Move function.
Am I right?

Comment 28

[David Teller [:Yoric] - still alive but not very active]

(In reply to Priyank Singh from comment #27)

I had a look at windows implementation also, looks like throw_on_zero() function throws an error if the function WinFile.MoveFileEx returns zero.
So to have a fallback we must simply add File.copy and File.remove at the end of File.Move function.
Am I right?

Well, you need to call File.copy and File.remove if WinFile.MoveFileEx fails.

Comment 29

[Priyank Singh]

Attachment: Bug 1555644 - Added fallback to call File.copy and File.remove function if MoveEx function fails

Comment 30

[Priyank Singh]

Attachment: Bug 1555644 - Added fallback to call File.copy and File.remove function if MoveEx function fails

Comment 31

[David Teller [:Yoric] - still alive but not very active]

Mmmh... Why did you upload two patches? One of them is probably wrong? Maybe you intended to upload the Unix version?

Comment 32

[Priyank Singh]

I mistakingly committed my changes rather than amending it after I had sent my first patch for review. The second patch is just the continuation of the first one. I guess both the patches need to be merged.

Comment 33

[David Teller [:Yoric] - still alive but not very active]

Ok, could you merge them and resubmit?

If you're using hg, the most common way of doing this would be histedit tip^ and then choose roll for your latest patch.

Comment 34

[:Gijs (out for now; he/him)]

Fairly sure Yoric meant to needinfo Priyank...

Comment 35

[Priyank Singh]

Can you provide me with more information as I don't know how to merge the patches.

Comment 36

[David Teller [:Yoric] - still alive but not very active]

That depends, are you using mercurial or git?

Comment 37

[Priyank Singh]

mercurial

Comment 38

[David Teller [:Yoric] - still alive but not very active]

In this case, if you have the fold extension installed, run

hg fold --from tip^

This will take your previous patch and your current patch and turn them into a single patch.

If you do not have the fold extension installed, you'll need to use histedit, so

hg histedit tip^

and choose roll for your latest patch. This will make it part of the previous patch.

Comment 39

[Priyank Singh]

But I haven't created another patch,
what I'd done previously was used hg commit --amend -m <I used the same message here>.
Therefore it didn't create another patch but the as the commit message was changed phabricator thought that it is another patch while I used --amend rather than creating another patch.

Comment 40

[David Teller [:Yoric] - still alive but not very active]

Well, that is odd.

In this case, in the "Add action" menu (in the bottom of phabricator), you should have access to an item "Abandon Revision" (if I recall correcty) that will let you remove this patch: https://phabricator.services.mozilla.com/D43211 .

Once this is done, don't forget to apply the changes I requested :)

Comment 41

[David Teller [:Yoric] - still alive but not very active]

Oh, looks like I forgot to click "Submit" with said requests when I wrote them. Sorry about that!

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1091690427f2
Added fallback to call File.copy and File.remove function if moving function fails r=Yoric

Comment 43

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4411daffea4e
Backed out changeset 1091690427f2 for causing browser-chrome failures. CLOSED TREE

Comment 44

[Alexandru Michis [:malexandru]]

Backed out changeset 1091690427f2 for causing browser-chrome failures and copy failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/4411daffea4e597e7c845944db67d8da3b377556

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&fromchange=1091690427f248df7ee3cb707e99197160cf8b2c&tochange=4411daffea4e597e7c845944db67d8da3b377556

Failure log:

• https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=266911115&repo=autoland&lineNumber=7670
• https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=266911099&repo=autoland&lineNumber=4630
• https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=266914044&repo=autoland&lineNumber=5818

Comment 45

[Alexandru Michis [:malexandru]]

Also caused marionette failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=windows%2C10%2Cx64%2Cdebug%2Ctest-windows10-64%2Fdebug-marionette-e10s%2C%28mn%29&tochange=71978ebe41f1bcee35fc8ab91424cd0a5e5f115f&fromchange=b47e17be584471f8033ca63aaff59efd3fc371ef

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=266916533&repo=autoland&lineNumber=65841

Comment 46

[David Teller [:Yoric] - still alive but not very active]

Ok, so a few errors to fix! I've left you new comments on Phabricator.

Comment 47

[David Teller [:Yoric] - still alive but not very active]

Tests running here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c52ec22a0cb213f1d4eefab47c270041dd6e4003 .

Comment 48

[David Teller [:Yoric] - still alive but not very active]

Interesting. I see some errors:

"Win error 2 during operation copy on file C:\Users\task_1568812477\AppData\Local\Temp\test_osfile_async_copy_dest.tmp"

In the Windows version, maybe the call to File.copy has the wrong arguments? Could you double-check?

Comment 49

[Priyank Singh]

I checked the call, arguments are correct.
What if it is failing due to the previous patch which I had deleted.
Could you check if thats the issue?

Comment 50

[David Teller [:Yoric] - still alive but not very active]

If there's no problem with the call, it might be that the call to WinFile somehow succeeded at moving the file but returned non-0. Odd.

Comment 51

[Priyank Singh]

Have you found the problem yet?
I observed that the build was failing in phabricator after I had deleted/abandoned the previous patch.
Could you check the complete code that is being pushed whether it is correct or not?

Comment 52

[David Teller [:Yoric] - still alive but not very active]

I'm not sure yet what the issue is, but it's clearly not related to the previous patch.

Let's start by landing only the Unix version. We'll think about the Windows version later. Looking at the documentation of MoveFileEx ( https://msdn.microsoft.com/en-us/windows/aa365240(v=vs.80) ), I am starting to think that maybe MoveFileEx does everything we need already.

Comment 53

[:Gijs (out for now; he/him)]

Yoric, looks like Priyank Singh has disappeared, can you resubmit the unix part to phab and land that, please? We keep getting bugs filed on this, cf. bug 1602306.

Comment 54

[David Teller [:Yoric] - still alive but not very active]

I'll try and do that within the week.

Comment 55

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1555644 - If a file cannot be fast-renamed, copy it and remove the original;r?Gijs

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e7deb2b7e09e
If a file cannot be fast-renamed, copy it and remove the original;r=Gijs

Comment 57

[Cristina Coroiu [:ccoroiu]]

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