Closed
Bug 589413
Opened 13 years ago
Closed 13 years ago
Failure to open libnss3.so when Firefox path contains UTF-8 characters
Categories
(Core :: js-ctypes, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
People
(Reporter: merike, Assigned: dwitte)
Details
(Keywords: intl)
Attachments
(4 files, 1 obsolete file)
11.70 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
3.09 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
7.36 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
6.72 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•13 years ago
|
Component: Crypto → js-ctypes
Product: Weave → Core
QA Contact: crypto → js-ctypes
Assignee | ||
Comment 1•13 years ago
|
||
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.
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → dwitte
Comment 2•13 years ago
|
||
We should at least assume UTF8, but is there no way to provide a callback so that we can hook up NS_CopyUnicodeToNative?
blocking2.0: ? → final+
Assignee | ||
Comment 3•13 years ago
|
||
Indeed, we could. I think I'll go that route!
Assignee | ||
Comment 4•13 years ago
|
||
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.
Assignee | ||
Comment 5•13 years ago
|
||
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
Assignee | ||
Comment 6•13 years ago
|
||
... right, because the string literal in JS has to be '\u00F6'. Works fine with that change.
Assignee | ||
Comment 7•13 years ago
|
||
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.
Attachment #469276 -
Flags: review?
Assignee | ||
Updated•13 years ago
|
Attachment #469276 -
Flags: review? → review?(jwalden+bmo)
Assignee | ||
Comment 8•13 years ago
|
||
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?
Attachment #469277 -
Flags: review?(benjamin)
Comment 9•13 years ago
|
||
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.
Attachment #469276 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 10•13 years ago
|
||
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.
Attachment #469277 -
Flags: review?(benjamin)
Assignee | ||
Comment 11•13 years ago
|
||
I forgot to actually make the default case do a UTF8 conversion. Here we go.
Attachment #469551 -
Flags: review?(jwalden+bmo)
Comment 12•13 years ago
|
||
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.
Assignee | ||
Comment 13•13 years ago
|
||
Not urgent, but please do keep me posted!
Assignee | ||
Comment 14•13 years ago
|
||
Attachment #469277 -
Attachment is obsolete: true
Attachment #471680 -
Flags: review?(benjamin)
Assignee | ||
Comment 15•13 years ago
|
||
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...
Attachment #471683 -
Flags: review?(bent.mozilla)
Updated•13 years ago
|
Attachment #471680 -
Flags: review?(benjamin) → review+
Comment 16•13 years ago
|
||
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.
Attachment #469551 -
Flags: review?(jwalden+bmo) → review+
Updated•13 years ago
|
Attachment #471683 -
Flags: review?(bent.mozilla) → review+
Assignee | ||
Comment 17•13 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•