470914 - add a __LOCATION__ field to the global object for files loaded on the commandline in xpcshell

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[(not currently active) Ted Mielczarek]

Attachment: add __LOCATION__ to files loaded on the xpcshell commandline

JS Components loaded via the component loader get a nice __LOCATION__ field on the global object--a nsILocalFile containing the full path to the script being loaded. We should add this same field to files loaded from the commandline in xpcshell. This patch basically copies the logic from the component loader into xpcshell.

This would be specifically useful for me in fixing bug 421611, since the mochitest server.js needs to know where its test path is, and it currently relies on finding the current process directory and then hard coding the _tests/testing/mochitest path from there. With this patch, it could just use __LOCATION__.parent instead.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: much longer but more complete

I fixed a few timeless nits, and then added a bunch of code to handle relative paths. This patch is much longer than I would have liked, but it's correct and works on Win/Mac/Linux. timeless' other concern was that he'd like to unify the js shell and xpcshell code at some point, so adding things like this to only one is yet more drift. This code has already suffered some drift, so I'm not sure what the best way to do this would be.

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 354305 [details] [diff] [review]
much longer but more complete

timeless, I'd like some feedback on the patch itself, as well as how I could make this more modular to not make merging js shell and xpcshell harder in the future.

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 354305 [details] [diff] [review]
much longer but more complete

Turns out that this works, but it doesn't work like I expected, and I think the behavior is too confusing to be useful as-is. Because all files loaded share the same global object, we redefine __LOCATION__ for each one. The problem is that if you run:
xpcshell -f one.js -f two.js, with the files like:
one.js:
function one() { print(__LOCATION__.path); }
two.js:
one();

You'll see "two.js" printed, because that's what __LOCATION__ is bound to when that function is called. sayrer suggested that I might be able to add a getter for __LOCATION__ instead, and use cx->fp->script to determine the filename.

Comment 4

[timeless]

in theory it should be possible to create a local (not global!) frame which has location and let scope chaining capture it. but i'd imagine that'd be annoyingly expensive :).

Comment 5

[(not currently active) Ted Mielczarek]

Attachment: better, faster, stronger

So yeah, this is actually much simpler. sayrer pointed out that cx->fp->script->name is basically exactly what I want. This just implements __LOCATION__ as a getter that gets that and wraps it in a nsLocalFile. The tests pass now, which is cool.

Comment 6

[timeless]

assuming it's possible to dynamically change the cwd, your code will fail ;-).

       If buf is NULL, the behaviour of getcwd() is undefined.

       If the current absolute path name would require  a  buffer
       longer  than size elements, NULL is returned, and errno is
       set to ERANGE; an application should check for this error,
       and allocate a larger buffer if necessary.

i'd suggest:

char a[1];
getcwd(a, 1);

then dynamically allocate a proper array.

Comment 7

[(not currently active) Ted Mielczarek]

Well, glibc and Darwin both support passing NULL:

"The GNU library version of this function also permits you to specify a null pointer for the buffer argument. Then getcwd allocates a buffer automatically, as with malloc (see Unconstrained Allocation). If the size is greater than zero, then the buffer is that large; otherwise, the buffer is as large as necessary to hold the result. "

http://www.gnu.org/software/libc/manual/html_node/Working-Directory.html#Working-Directory

"If buf is NULL, space is allocated as necessary to store the pathname.  This space may later be
     free(3)'d."

http://developer.apple.com/DOCUMENTATION/Darwin/Reference/ManPages/man3/getcwd.3.html

Comment 8

[timeless]

sure, but it's non standard, why not do it "right" instead? :)

btw instead of NS_ConvertUTF8toUTF16, do you want to use the Copy... variant?


+            rv = NS_NewLocalFile(absolutePath,
+                                 PR_FALSE, getter_AddRefs(location));
+
^ i'm pretty sure this blank line isn't wanted
+        }

\ No newline at end of file

^ ...

Comment 9

[(not currently active) Ted Mielczarek]

(In reply to comment #6)
>        If the current absolute path name would require  a  buffer
>        longer  than size elements, NULL is returned, and errno is
>        set to ERANGE; an application should check for this error,
>        and allocate a larger buffer if necessary.

The reason I went with the glibc/Darwin POSIX extension behavior is because you'll note that nowhere do you get the *necessary length* of the buffer with an ERANGE error code. It's just "guess again". I felt like it just wasn't worthwhile to deal with that when the 2 Unixy platforms that I actually care about had a sane behavior available. If you really want it, I'll write the POSIX-compat version.

Comment 10

[(not currently active) Ted Mielczarek]

Attachment: POSIXy etc

Ok, ok. I moved the code to get the current working directory out into a separate function, and we just call it once on startup, so in case anything changes cwd, this will still work fine. While I was there, I changed the getcwd code to be POSIX compatible. It starts with a 1024 char buffer and will double it until it works. I didn't put in a fail safe there, I don't know if you think I ought to.

Also cleaned up the spacing/end of file newline nits.

Comment 11

[(not currently active) Ted Mielczarek]

Attachment: fix some more things [Checkin: See comment 13 & 24]

Fixed a few more nits timeless pointed out over IRC. Check the return value of SetLength, use .Length() anywhere we pass buffer location to a system call.

Comment 12

[timeless]

Comment on attachment 356945 [details] [diff] [review]
fix some more things
[Checkin: See comment 13 & 24]


+    workingDirectory.SetLength(requiredLength - 1);
+    workingDirectory.Append(NS_LITERAL_STRING("\\"));

can't you use SetLength(requiredLength); workingDirectory[requiredLength-1] = '\\'

and similarly for the other?

Comment 13

[(not currently active) Ted Mielczarek]

I changed them to:
    workingDirectory.SetLength(requiredLength);
    workingDirectory.Replace(workingDirectory.Length() - 1, 1, L'\\');

(there's no operator[] that allows assignment in nsStringAPI)

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/6d33746ff263

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

This broke the Windows Mobile build because GetCurrentDirectoryW doesn't exist on the platform.  

We've implemented _wgetcwd in the shunt:
http://mxr.mozilla.org/mozilla-central/source/build/wince/shunt/map.cpp#327

The caveat is that there is no concept of current working directory on the platform.  Instead we get the path to (essentially) the binary we're running from.  Will this approximation be good enough for you purposes?  Or should we just punt and return false?

Comment 15

[(not currently active) Ted Mielczarek]

Well, that code exists to deal with relative paths. Can you not pass relative paths on the commandline of wince programs? Namely, the Mochitest harness runs xpcshell something like:
/path/to/xpcshell -f ./httpd.js -f ./server.js
and we need to resolve those filenames to full pathnames. If wince is truly unable to cope here, just change the code to return false if that function doesn't get a full pathname, and we'll fix test harnesses to pass full paths in situations where we need them.

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: returns false when WINCE is defined [Checkin: Comment 18 & 25]

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 358017 [details] [diff] [review]
returns false when WINCE is defined
[Checkin: Comment 18 & 25]

r+sr=me (looking mostly at comment 15).

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

http://hg.mozilla.org/mozilla-central/rev/3a1526770db4

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 356945 [details] [diff] [review]
fix some more things
[Checkin: See comment 13 & 24]

In case you want to land this to 1.9.1,
bug 446300 moved
/tools/test-harness/xpcshell-simple/
...

Comment 20

[(not currently active) Ted Mielczarek]

Attachment: followup, don't normalize symlinks [Checkin: Comment 22 & 26]

I didn't realize that nsIFile.normalize() would dereference symlinks. That seems like the wrong behavior here. (In fact, it breaks my intended use case in the Mochitest harness.) This makes us not normalize __LOCATION__ if it's a symlink.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 359743 [details] [diff] [review]
followup, don't normalize symlinks
[Checkin: Comment 22 & 26]

Sure.

Comment 22

[(not currently active) Ted Mielczarek]

Comment on attachment 359743 [details] [diff] [review]
followup, don't normalize symlinks
[Checkin: Comment 22 & 26]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/b6492707ea27

Comment 23

[(not currently active) Ted Mielczarek]

You don't need approval or anything, we don't ship xpcshell. If you want this on 1.9.1 (which is a good idea), just land the changesets mentioned here on 1.9.1.

Comment 24

[Serge Gautherie (:sgautherie)]

Comment on attachment 356945 [details] [diff] [review]
fix some more things
[Checkin: See comment 13 & 24]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e622ecc23fc1
(Updated after bug 446300.)

Comment 25

[Serge Gautherie (:sgautherie)]

Comment on attachment 358017 [details] [diff] [review]
returns false when WINCE is defined
[Checkin: Comment 18 & 25]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3221ea405ac0

Comment 26

[Serge Gautherie (:sgautherie)]

Comment on attachment 359743 [details] [diff] [review]
followup, don't normalize symlinks
[Checkin: Comment 22 & 26]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e4efdaba2f4f