268219 - NS_CopyNativeToUnicode/NS_CopyUnicodeToNative are broken on OS X

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jungshik Shin]

In xpcom/io/nsNativeCharsetUtils.cpp, either iconv() or wcstombs/mbstowcs is
used for NS_CopyNativeToUnicode/NS_CopyUnicodeToNative on OS X. If iconv() is
used at the compilation time, this would not work in absence of iconv(). It
seems like wcstombs/mbstowcs()' don't work either. 

Because the native file system character encoding on OS X is UTF-8 (NFD. NFD/NFC
issue is taken care of in nsLocalFileOSX.cpp), we'd better just do what's done
for BeOS, that is, call CopyUTF8toUTF16 and CopyUTF16toUTF8. 

It's strange that this hasn't been noticed before....

Comment 1

[Jungshik Shin]

This is rather urgent.

Comment 2

[Jungshik Shin]

Attachment: patch

this is rather trivial, but the impact would be large.

Comment 3

[Jungshik Shin]

Comment on attachment 164989 [details] [diff] [review]
patch

asking for r/sr

Comment 4

[Jungshik Shin]

IMHO, it's necessary to release TB 0.9.1 for Mac OS X with this fixed.

Comment 5

[David :Bienvenu]

requesting blocking aviary1.0 mac flag...I'll talk to mscott about rolling out a
.9.1 build for tbird tomorrow.

Comment 6

[Jungshik Shin]

David, thanks for backing me :-) also, thanks for reminding me of
'aviary-1.0mac'  which I missed.

Now I understand why this hasn't been noticed before. The pair of functions are
used only in a few files on Mac OS X (they're a lot more used on Windows).
Here's the list:

http://lxr.mozilla.org/seamonkey/ident?i=NS_CopyUnicodeToNative
http://lxr.mozilla.org/seamonkey/ident?i=NS_CopyNativeToUnicode

Note that most places are Windows only. On a few-to-several XP cases, they're
used for something that are usually ASCII-only (envirionment variable names and
values. In case of values, they can be non-ASCII far more often than names, but
it could well have escaped our radar on OS X). And, mailnews uses pretty often
in XP code,  a symptom of which is bug 268185. 

So, it seems that the patch is pretty safe even for firefox 1.0. Anyway, it's
for sure necessary for TB. 

Comment 7

[Darin Fisher]

Comment on attachment 164989 [details] [diff] [review]
patch

sr=darin

Comment 8

[Darin Fisher]

some history: when nsNativeCharsetUtils was created, its sole consumer was
nsLocalFile.  nsLocalFileOSX does not use nsNativeCharsetUtils since it knows
that the "native" charset is just UTF-8.  this is in part why there is the
disclaimer in nsNativeCharsetUtils.h -- the I18N team was very concerned that
people would use these methods outside of xpcom/io.

it looks like over time that these methods were used in many other places
throughout the tree.  but it's no surprise that they were mainly used in
platform specific code.  (i recall reviewing patches that added callers in UNIX
specific code.)

it turns out that nsStringAPI.h also exposes these methods indirectly via
NS_CStringToUTF16/NS_UTF16ToCString when NS_CSTRING_ENCODING_NATIVE_FILESYSTEM
is specified.

i think it's very important that we take this patch on the 1.7 branch, and it
would be good if it could be taken on the aviary branch too (for firefox 1.0 if
at all possible).

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #8)
> the I18N team was very concerned that
> people would use these methods outside of xpcom/io.

note bug 268266 comment 3

if people did not want this to be used outside of xpcom, they shouldn't have
exported the header... this is way too convenient to use, compared to the other
way (get platform default charset, get a unicode converter, call some functions
on it to do the conversion)

Comment 10

[Darin Fisher]

I think it makes sense to use these functions whenever dealing with the native
filesystem / platform charset.   Clearly, the behavior should be consistent with
whatever our intl code would provide or else badness would ensue.  The problem
is that nsNativeCharsetConv was never finished as a general purpose utility.

Comment 11

[Conrad Carlen (not reading bugmail)]

Comment on attachment 164989 [details] [diff] [review]
patch

r=ccarlen

Comment 12

[Jungshik Shin]

Comment on attachment 164989 [details] [diff] [review]
patch

thanks for r/sr, explanation, and support.  fix checked into the trunk

asking for a to branches. As Darin mentioned in comment #8, it's very important
to fix this on 1.7.x. It'd be great if this can make it in ff 1.0. If too late
for ff 1.0, this is for sure necessary for TB 0.9 and later (TB 0.9 on Mac is
hit hard enough to warrant releasing 0.9.1 with this fixed.)

Comment 13

[Mike Kaply [:mkaply]]

Comment on attachment 164989 [details] [diff] [review]
patch

a=mkaply for 1.7.

Ping aviary

Comment 14

[Jungshik Shin]

thanks for a. fix checked into 1.7 branch

Comment 15

[Scott MacGregor]

not sure if we'll respin mac 0.9 bits yet since 1.0 is right around the corner
anyway. Will discuss at a mtg later today. 

jshin, we shouldn't check this into aviary yet until Firefox 1.0 is out the door
(ETA tomorrow). I'll check this in after the firefox release.

Comment 16

[Scott MacGregor]

I've now checked this into the aviary 1.0 branch.

Comment 17

[Darin Fisher]

*** Bug 199210 has been marked as a duplicate of this bug. ***