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

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 1

[:Paolo Amadini]

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.

Comment 2

[k]

Attachment: bug-960046

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");
       }
     };

Comment 3

[:Paolo Amadini]

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.

Comment 4

[:Paolo Amadini]

(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.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 6

[k]

Attachment: bug-960046.patch

Had the whitespaces hidden, sorry.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

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 "//".

Comment 8

[k]

What kind of test and how?

Comment 9

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 10

[k]

I see.

May I put the test into already existent test_makeDir.js?

Comment 11

[David Teller [:Yoric] - still alive but not very active]

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

Good idea.

Comment 12

[k]

Attachment: makeDirFixAndTest1.patch

I tried it this way.

Comment 13

[k]

Attachment: makeDirFixAndTest2.patch

Sorry, used qnew and not qrefresh...

Comment 14

[David Teller [:Yoric] - still alive but not very active]

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 {

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Looks like the tests don't pass. Can you fix the issues?

Comment 16

[k]

Don't know if I can, but I will try.

Got me a Win7 box and testing it there.

Comment 17

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 18

[k]

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.

Comment 19

[k]

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 :\

Comment 20

[David Teller [:Yoric] - still alive but not very active]

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

Comment 21

[k]

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]

Comment 22

[David Teller [:Yoric] - still alive but not very active]

I am not familiar with that error. You should ask around on #introduction.

Comment 23

[k]

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 :(

Comment 24

[David Teller [:Yoric] - still alive but not very active]

(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")

Comment 25

[k]

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|)

Comment 26

[David Teller [:Yoric] - still alive but not very active]

No, it's not safe. However, you could check that there is at most one non-empty item. This will require documentation, of course.

Comment 27

[k]

Attachment: added fix for OSX and Windows for failing makeDir on root directories and added a new Windows test

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.

Comment 28

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 29

[k]

Attachment: added fix for OSX and Windows for failing makeDir on root directories and added a new Windows test

These tests passed under Linux and Windows.
Added them to tests_makeDir.js

Comment 30

[David Teller [:Yoric] - still alive but not very active]

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

Comment 31

[k]

After all this stuff I feel like an imbecile, haha.
But I learned some stuff, thank you :)

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/d78c5dc30bab

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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

Comment 34

[David Teller [:Yoric] - still alive but not very active]

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:

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/48461ef116f2
https://hg.mozilla.org/releases/mozilla-beta/rev/f4b5ae829393

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/f4b5ae829393

Comment 37

[Jim Jeffery not reading bug-mail 1/2/11]

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.

Comment 38

[:Paolo Amadini]

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

Comment 39

[:Gavin Sharp [email: [e-mail]]]

Can we fix that issue in a followup, please?

Comment 40

[:Paolo Amadini]

(In reply to :Gavin Sharp (email [e-mail]) from comment #39)
> Can we fix that issue in a followup, please?

Filed bug 973931.