682571 - nsLocalFile::isWritable behaves wrongly on Windows

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Brian R. Bondy [:bbondy]]

nsLocalFile::isWritable behaves wrongly on Windows.

1) For read only file shares, we return true:
var file = Components.classes['@mozilla.org/file/local;1'].createInstance(Components.interfaces.nsILocalFile); file.initWithPath("\\\\rhesus\\readonlyshare\\1.txt"); file.isWritable()

> true

This should not return true, it should return false.


2) For files that are exclusively opened or locked:
var file = Components.classes['@mozilla.org/file/local;1'].createInstance(Components.interfaces.nsILocalFile); file.initWithPath("C:\\hiberfil.sys"); file.isWritable()
NS_ERROR_FILE_NOT_FOUND on line 3: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsILocalFile.isWritable]

This should not throw an error, it should return false.

3) For files that have no permission (security):

var file = Components.classes['@mozilla.org/file/local;1'].createInstance(Components.interfaces.nsILocalFile); 
file.initWithPath("C:\\restricted.txt"); 
file.isWritable();
> true

This should not return true, it should return false

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Patch for the 3 problems mentioned for nsLocalFile::IsWritable

Setting you as the reviewer since there are 2 similar bugs I already requested from you.

Comment 2

[Benjamin Smedberg]

I don't think that #2 is correct: if a file would normally be writeable but is currently locked, we should still return `true`.

Comment 3

[Brian R. Bondy [:bbondy]]

OK I'll change that. The documentation is vague on if it means currently writeable or writeable in general regarding permissions. They are the same on other platforms more or less (other than if you mount your filesystem with mandatory locks), but differ quite a bit on Windows.  

I'll clarify on this point at https://developer.mozilla.org/en/nsIFile

Comment 4

[Brian R. Bondy [:bbondy]]

Attachment: Patch for the 3 problems mentioned for nsLocalFile::IsWritable. v2.

Works the same as before except it will return true for #2. 
I also tested other cases and they work as expected.

Summary:
- Normal file: IsWritable -> true
- Read only file share's file: IsWritable -> false
- Exclusively opened or locked but normally writeable: IsWritable -> true
- Exclusively opened or locked and read only flag: IsWritable -> false
- Exclusively opened or locked and no permissions: IsWritable -> false
- No write permissions on local file: IsWritable -> false
- Read only flag set on file: IsWritable -> false

Comment 5

[Benjamin Smedberg]

Comment on attachment 558896 [details] [diff] [review]
Patch for the 3 problems mentioned for nsLocalFile::IsWritable. v2.

+    if(*aIsWritable) {

unsnuggle the if (twice in this patch)

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: Patch for the 3 problems mentioned for nsLocalFile::IsWritable. v3.

Old habits die hard.  Fixed the snuggled parentheses and a couple others in the same file not related to my changes.

Comment 7

[Brian R. Bondy [:bbondy]]

I put a reminder in my calendar to update the documentation regarding the meaning of isWritable once this reaches release.  If you'd prefer I update it now just let me know.

Comment 8

[Benjamin Smedberg]

Do it now, with release-specific notes as necessary. Sheppy may add it to the "for developers" page too, although this is minor enough that it's probably not worth it.

Comment 9

[Brian R. Bondy [:bbondy]]

Updated doc here:
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIFile#isWritable()

Comment 10

[Brian R. Bondy [:bbondy]]

Removing dev-doc-needed keyword since it's already updated as per Comment 9.

Comment 11

[Brian R. Bondy [:bbondy]]

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=ed47da6511a5

Comment 12

[Brian R. Bondy [:bbondy]]

Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/225001455808

Comment 13

[Ed Morley [:emorley]]

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