589413 - Failure to open libnss3.so when Firefox path contains UTF-8 characters

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[Merike Sell (:merike)]

By default the desktop directory on Ubuntu contains 'ö' character when Estonian localization is used. If Firefox build is put into desktop directory then Sync  doesn't sync and displays "couldn't open library" in error console. It refers to line 137 where Sync tries to load libnss. As soon as Firefox build is moved to a path not containing 'ö' or other non-ascii characters the same build with the same profile syncs and no error is present in console. This does not happen on Windows 7. It also should not happen on Ubuntu as the rest of Firefox works just fine with such path. The failure occurs at ctypes.open(nssfile.path) call so this might be better suited in js-ctypes component.

Comment 1

[[e-mail]]

Indeed, this is expected. See http://mxr.mozilla.org/mozilla-central/source/js/src/ctypes/Library.cpp#147. Ideally we want a UTF16 -> native charset converter, which differs per platform. (Do you really want to know? Gecko has a thousandd lines of it. See http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsNativeCharsetUtils.cpp.)

We could make a reasonable assumption that the native charset is UTF8 -- we already have a specialcase for Windows, so we mostly have Mac and the Unices to worry about. It'd probably be better than assuming ASCII.

UTF8 platforms:
Mac OS X, Android, BeOS.

Not sure about OS/2. XP_UNIX looks like a mess. I have no idea if it's that complicated just for completeness, or if we can make a reasonable assumption for the majority case.

Comment 2

[Benjamin Smedberg]

We should at least assume UTF8, but is there no way to provide a callback so that we can hook up NS_CopyUnicodeToNative?

Comment 3

[[e-mail]]

Indeed, we could. I think I'll go that route!

Comment 4

[[e-mail]]

I've implemented the callback and conversion code, but it doesn't work. I tested it on Linux with a file containing 'ö', and NSPR can't find it.

In fact, creating an nsILocalFile and using .initWithPath() fails too -- it can't find the file. Similarly, getting the .directoryEntries for the containing path and enumerating them returns a filename containing a unicode � character.

Our underlying file code can't even handle this, so this is going to take a bunch more work to fix, I think.

Or I could just skip all our Gecko code and call straight into libc to do it. fopen() doesn't work on the converted-to-native string, but perhaps the conversion is wrong or there's a unicode libc API for opening files.

Comment 5

[[e-mail]]

Hmm. fopen() actually works fine (as expected!) with a UTF8 string literal.

The problem is somewhere in the UTF16 -> UTF8 conversion code. Dumping the literal 'ö' from a string literal in C gives the hex digits:

char ffffffc3
char ffffffb6

but going through js_DeflateStringToUTF8Buffer or NS_CopyUnicodeToNative gives:

char ffffffc3
char ffffff83
char ffffffc2
char ffffffb6

Comment 6

[[e-mail]]

... right, because the string literal in JS has to be '\u00F6'.

Works fine with that change.

Comment 7

[[e-mail]]

Attachment: part 1: ctypes impl

Adds a JSCTypesCallbacks struct. We can (and will) fill this out in future; shaver assures me that we don't need to worry too much about backward compat.

It would be nice to abstract the entire library opening/symbol resolution/closing ops into callbacks, too (such that JS_HAS_CTYPES doesn't depend on NSPR) -- but that can wait.

Comment 8

[[e-mail]]

Attachment: part 2: consumers

I'm not fantastically happy about the same code snippet in three different places. Can you think of a good spot common to all three of them, where I could stash a CTypesHelpers.h?

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 469276 [details] [diff] [review]
part 1: ctypes impl

>diff --git a/js/src/ctypes/Library.cpp b/js/src/ctypes/Library.cpp

> JSBool
> Library::Open(JSContext* cx, uintN argc, jsval *vp)
> {
>+  JSObject* ctypesObj = JS_THIS_OBJECT(cx, vp);
>+  if (!IsCTypesGlobal(cx, ctypesObj)) {

Check for failure of JS_THIS_OBJECT: you're going to null-deref if it fails.

r=me with that fixed.

Comment 10

[[e-mail]]

Comment on attachment 469277 [details] [diff] [review]
part 2: consumers

This didn't quite make it through the workers or jetpack tests. Will need some tweaks there.

Comment 11

[[e-mail]]

Attachment: part 1 part 2: actually default to utf8

I forgot to actually make the default case do a UTF8 conversion. Here we go.

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 469551 [details] [diff] [review]
part 1 part 2: actually default to utf8

Regrettably for you, I won't be able to get to this until Monday, at the earliest -- please redirect if you need this done sooner.

Comment 13

[[e-mail]]

Not urgent, but please do keep me posted!

Comment 14

[[e-mail]]

Attachment: part 2: consumers

Comment 15

[[e-mail]]

Attachment: part 3: tests

For the worker tests, we need to copy the file in the parent, since the worker doesn't have access to the right bits. This isn't exactly pretty, but it works!

The try/catch wrapping in test_jsctypes.js.in isn't strictly necessary, but I added it when I was messing around with getting jetpack to run the whole thing in the child. I'll leave all that for another bug...

Comment 16

[Jeff Walden [:Waldo]]

Comment on attachment 469551 [details] [diff] [review]
part 1 part 2: actually default to utf8

If JS_ALWAYS_TRUE is exposed in a public header, you might consider using that rather than rolling your own in ASSERT_OK, in a followup bug.

Comment 17

[[e-mail]]

http://hg.mozilla.org/mozilla-central/rev/07612386060b
http://hg.mozilla.org/mozilla-central/rev/313342d1b5a9
http://hg.mozilla.org/mozilla-central/rev/008351527072
http://hg.mozilla.org/mozilla-central/rev/7b0f220344a9