Last Comment Bug 682571 - nsLocalFile::isWritable behaves wrongly on Windows
: nsLocalFile::isWritable behaves wrongly on Windows
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla9
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
Depends on: 300692 429484
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-27 09:27 PDT by Brian R. Bondy [:bbondy]
Modified: 2011-09-22 17:45 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for the 3 problems mentioned for nsLocalFile::IsWritable (2.03 KB, patch)
2011-08-29 08:06 PDT, Brian R. Bondy [:bbondy]
benjamin: review-
Details | Diff | Review
Patch for the 3 problems mentioned for nsLocalFile::IsWritable. v2. (2.41 KB, patch)
2011-09-07 11:45 PDT, Brian R. Bondy [:bbondy]
benjamin: review+
Details | Diff | Review
Patch for the 3 problems mentioned for nsLocalFile::IsWritable. v3. (3.88 KB, patch)
2011-09-07 12:14 PDT, Brian R. Bondy [:bbondy]
netzen: review+
Details | Diff | Review

Description Brian R. Bondy [:bbondy] 2011-08-27 09:27:15 PDT
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] 2011-08-29 08:06:09 PDT
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.
Comment 2 Benjamin Smedberg [:bsmedberg] 2011-09-06 13:19:41 PDT
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] 2011-09-06 13:33:54 PDT
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] 2011-09-07 11:45:42 PDT
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
Comment 5 Benjamin Smedberg [:bsmedberg] 2011-09-07 12:00:44 PDT
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] 2011-09-07 12:14:48 PDT
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.
Comment 7 Brian R. Bondy [:bbondy] 2011-09-07 12:15:59 PDT
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 [:bsmedberg] 2011-09-07 12:41:39 PDT
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] 2011-09-07 13:40:38 PDT
Updated doc here:
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIFile#isWritable()
Comment 10 Brian R. Bondy [:bbondy] 2011-09-21 06:43:43 PDT
Removing dev-doc-needed keyword since it's already updated as per Comment 9.
Comment 11 Brian R. Bondy [:bbondy] 2011-09-21 10:40:57 PDT
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=ed47da6511a5
Comment 12 Brian R. Bondy [:bbondy] 2011-09-22 06:24:13 PDT
Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/225001455808
Comment 13 Ed Morley [:emorley] 2011-09-22 17:45:17 PDT
https://hg.mozilla.org/mozilla-central/rev/225001455808

Note You need to log in before you can comment on or make changes to this bug.