nsLocalFile::isWritable behaves wrongly on Windows

RESOLVED FIXED in mozilla9

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

Trunk
mozilla9
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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
(Assignee)

Comment 1

6 years ago
Created attachment 556553 [details] [diff] [review]
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.
Attachment #556553 - Flags: review?(benjamin)
I don't think that #2 is correct: if a file would normally be writeable but is currently locked, we should still return `true`.
Attachment #556553 - Flags: review?(benjamin) → review-
(Assignee)

Comment 3

6 years ago
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
(Assignee)

Comment 4

6 years ago
Created attachment 558896 [details] [diff] [review]
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
Attachment #556553 - Attachment is obsolete: true
Attachment #558896 - Flags: review?(benjamin)
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)
Attachment #558896 - Flags: review?(benjamin) → review+
(Assignee)

Comment 6

6 years ago
Created attachment 558904 [details] [diff] [review]
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.
Attachment #558896 - Attachment is obsolete: true
Attachment #558904 - Flags: review+
(Assignee)

Comment 7

6 years ago
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.
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.
Keywords: dev-doc-needed
(Assignee)

Comment 9

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

Comment 10

6 years ago
Removing dev-doc-needed keyword since it's already updated as per Comment 9.
Keywords: dev-doc-needed
(Assignee)

Comment 11

6 years ago
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=ed47da6511a5
(Assignee)

Comment 12

6 years ago
Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/225001455808
https://hg.mozilla.org/mozilla-central/rev/225001455808
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.