322426 - Properly handle the deletion of filesystem objects with a read-only file attribute on Win32

Status: NEW

(Firefox :: File Handling, defect)

Description

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

This bug is to track the several areas where we respect a filesystem object's read-only file attribute and the application breaks. Some examples are:
Extension install, upgrade, uninstall (see bug 321333)
Deleting a profile - all of the files / directories that have a read-only attribute are not deleted.
bookmarks.html - see bug 223771 and to reproduce use attrib +R on bookmarks.html.
I also suspect that clear private data is affected by this though I haven't verified it.
etc.

To reproduce you can copy a backed up profile from a read-only CD on a Win32 system prior to WinXP - with WinXP the behavior was changed when copying files from a read-only CD but I would not be surprised if some backup / restore applications will also cause this issue. You can also cause this by using the attrib command on a profile directory with the /S /D switches.

Also of note is that the explorer ui will not set or remove a read only attribute on a directory and that the attrib command must be used.

Some background articles:
Win2K, NT 4.0, WinME, Win98, and Win95 MS KB artcle
http://support.microsoft.com/kb/256614/EN-US/ - important points:
SYMPTOMS
You may be unable to remove the Read-Only attribute from a folder using Windows Explorer. In addition, some programs may display error messages when you try to save files to the folder.
CAUSE
This behavior occurs because the folder is customized. You can customize a folder by clicking Customize this folder on the View menu. Many programs also customize folders (for example, the Fonts folder comes with a customization as part of the standard system configuration).
------
So, the read-only attribute is being used for purposes other than to signify that a filesystem object is to be treated as read-only.


WinXP MS KB artcle - I suspect this will apply to Vista in some manner as well
http://support.microsoft.com/?id=326549 - important points:
Unlike the Read-only attribute for a file, the Read-only attribute for a folder is typically ignored by Windows, Windows components and accessories, and other programs. For example, you can delete, rename, and change a folder with the Read-only attribute by using Windows Explorer.
------
This is contrary to the following in MSDN
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/fileio/fs/win32_file_attribute_data_str.asp
FILE_ATTRIBUTE_READONLY - The file or directory is read-only.
Applications can read the file but cannot write to it or delete it.
For a directory, applications cannot delete it.

BTW: the only difference in the file deletion confirmation prompt with a default WinXP configuration is "The filename is a read-only file." and then continues with text asking if you want to delete the file followed by a successful deletion. This includes directories so the statement "applications cannot delete it" is clearly incorrect though one might argue that explorer is part of the OS instead of an application that comes with the OS even though it is not required by the OS.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

that does not mean that "applications cannot delete it" is incorrect. explorer may unset the attribute and only then delete the file.

Comment 2

[Brian R. Bondy [:bbondy]]

It's common amongst windows programs to remove the read only attribute before removing them.  

rstrong, would you prefer if I add a new method something like nsILocalFile::removeFileAlways? Or change the existing implementation of nsIFile::remove to unset the attribute if set, and then delete the file? 

Once this is done I can take the other dependent tasks.

Comment 3

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

I would prefer if it unset the attribute and then deleted the file. I am somewhat concerned there may be consumers that utilize the current behavior though I am of the opinion that we should just deal with any fallout (if any) that occurs.

Would be a good thing to get the input of a peer of this code as well.

Comment 4

[Brian R. Bondy [:bbondy]]

I also agree with Comment 3, I know I'm not yet a peer on this though.
CC'ing Benjamin Smedberg for peer / module owner feedback.

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: Patch for ability to remove read only files

For peer review as well, this patch allows the deletion of read only files.

Comment 6

[Benjamin Smedberg]

Comment on attachment 556631 [details] [diff] [review]
Patch for ability to remove read only files

Is there no way to write an xpcshell test for this? I would think that you could at least using ctypes, if there's currently no nsIFile way to mark a file as readonly.

Comment 7

[Brian R. Bondy [:bbondy]]

You can mark a file as readonly using the permissions attribute.  I am going to be posting a bug soon which will detail a bunch of xpcshell tests for nsIFile since we don't currently have very good coverage there.  I will handle this case there if that's OK?

Comment 8

[Jim Mathies [:jimm]]

Note I think we still use FAT partitions on the slaves which prevents us from testing some attributes. There are a few existing tests that are disabled due to this.

Comment 9

[Brian R. Bondy [:bbondy]]

Maybe I could also do a ticket on adding an interface member function to get the filesystem type?  Maybe in file/directory_service;1 or file/local?

It would be nice to have a bunch more tests for NTFS specific cases which would run when running on NTFS.

Comment 10

[Brian R. Bondy [:bbondy]]

> ...interface member function...

I guess it would be a read only attribute.

Comment 11

[Brian R. Bondy [:bbondy]]

I'll just put some unit tests in this ticket and then we can discuss the separate tests offline or in the context of the other ticket.