446692 - do_get_file fails on Windows with ".." in the file location.

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Mark Banner (:standard8)]

Now we are switching to comm-central, the windows buildbots (Thunderbird and SeaMonkey) are showing consistent errors on "make check":

TEST-UNEXPECTED-FAIL | ../../../mozilla/_tests/xpcshell-simple/test_mailnewsbase/unit/test_searchJunk.js | test failed, see log
../../../mozilla/_tests/xpcshell-simple/test_mailnewsbase/unit/test_searchJunk.js.log:
>>>>>>>
*** test pending
Wants directory: SysD
Wants directory: MailD
Wants directory: MFCaF
Wants directory: DefRt
*** test pending
[Exception... "Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsILocalFile.append]"  nsresult: "0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH)"  location: "JS frame :: d:/builds/slave/comm-central-win32/build/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_get_file :: line 178"  data: no]
*** FAIL ***

<<<<<<<

If someone on Windows has chance to try this, then it would be interesting to look in:

http://mxr.mozilla.org/comm-central/source/mozilla/tools/test-harness/xpcshell-simple/head.js#155

at the do_get_file function, and find out what lf.path is before the for loop, and what is the value of comps[i] when it fails.

Comment 1

[Kent James (:rkent)]

In passing, I noticed that bug 443220 introduced a problem that causes check-one to give incorrect results. That is, it creates a new log file in the wrong directory, then scans the old log file for errors. I'll file a bug on that.

The current failures in mailnews unit tests are coming from code like:

var bugmail1 = do_get_file("../mailnews/test/data/bugmail1");

do_get_file is choking on the ".." in the file path. I see that was added as part of the switchover to comm-central. comps[i] is ".." when it fails. lf.path is "s:\hg\src\mozilla"

Comment 2

[Josh Geenen (:pi)]

As rkent said, the problem is all of the instances of ".." in the calls to do_get_file.

It turns out that the AppendInternal method of nsLocalFileWin.cpp (called by the Append method) returns NS_ERROR_FILE_UNRECOGNIZED_PATH if the parameter is ".."

http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/io/nsLocalFileWin.cpp#1060
There is a possibly related note in nsILocalFile.idl that mentions not allowing .. for "security reasons"
http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/io/nsILocalFile.idl#123

As a temporary workaround, I was able to make all tests in mailnews pass in Windows (and Linux) by changing the for loop in the do_get_file method
http://mxr.mozilla.org/comm-central/source/mozilla/tools/test-harness/xpcshell-simple/head.js#155
To:
  for (var i = 0, sz = comps.length; i < sz; i++) {
    // avoids problems if either path ended with /
    if (comps[i].length > 0)
      if (comps[i] == "..")
        lf = lf.parent;
      else
        lf.append(comps[i]);
  }

But I'm curious as to why nsLocalFileWin.cpp throws an error in this case but nsLocalFileUnix.cpp, for example, doesn't.
http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/io/nsLocalFileUnix.cpp#1755

Comment 3

[Mark Banner (:standard8)]

Moving to Testing/TUnit and updating summary to reflect the real problem. I'm going to attach a patch based on Josh's one in a moment.

Comment 4

[Mark Banner (:standard8)]

Attachment: The fix

Patch based on Josh's version. I think this should work (it works fine for me on mac).

Comment 5

[Kent James (:rkent)]

The proposed change also works for me on Windows 2003 Server x64 (which previously failed).

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 330929 [details] [diff] [review]
The fix

Can you add braces to that outer if statement? I'm not a fan of nested ifs without braces.

Comment 7

[Mark Banner (:standard8)]

(In reply to comment #6)
> (From update of attachment 330929 [details] [diff] [review])
> Can you add braces to that outer if statement? I'm not a fan of nested ifs
> without braces.
> 
Definitely, I'd have probably done that myself had I not been copying another patch.

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)
> In passing, I noticed that bug 443220 introduced a problem that causes
> check-one to give incorrect results. That is, it creates a new log file in the
> wrong directory, then scans the old log file for errors. I'll file a bug on
> that.

Bug 447043 ;-)

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)
> But I'm curious as to why nsLocalFileWin.cpp throws an error in this case but
> nsLocalFileUnix.cpp, for example, doesn't.

That's bug 349090 ;-)

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 330929 [details] [diff] [review]
The fix

{{
changeset:  16178:fd1ba554c41d
user/date:  Mark Banner <[e-mail]>	2008-07-24 15:33:41
}}
with comment 7 suggestion(s).

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #10)
> changeset:  16178:fd1ba554c41d
> user/date:  Mark Banner <[e-mail]>   2008-07-24 15:33:41

Let's use shared repository numbers :->
[
Mark Banner <[e-mail]> - Thu, 24 Jul 2008 16:33:41 +0100 - rev 16174
]