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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

defect
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: ted, Assigned: ted)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed1.9.1b3])

Attachments

(3 attachments, 4 obsolete attachments)

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.
Posted patch much longer but more complete (obsolete) — Splinter Review
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.
Assignee: nobody → ted.mielczarek
Attachment #354293 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Blocks: 460515
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.
Attachment #354305 - Flags: review?(timeless)
Attachment #354305 - Attachment is obsolete: true
Attachment #354305 - Flags: review?(timeless)
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.
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 :).
Posted patch better, faster, stronger (obsolete) — Splinter Review
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.
Attachment #355812 - Flags: review?(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.
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
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

^ ...
(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.
Posted patch POSIXy etc (obsolete) — Splinter Review
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.
Attachment #355812 - Attachment is obsolete: true
Attachment #356846 - Flags: review?(timeless)
Attachment #355812 - Flags: review?(timeless)
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.
Attachment #356846 - Attachment is obsolete: true
Attachment #356945 - Flags: review?(timeless)
Attachment #356846 - Flags: review?(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?
Attachment #356945 - Flags: review?(timeless) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 474366
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #358017 - Flags: superreview+
Attachment #358017 - Flags: review?(mrbkap)
Attachment #358017 - Flags: review+
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).
http://hg.mozilla.org/mozilla-central/rev/3a1526770db4
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
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/
...
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.
Attachment #359743 - Flags: review?(mrbkap)
Comment on attachment 359743 [details] [diff] [review]
followup, don't normalize symlinks
[Checkin: Comment 22 & 26]

Sure.
Attachment #359743 - Flags: superreview+
Attachment #359743 - Flags: review?(mrbkap)
Attachment #359743 - Flags: review+
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
Attachment #359743 - Attachment description: followup, don't normalize symlinks → followup, don't normalize symlinks [checked in]
Attachment #358017 - Attachment description: returns false when WINCE is defined → returns false when WINCE is defined [checked in]
Attachment #356945 - Attachment description: fix some more things → fix some more things [checked in]
Flags: wanted1.9.1?
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 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.)
Attachment #356945 - Attachment description: fix some more things [checked in] → fix some more things [Checkin: See comment 13 & 24]
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
Attachment #358017 - Attachment description: returns false when WINCE is defined [checked in] → returns false when WINCE is defined [Checkin: Comment 18 & 25]
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
Attachment #359743 - Attachment description: followup, don't normalize symlinks [checked in] → followup, don't normalize symlinks [Checkin: Comment 22 & 26]
Flags: wanted1.9.1? → in-testsuite+
Keywords: fixed1.9.1
Whiteboard: [fixed1.9.1b3]
You need to log in before you can comment on or make changes to this bug.