Closed Bug 960046 Opened 6 years ago Closed 6 years ago

[OS.File] makeDir should not fail if the directory is a root

Categories

(Toolkit :: OS.File, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox28 + fixed
firefox29 + fixed
firefox30 + fixed
b2g-v1.3 --- fixed

People

(Reporter: Yoric, Assigned: k)

References

Details

(Whiteboard: [Async:ready][mentor=Yoric][lang=js])

Attachments

(1 file, 5 obsolete files)

At the moment, under Windows, if we call OS.File.makeDir("C:\\"), the call will fail because we do not have write access to "C:\\". However, by definition, makeDir should succeed if the directory exists. We should fix this.

This should be quite easy. The file to modify is:
http://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_win_front.jsm?from=osfile_win_front.jsm#441

What we need to do is change makeDir as follows:
- if |result| is |true|, succeed;
- otherwise, if "ignoreExisting" is in options and if |options.ignoreExisting| is false, fail;
- otherwise, if ctypes.winLastError == Const.ERROR_ALREADY_EXISTS, succeed;
- otherwise, if |ctypes.winLastError == Const.ERROR_ACCESS_DENIED|,
 if OS.Path.winIsAbsolute(path) and OS.Path.normalize(path) == OS.Path.winGetDrive(path), succeed;
- otherwise, fail.
Unless someone else provides a patch in the meantime, I guess I'll take this in the next few days as this prevents downloads to be automatically saved to a root directory by default.
Attached patch bug-960046 (obsolete) — Splinter Review
Like this?

File.makeDir = function makeDir(path, options = {}) {
       let security = options.winSecurity || null;
       let result = WinFile.CreateDirectory(path, security); 
       if (!result) {
         if (("ignoreExisting" in options) && !options.ignoreExisting) {
           throw new File.Error("makeDir");
         }
         else if (ctypes.winLastError == Const.ERROR_ALREADY_EXISTS) {
           return;
         }
         else if (ctypes.winLastError == Const.ERROR_ACCESS_DENIED && 
                  OS.Path.winIsAbsolute(path) && 
                  OS.Path.normalize(path) == OS.Path.winGetDrive(path) ) {
           return;
         }
         throw new File.Error("makeDir");
       }
     };
Attachment #8360960 - Flags: review?(dteller)
Assignee: nobody → k
Whiteboard: [Async][mentor=Yoric][lang=js] → [Async:ready][mentor=Yoric][lang=js]
Comment on attachment 8360960 [details] [diff] [review]
bug-960046

Review of attachment 8360960 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +449,5 @@
> +           return;
> +         }
> +         else if (ctypes.winLastError == Const.ERROR_ACCESS_DENIED && 
> +                  OS.Path.winIsAbsolute(path) && 
> +                  OS.Path.normalize(path) == OS.Path.winGetDrive(path) ) {

David, this will actually not work when path == "C:\\", as opposed to just "C:". How could we fix that?

Side-note: there are spaces at end of line, should be removed in the next iteration of the patch.
(In reply to :Paolo Amadini from comment #3)
> David, this will actually not work when path == "C:\\", as opposed to just
> "C:". How could we fix that?

Actually, I've just verified that, since we are calling CreateDirectoryW directly, "C:" is interpreted as "current directory on drive C:", and returns ERROR_ALREADY_EXISTS.

Calling CreateDirectoryW with "C:\\" as the path is interpreted as the root of the drive, and returns ERROR_ACCESS_DENIED.
Depends on: 960962
Comment on attachment 8360960 [details] [diff] [review]
bug-960046

Review of attachment 8360960 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +439,5 @@
>        * already exists. |true| by default
>        */
>       File.makeDir = function makeDir(path, options = {}) {
>         let security = options.winSecurity || null;
> +       let result = WinFile.CreateDirectory(path, security); 

Nit: whitespace at the end of the line, here and on a few other lines.

@@ +444,1 @@
>         if (!result) {

To simplify code, let's make this

if (result) {
  return;
}

@@ +444,5 @@
>         if (!result) {
> +         if (("ignoreExisting" in options) && !options.ignoreExisting) {
> +           throw new File.Error("makeDir");
> +         }
> +         else if (ctypes.winLastError == Const.ERROR_ALREADY_EXISTS) {

This |else| is not needed.

@@ +447,5 @@
> +         }
> +         else if (ctypes.winLastError == Const.ERROR_ALREADY_EXISTS) {
> +           return;
> +         }
> +         else if (ctypes.winLastError == Const.ERROR_ACCESS_DENIED && 

Same here, no |else|.

@@ +449,5 @@
> +           return;
> +         }
> +         else if (ctypes.winLastError == Const.ERROR_ACCESS_DENIED && 
> +                  OS.Path.winIsAbsolute(path) && 
> +                  OS.Path.normalize(path) == OS.Path.winGetDrive(path) ) {

This requires documentation.
Also, as noticed by Paolo, I suggested a wrong algorithm. Instead of calling winIsAbsolute() and normalize(), we should call OS.Path.split() and check that
- |absolute| is |true|;
- and there is only one component.
Attachment #8360960 - Flags: review?(dteller) → feedback+
Attached patch bug-960046.patch (obsolete) — Splinter Review
Had the whitespaces hidden, sorry.
Attachment #8360960 - Attachment is obsolete: true
Attachment #8362566 - Flags: review?(dteller)
Comment on attachment 8362566 [details] [diff] [review]
bug-960046.patch

Review of attachment 8362566 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Could you add a test before I mark this r+?

Side-note: Could you add a human-readable title and a version number to your patch when you upload it?

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +454,5 @@
> +         return;
> +       }
> +
> +       let splitPath = OS.Path.split(path);
> +       //One component and an absolute path implies a directory root.

Nit: Please add one space after "//".
Attachment #8362566 - Flags: review?(dteller) → feedback+
What kind of test and how?
Status: NEW → ASSIGNED
Flags: needinfo?(dteller)
I'd like you to write a xpcshell test. See toolkit/components/osfile/tests/xpcshell for a few examples (don't forget to add your test to xpcshell.ini). The test only needs to call OS.File.makeDir("C:\\") under Windows and OS.File.makeDir("/") under Unix, while we're at it.
Flags: needinfo?(dteller)
I see.

May I put the test into already existent test_makeDir.js?
Flags: needinfo?(dteller)
(In reply to k from comment #10)
> I see.
> 
> May I put the test into already existent test_makeDir.js?

Good idea.
Flags: needinfo?(dteller)
Attached patch makeDirFixAndTest1.patch (obsolete) — Splinter Review
I tried it this way.
Attachment #8362566 - Attachment is obsolete: true
Attachment #8363216 - Flags: review?(dteller)
Attached patch makeDirFixAndTest2.patch (obsolete) — Splinter Review
Sorry, used qnew and not qrefresh...
Attachment #8363216 - Attachment is obsolete: true
Attachment #8363216 - Flags: review?(dteller)
Attachment #8363238 - Flags: review?(dteller)
Comment on attachment 8363238 [details] [diff] [review]
makeDirFixAndTest2.patch

Review of attachment 8363238 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks for the patch.
Let's see if this passes tests on all platforms: https://tbpl.mozilla.org/?tree=Try&rev=5470716e3b5b

Nit: In the future, could you make the patch name human-readable?

::: toolkit/components/osfile/tests/xpcshell/test_makeDir.js
@@ +57,5 @@
> +  // Make a root directory that already exists
> +  if (OS.Constants.Win) {
> +    dir = "C:\\";
> +  }
> +  else {

Nit:
 } else {
Attachment #8363238 - Flags: review?(dteller) → review+
Looks like the tests don't pass. Can you fix the issues?
Don't know if I can, but I will try.

Got me a Win7 box and testing it there.
Under MacOS X, the call returned EISDIR, so you need to handle this case, too (that's in osfile_unix_front.jsm).

Under Windows, it seems that the test is incorrect.
Hum, Okay.

It was a bit of an adventure to build Firefox under Windows. Had to set up a new, clean, Win7 VM, which I got done today.

I will look into it. 

Thank you.
Can't get the tests running unter Windows7.
Somehow most of them time-out and at one point the whole thing just stops.
Also, I don't got a OSX to test the other error.

But if you have an idea what I should change, I can do it.

Sorry :\
Flags: needinfo?(dteller)
To run the relevant test under Windows, just launch

./mach xpcshell-test toolkit/components/osfile/tests/xpcshell/test_makeDir.js

For MacOS X, you need to patch makeDir in osfile_unix_front.jsm. There is a test 
  ctypes.errno == Const.EEXIST
 which you should replace with 
  ctypes.errno == Const.EEXIST || ctypes.errno == Const.EISDIR
Flags: needinfo?(dteller)
Added the fix for OSX.

But I still getting errors for the xpcshell under win7:

> From _tests: Kept 14430 existing; Added/updated 17; Removed 0 files and 0 directories.
>  0:08.19 Found node at c:\mozilla-source\mozilla-central\testing\xpcshell\node
>  0:08.20 Found moz-spdy at c:\mozilla-source\mozilla-central\testing\xpcshell\moz-spdy\moz-spdy.js
>  0:08.21 Could not run moz-spdy server: [Error 193] %1 is not a valid Win32 application
>  0:08.21 Found moz-http2 at c:\mozilla-source\mozilla-central\testing\xpcshell\moz-http2\moz-http2.js
>  0:08.22 Could not run moz-http2 server: [Error 193] %1 is not a valid Win32 application
>  0:08.22 INFO | Using at most 4 threads.
>  0:08.22 TEST-INFO | test_makeDir.js | Test failed or timed out, will retry.
>  0:08.22 Retrying tests that failed when run in parallel.
>  0:08.23 INFO | Following exceptions were raised:
>  0:08.23 Traceback (most recent call last):
>   File "c:\mozilla-source\mozilla-central\testing/xpcshell\runxpcshelltests.py", line 166, in run
>     self.run_test()
>   File "c:\mozilla-source\mozilla-central\testing/xpcshell\runxpcshelltests.py", line 558, in run_test
>     self.test_object['here'])
> Exception: tests_root_dir is not a parent path of c:/mozilla-source/mozilla-central/obj-i686-pc-mingw32/_tests/xpcshell/toolkit/components/osfile/tests/xpcshell
> 
> 
> Error running mach:
> 
>     ['xpcshell-test', 'toolkit/components/osfile/tests/xpcshell/test_makeDir.js'
> ]
> 
> The error occurred in code that was called by the mach command. This is either a bug in the called code itself or in the way that mach is calling it.
> 
> You should consider filing a bug for this issue.
> 
> If filing a bug, please include the full output of mach, including this error message.
> 
> The details of the failure are as follows:
> 
> Exception: tests_root_dir is not a parent path of c:/mozilla-source/mozilla-central/obj-i686-pc-mingw32/_tests/xpcshell/toolkit/components/osfile/tests/xpcshell
> 
> 
>   File "c:\mozilla-source\mozilla-central\testing/xpcshell/mach_commands.py", line 339, in run_xpcshell_test
>     return xpcshell.run_test(**params)
>   File "c:\mozilla-source\mozilla-central\testing/xpcshell/mach_commands.py", line 102, in run_test
>     return self._run_xpcshell_harness(**args)
>   File "c:\mozilla-source\mozilla-central\testing/xpcshell/mach_commands.py", line 187, in _run_xpcshell_harness
>     result = xpcshell.runTests(**filtered_args)
>   File "c:\mozilla-source\mozilla-central\testing/xpcshell\runxpcshelltests.py", line 1504, in runTests
>     raise exceptions[0]
Flags: needinfo?(dteller)
I am not familiar with that error. You should ask around on #introduction.
Flags: needinfo?(dteller)
Okay, got the test running. (replaced \ with / in tests_root_dir)

For test purposes I stripped it down to

> add_task(function(){
>   yield OS.File.makeDir("C:\\");
> });

This throws the error.

But since I can't debug this xpcshell and all my attempts to generate some output in JS are muted, I feel unable to trace this problem any further :(
Flags: needinfo?(dteller)
(In reply to k from comment #23)
> Okay, got the test running. (replaced \ with / in tests_root_dir)
> 
> For test purposes I stripped it down to
> 
> > add_task(function(){
> >   yield OS.File.makeDir("C:\\");
> > });
> 
> This throws the error.

Yes, that was to be expected from the Try Server error message.

> But since I can't debug this xpcshell and all my attempts to generate some
> output in JS are muted, I feel unable to trace this problem any further :(

It seems that the non-failure-condition we have added in osfile_win_front.jsm is not sufficient. You should check the value of splitPath and see where we made a mistake.

To print something from JS, use function |dump()|, so to print |splitPath| do something along the lines of
 dump("splitPath: " + JSON.stringify(splitPath) + "\n")
Flags: needinfo?(dteller)
Yes, the problem was there

> C:\\ got split to ["C:",""]
> C:\\test\\ got split to ["C:","test",""]
> C:\\test got split to ["C:","test"]

Is it safe to assume paths always have trailing slashes? (so I could change the |=== 1| to |<= 2|)
Flags: needinfo?(dteller)
No, it's not safe. However, you could check that there is at most one non-empty item. This will require documentation, of course.
Flags: needinfo?(dteller)
Now I'm dropping the last component, if it's empty. This way paths with and without trailing slashes can be handled the same.

Also added the fix you mentioned for OSX.
Attachment #8363238 - Attachment is obsolete: true
Attachment #8368028 - Flags: review?(dteller)
Comment on attachment 8368028 [details] [diff] [review]
added fix for OSX and Windows for failing makeDir on root directories and added a new Windows test

Review of attachment 8368028 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +451,5 @@
>         }
> +
> +       if (ctypes.winLastError == Const.ERROR_ALREADY_EXISTS) {
> +         return;
> +       }

Could you add/move up here the comment here explaining why you're doing all of this?

::: toolkit/components/osfile/tests/xpcshell/test_makeDir.js
@@ +57,5 @@
> +  // Make a root directory that already exists
> +  if (OS.Constants.Win) {
> +    dir = "C:\\";
> +  }
> +  else {

Nit:
  } else {

@@ +61,5 @@
> +  else {
> +    dir = "/";
> +  }
> +
> +  yield OS.File.makeDir(dir);

Could you take the opportunity to test what happens when we attempt to create "C:\\Program Files" under Windows or "/tmp" under Linux?

If this succeeds, add that check in test_makeDir.js. Otherwise, we'll keep this for a followup bug.
Attachment #8368028 - Flags: review?(dteller) → review+
These tests passed under Linux and Windows.
Added them to tests_makeDir.js
Attachment #8368028 - Attachment is obsolete: true
Attachment #8368606 - Flags: review?(dteller)
Comment on attachment 8368606 [details] [diff] [review]
added fix for OSX and Windows for failing makeDir on root directories and added a new Windows test

Review of attachment 8368606 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thanks for the patch.
Try: https://tbpl.mozilla.org/?tree=Try&rev=2d1b59bf6002
Attachment #8368606 - Flags: review?(dteller) → review+
After all this stuff I feel like an imbecile, haha.
But I learned some stuff, thank you :)
https://hg.mozilla.org/integration/fx-team/rev/d78c5dc30bab
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [Async:ready][mentor=Yoric][lang=js] → [Async:ready][mentor=Yoric][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d78c5dc30bab
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Async:ready][mentor=Yoric][lang=js][fixed-in-fx-team] → [Async:ready][mentor=Yoric][lang=js]
Target Milestone: --- → mozilla29
Target Milestone: mozilla29 → mozilla30
Comment on attachment 8368606 [details] [diff] [review]
added fix for OSX and Windows for failing makeDir on root directories and added a new Windows test

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug os.file.
User impact if declined: Windows users cannot download files to a root directory.
Testing completed (on m-c, etc.): Has been on m-c for a few days.
Risk to taking this patch (and alternatives if risky): Can't think of any.
String or IDL/UUID changes made by this patch:
Attachment #8368606 - Flags: approval-mozilla-beta?
Attachment #8368606 - Flags: approval-mozilla-aurora?
Attachment #8368606 - Flags: approval-mozilla-beta?
Attachment #8368606 - Flags: approval-mozilla-beta+
Attachment #8368606 - Flags: approval-mozilla-aurora?
Attachment #8368606 - Flags: approval-mozilla-aurora+
This bug appears to not have fixed the issue of trying to assign Driveletter:\ as the download directory.

It will allow setting for example: k:\, but a download still fails-back to Maindrive letter,username, downloads.
I debugged the latest patch and the issue is that, while the new makeDir now works for the case described in comment 3, it doesn't work anymore for the "D:" case, which is the format used by the download preferences to store the root directory name.

I guess, but I couldn't verify, that changing this check for "absolute" to a check for "winDrive" can fix the issue:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_win_front.jsm#467
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Can we fix that issue in a followup, please?
Depends on: 973931
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #39)
> Can we fix that issue in a followup, please?

Filed bug 973931.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.