827206 - OS.File.exists() false on Windows if path has spaces

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Gregory Szorc [:gps]]

I wrote some code that did OS.File.exists(path) where path had spaces. xpcshell tests passed on Linux and OS X but failed on Windows! I was able to reproduce on my local Windows machine.

Immediately after the failed call to OS.File.exists() I instantiated an nsIFile from the same path. nsIFile.exists() reported true. Huh.

The directory was created just a few ticks earlier using nsIFile. I suppose there could be a race condition between nsIFile.create() and OS.File. But, I thought nsIFile I/O was synchronous. So, I don't think there should be any difference between nsIFile and OS.File.

Comment 1

[Gregory Szorc [:gps]]

Oh, so I guess OS.File.exists() only works for files? I'm able to open the directory just fine with OS.File.DirectoryIterator()! What's the preferred way to check if a directory exists? Try to open a DirectoryIterator and catch errors? I see that throws a WorkerErrorEvent if the directory doesn't exist. That seems weird. But it is catchable.

Could we get an OS.Path.exists() or equivalent that works on files/directories/symlinks/devices, etc?

Comment 2

[Gregory Szorc [:gps]]

So, you can open a DirectoryIterator on a non-existent directory without throwing. That kinda makes since since I assume the opendir() or equivalent is not on the main thread.

But, iterator.next() yields a rejected promise that is a WorkerErrorEvent!

print(ex): -> [object WorkerErrorEvent]
print(ex.message): -> TypeError

Could we get DirectoryIterator to do something sane for directories that don't exist?

Feel free to close/split/repurpose whatever this bug, as I've reported a number of different issues.

Comment 3

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

The expected way to test whether a directory exists is to |OS.File.stat()| it and then check field |isDir|. The behavior of |DirectoryIterator| is mostly expected. As for the behavior of |exists()| on directories, it is not specified, but that really should be documented.

Now, |WorkerErrorEvent|? That's a bug.

Comment 4

[Richard Newman [:rnewman]]

(In reply to David Rajchenbach Teller [:Yoric] <away until Jan 7th> from comment #3)
> As for the behavior of |exists()| on directories, it is not
> specified, but that really should be documented.

`exists` really shouldn't call an unmodified `open` then `close`, as it does after Bug 799226. (It seems horribly inefficient, as well as having the side-effect of modifying filesystem timestamps!)

It should call `stat` (which opens in FILE_STAT_MODE, which you suggest will work), which would then make it work for all file types, no?

Comment 5

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

Good point.

Comment 6

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

Attachment: OS.File.exists should now behave correctly with directories under Windows

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 698556 [details] [diff] [review]
OS.File.exists should now behave correctly with directories under Windows

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

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ +790,5 @@
> +
> +  let dir_name = OS.Path.join("chrome", "toolkit", "components" ,"osfile",
> +                               "tests", "mochi");
> +  ok(OS.File.exists(dir_name), "test_exists_file: directory exists");
> +  ok(!OS.File.exists(dir_name) + ".tmp", "test_exists_file: directory does not exist");

Technically, this is just checking for non-existence of a file (directory or not), since there's no way to indicate that a path should be interpreted as a directory, right?

Comment 8

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

(In reply to Nathan Froyd (:froydnj) from comment #7)
> Technically, this is just checking for non-existence of a file (directory or
> not), since there's no way to indicate that a path should be interpreted as
> a directory, right?

Well, without the patch, this test would fail under Windows, because directories cannot be opened as regular files.

Comment 9

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=1b80f6d2f574

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b34098fb12c7

Comment 11

[Ed Morley [:emorley]]

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