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.

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

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.

Had the whitespaces hidden, sorry.

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

What kind of test and how?

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.

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

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

I tried it this way.

Sorry, used qnew and not qrefresh...

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 {

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

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

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]

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

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

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

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

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

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

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

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

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

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:

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

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

Can we fix that issue in a followup, please?

(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #39) > Can we fix that issue in a followup, please? Filed bug 973931.