332389 - nsLocalFile::GetParent() doesn't return nsnull at top of volume

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Wladimir Palant]

I was investigating why my extension would hang when traversing upwards through the directory tree. It came out that on Mac OS X nsIFile.parent always returns a value, even on top of the volume (I was testing with Firefox 1.5 on Mac OS X 10.3, but this code hasn't been touched in a while, so the problem should occur on trunk as well). This is a regression, bug 133617 refers to this issue as fixed. Execute the following code on Mac OS X:

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

This will show: /..
That's clearly not what it should be, see also comment in nsLocalFile::GetParent (http://lxr.mozilla.org/mozilla/source/xpcom/io/nsLocalFileOSX.cpp#1058). There should be an exception because file.parent should be null in this case. It seems that CFURLCreateCopyDeletingLastPathComponent doesn't return null on top of volume after all (I couldn't find any documentation stating what the return value of this function should be in such a case).

My hack to work around this problem was to break out of the loop when path is "/". But I think this should somehow be fixed in Gecko as well...

Comment 1

[Robert Sayre]

http://darwinsource.opendarwin.org/10.4.2/CF-368.11/URL.subproj/CFURL.c

looks like CFURLCreateCopyDeletingLastPathComponent only returns NULL if the CFURL is a network URL with no path component.

Comment 2

[Wladimir Palant]

Robert, thanks for pointing me into the right direction. The way I read this code it will almost always do what we need (remove the last path component). The exceptions are the paths "", ".", ".." and "/" where it adds two dots. Since nsLocalFile cannot have relative paths, only the last case is problematic. We could add a special check for this case and return nsnull if mBaseRef is "/". A more general solution might be comparing the length of parentURLRef with the length of mBaseRef - we should only return a value if the former is smaller.

I am tending towards the second solution, the first one might be broken by changes in the Darwin code. The problem is - I cannot compile Firefox on a Mac. If I write a patch, will you be able to test/review it?

Comment 3

[Robert Sayre]

(In reply to comment #2)
>
> If I write a patch, will you be able to test/review it?

I touched this file last, so it should be kosher to do r=sayrer? / sr=bsmedberg?, so we get module owner approval. 

Comment 4

[Wladimir Palant]

Attachment: Proposed patch

This will make the method only accept the result of CFURLCreateCopyDeletingLastPathComponent if it is shorter than what we had before - an increase in length indicates that there is no parent. I will also attach a diff with -w to make reviewing easier.

Comment 5

[Wladimir Palant]

Attachment: Same patch with -w

Comment 6

[Robert Sayre]

Comment on attachment 256467 [details] [diff] [review]
Same patch with -w

>Index: nsLocalFileOSX.cpp
>
>     }
>+    }

Indent the nested block, please.

Comment 7

[Robert Sayre]

Comment on attachment 256467 [details] [diff] [review]
Same patch with -w

oops, wrong patch.

Comment 8

[Benjamin Smedberg]

Comment on attachment 256466 [details] [diff] [review]
Proposed patch

I'm increasingly thinking that we should replace most of the guts of nsLocalFileOSX with nsLocalFileUnix. In any case, I defer to Josh as mac guru.

Comment 9

[Josh Aas]

Comment on attachment 256466 [details] [diff] [review]
Proposed patch

Symlinks aren't getting resolved, but if they were then parent paths could potentially be longer than their children. To be safe, let's check for "/" as in addition to the path length check.

Comment 10

[Wladimir Palant]

I don't see a point in checking for "/" *in addition* - we know that this path cannot have a parent, right? So if you are sure that this will always be the only problematic case then we can simply check for "/" and return nsnull here.

Comment 11

[Wladimir Palant]

Attachment: Patch v2, Comparing path with /

This is untested again - I cannot compile on a Mac.

Comment 12

[Josh Aas]

Attachment: Patch v1 w/warning fix

This is the same as the first patch, plus a warning fix. I like the first patch better afterall.

Comment 13

[Josh Aas]

There were also trailing CRs in the first patch, mine gets rid of that.

Comment 14

[Josh Aas]

Patch v1 w/warning fix landed on trunk.

Comment 15

[Wladimir Palant]

Attachment: xpcshell testcase

Proposed /mozilla/xpcom/tests/unit/test_bug332389.js file - tests whether following the .parent chain terminates.

Comment 16

[Josh Aas]

Wladimir - how do you run that test?

Comment 17

[Jeff Walden [:Waldo]]

Same way you run all xpcshell tests -- drop it in the location mentioned, do a |make| in xpcom/tests so that it gets propagated to the right locations in the objdir, and then run |make check| in xpcom/tests in your objdir.

Comment 18

[Josh Aas]

That doesn't seem to be working in this case. I don't see any results.

Comment 19

[Jeff Walden [:Waldo]]

Odd -- works for me.

However, when looking at the test I see:

  for (var i = 0; i < 100; i++) {
    if (f == null) {
      terminated = true;
      break;
    }
    try {
      f = f.parent
    }
    catch (e) {
      // Exception means we reached the top of a volume, terminate
      f = null;
    }
  }

f.parent throwing an exception is not expected behavior. When f is the top directory, f.parent should be null and not throw, and on relooping the null will be detected and the test will pass.  The try/catch is unnecessary -- just do |f = f.parent;| directly (and do include the semicolon).

Comment 20

[Josh Aas]

Comment on attachment 257957 [details]
xpcshell testcase

I got it working now. Thanks!

Comment 21

[Josh Aas]

test checked in

Comment 22

[Jeff Walden [:Waldo]]

Could you remove the try/catch and add the semicolon, please, per comment 19?

Comment 23

[Josh Aas]

try/catch removed, semicolon added, checked in

Comment 24

[Wladimir Palant]

Jeff, exception is the expected behavior on Windows. This test isn't Mac only. Now if you think that GetParent on Windows shouldn't throw an exception feel free to file a bug.

Josh, please check in the original patch, with the try/catch.

Comment 25

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

I've reverted the test to the original version, per comment 24.

Comment 26

[Jeff Walden [:Waldo]]

(In reply to comment #24)
> Jeff, exception is the expected behavior on Windows.

The IDL documentation for nsIFile flatly contradict this, mentioning nothing about accessing the attribute throwing any exceptions and explicitly stating that |f.parent == null| when |f| represents the topmost directory:

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

I filed bug 373640 to fix this issue.