96388 - nsIFile::Remove() will fail to delete an open file on win32

Status: RESOLVED INVALID

(Core :: Networking: File, defect, P1)

Description

[John Morrison]

nsIFile::Remove() will fail to delete an open file on win32

This came up in the course of bug 68045 where, if the fastload file is 
corrupt, it is supposed to be deleted with nsIFile::Remove(). That's 
what happens on Linux, but the file is not deleted on win2k. 

Looking at the docs for win32's |int remove(const char *path)|, it says:

  "This function fails and returns -1 if the file is open. ... All
   handles to a file must be closed before it can be deleted."

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vccore98/HTML/_crt_remove.2c_._wremove.asp

I'm not sure what the intended semantics of Remove() are, but it seems
like it should be the same for win32, linux and mac.

Comment 1

[John Morrison]

Er, to be clear nsLocalFileWin.cpp uses |remove|, while 
nsLocalFileUnix.cpp uses |unlink|.

Comment 2

[John Morrison]

Hmm, it seems that nsLocalFileMac.cpp is similar to win32, in that |HDelete|
is used on the Mac, and the file must be closed before calling that. It's 
only *nix, using |unlink| that will expunge an open file (although unlink
defers deletion until noone has a handle; not sure how this quite works in 
the *nix case).

Comment 3

[Brendan Eich [:brendan]]

Yeah, Unix does open-unlink and other OSes do not.  The question is, should
nsIFile::Remove do as Unix does?  If there is a way to detect the open remove
case, rename the file (does Windows allow this, even?  Within the same drive, or
same directory?), and remove it later when the handle is closed, then we should
do that.  But I'm pessimistic.  I had better prepare a workaround in FastLoad
code....

/be

Comment 4

[timeless]

references to how windows handles installer code ...

If fastload has the file open and tries to nuke it, then that's a bug in 
fastload :-) $what_brendan_said =~ s/workaround/fix/

Comment 5

[Doug Turner (:dougt)]

what would you propose that nsIFile do if a file can not be deleted?  Should we
do what XPInstall does and manage a list of files which could not be deleted,
and try again when xpcom restarts??

Comment 6

[Brendan Eich [:brendan]]

Doug, all I think this bug should mean is to doc-comment the open file failure
case in nsIFile.idl.  Right?

/be

Comment 7

[Doug Turner (:dougt)]

Pete, could you add this to your changes to the interface?

Comment 8

[Pete Collins (MDG)]

No, this is a great bug and i hate the fact that i can't remove a "locked" file
on win and mac.

I needed this to implement an uninstall for Chameleon, which worked great on
unix but left the jar files on win and mac.

I think if you remove, then that means kill it.

Soug, are you just saying i comment that remove will not delete open files an
win and mac?

I would prefer it if the files were actually deleted.

--pete

Comment 9

[benc]

It's not the most well known fact, but there was actually a little extension 
(INIT if you go far enough back) with some Apple Installers called something 
like "Uninstall extension".

It's job was to take a list of files that were opened by the installer, and 
delete them upon reboot.

What do other vendors do?

Comment 10

[benc]

Pete: the bug interface does not illustrate it clearly enough, but code
contributors (owners) generally assign the "priority" field. At-large
contributors give input by setting the "severity" field.

Can you update the severity field to the level you think is appropriate?

Comment 11

[Pete Collins (MDG)]

normal is fine.

I am mass changing pre 1.0 nsIFile bugs assigned to me to P1 so I can knock them
off one at a time.

--pete

Comment 12

[benc]

jrgm: can you update, is this still happening in 1.4x?

-> whiteboxqa

If there is an end user set of steps, I can test this

Comment 13

[John Morrison]

Well, this is mainly just a bug to note the nsIFile::Remove has slightly
different semantics on unix-based-OS vs. win32. (See the first 6 or so
comments). I don't know what petejc's intentions are for this bug, but as at
comment #6 this was just about changing the doc-comment in the IDL to make it
clear that "Remove" can not
remove an open file on Win32.

Comment 14

[David G King]

Hmmm, it looks like MS Windows has an "unlink" function. See
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vccore98/HTML/_crt__unlink.2c_._wunlink.asp
for more info. Actually, this seems to be a Visual Studio function, so I'd hate
to think what this means for GCC on Win32.

Comment 15

[Malcolm Rowe]

The MSVC unlink() and remove() CRT functions both end up calling the Win32
DeleteFile function, so there's no real difference (unlink() is literally just a
wrapper for remove()).

Interestingly, the documentation for DeleteFile says both:

* The DeleteFile function fails if an application attempts to delete a file that
is open for normal I/O or as a memory-mapped file.

and

* The DeleteFile function marks a file for deletion on close. Therefore, the
file deletion does not occur until the last handle to the file is closed.
Subsequent calls to CreateFile to open the file fail with ERROR_ACCESS_DENIED.

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/fileio/base/deletefile.asp

I don't quite see how both can be correct.

Comment 16

[Pete Collins (MDG)]

Yea, this needs to be addressed. I haven't looked at this bug for a while but
the inconsistencies between platforms needs to be fixed.

Here is the implementation in question:

  http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileWin.cpp#1361

The implementation is using remove().

Has anyone tried removing a file on win and then shutting down Mozilla to see if
it is deleted once the file handle is released?

Comment 17

[David G King]

While I haven't tried the procedure in comment #16, I can say that the
documentation, although vague, is correct.

From my experience with patching Windows (with MS supplied patches), you often
need to reboot windows for the sole purpose of deleting an inuse file and
replacing it with a new version of that file.

So, DeleteFile will return an error if the file is in use. This error will need
to be caught and a process designed to ensure whatever is using the file is
closed so that the file can be deleted and/or replaced.

Comment 18

[Brian R. Bondy [:bbondy]]

You can't (*** see below) remove a file that is exclusively opened on Windows.
Other programs that open a file first, can specify no FILE_SHARE_DELETE flag.

You can ask that Windows delete a file on reboot if it is exclusively opened by using MoveFileEx with MOVEFILE_DELAY_UNTIL_REBOOT.
I don't think we should do that though.

I nominate that this bug be resolved as invalid. 
The user of nsIFile should not assume that delete will work on every platform.  
They should check for an error of NS_ERROR_FILE_IS_LOCKED and behave appropriately depending on the user's situation. 

***
There is an actual way to do the delete at the driver level, but we would definitely not do it.
I.e. you could use a mini filter driver, or a file system driver to do the work.

Comment 19

[Robert Strong (they/them - no direct email)]

Agree wholeheartedly with comment #18. Also, Linux has mount -o mand though I haven't verified whether it will also prevent deletion as does Windows.

Comment 20

[Brian R. Bondy [:bbondy]]

>  Also, Linux has mount -o mand though I haven't verified whether it will also prevent deletion as does Windows

I haven't tried either but I assume it will give errors.  Locking in unix/linux is by default advisory, meaning other processes don't need to follow the locking rules that are set by fcntl, lockf.   But as Robert mentioned, you can mount as mandatory instead of advisory and then deletions will probably not work even on unix/linux.

Resolving as invalid for now.