373640 - nsIFile.parent throws an exception on reaching top of file system -- should return null

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jeff Walden [:Waldo]]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/io/nsIFile.idl&rev=1.63&mark=324-327#320

The specification of nsIFile.parent in IDL makes no mention whatsoever of it possibly throwing exceptions.  Therefore, except in very anomalous circumstances such as OOM, using it should never throw an exception.  Currently, it seems that on Windows using .parent can result in an unrecognized-path exception, which is plain wrong, given the (minimal) specification given in IDL comments.

This was discovered when an automated test for bug 332389 broke the Windows testing tinderbox on the assumption that the Windows .parent implementation matched the specification.  We absolutely *should not* be having problems conforming to the basic functionality in the nsI(Local)?File specification on any of the major platforms.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Slightly-tested patch

I added in a couple lines of effort to avoid copying mWorkingPath when possible, but those changes aren't strictly necessary.

I haven't tested this on Windows where it's most likely to fail.  (It passes on Linux, for what little it's worth.)  I did check that the path of a file object representing C:\ is two characters and that the trailing slash is removed on a Windows machine, which matches what the code looks like it does.

Comment 2

[Wladimir Palant]

var file = Components.classes["@mozilla.org/file/directory_service;1"]
                     .getService(Components.interfaces.nsIProperties)
                     .get("Drivs", Components.interfaces.nsILocalFile);
alert(file.path);    // Shows empty string
alert(file.parent);  // Throws NS_ERROR_FILE_UNRECOGNIZED PATH

So I guess it should be:

+    if (mWorkingPath.Length() == 2) {

This will fix another bug as well:

var file = Components.classes["@mozilla.org/file/local;1"]
                     .createInstance(Components.interfaces.nsILocalFile);
file.initWithPath("\\\\");
alert(file.path);          // Shows "\"
alert(file.exists());      // Shows true
alert(file.parent.path);   // Shows "\\."
alert(file.parent.parent); // Shows null

With this patch we will return null immediately without going over the bogus "\\." path. Though not stripping the second slash in this case wouldn't be wrong either...

Comment 3

[Wladimir Palant]

Sorry, I meant it should be:

+    if (mWorkingPath.Length() <= 2) {

Comment 4

[Benjamin Smedberg]

Comment on attachment 258374 [details] [diff] [review]
Slightly-tested patch

It's too bad we don't have a simpler way to check for which platform we're on within a unit test.

Comment 5

[Jeff Walden [:Waldo]]

Fixed.

I filed bug 375259 for the "Drivs" issue (where an uninitialized file is returned) and bug 375262 for handling UNC paths better, both from comment 2.