227547 - [PATCH] need NFD <-> NFC conversion into and out of Mac OS X file system (Unicode normalization)

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[bart decrem, not checking bugmail]

See screenshot.  This problem is still present in Thunderbird 0.4rc2.  In hangul
(Korean), syllables are blocks of 2 or 3 characters stacked together.  In the
screenshot you'll see that the characters aren't stacked together, just
displayed side by side.  On my desktop you'll see the same filename probably
displayed.  

Basically, attachment file names are unreadable in Korean.

Comment 1

[bart decrem, not checking bugmail]

Attachment: the problem

attachment name improperly displayed (in Thunderbird) and properly (on the
desktop).

Comment 2

[Jungshik Shin]

This is because Mac OS X file system uses NFD. On Mac OS X, we have to convert
NFD -> NFC conversion when  rendering and when sending out filenames to the
'world' because most other platforms use NFC. I thought I had filed a bug on
this, but apparently I haven't.


BTW, I wouldn't say it's unreadable. It's still comprehensible. 

Another BTW, it's not limited to Korean. For instance, U+00C0  À (Latin Capital
Letter A with Grave) would be decomposed into 'A' and U+0300 (Combining Grave
Accent) on Mac OS X filesystem.

We may have to deal with this in nsLocalFileMacOS or ...

Comment 3

[Jungshik Shin]

Sorry, forgot to chagne the summary line.

Comment 4

[Scott MacGregor]

jshin, any chance of a quick fix or something I can back out to fix this?

I am minutes away from pushing thunderbird 0.4 when this bug came in. 

This would be pretty bad for I18N users. 

Comment 5

[Jungshik Shin]

I'm afraid it's not easy. If I had a Mac, I would spend several hours (perhaps
longer) to fix it (at least in an ad-hoc manner), but I don't have a Mac. We
have nsIUnicodeNormalizer (in intl/unicharutil), but I've never used it so I
have little idea how easy it's to use it. MacOS X may have its own API for this
stufff. 

If you can, you may try hooking it up with attachment code. On the way out, you
have to convert NFD to NFC (put together decomposed characters into a
precomposed character). On the way in, we have to reverse the process. 

smontagu, can you help? 

BTW, this is also an issue in form submission. 

Comment 6

[Scott MacGregor]

sounds like a release note item. I'll add it. Re-assigning to the I18N default
owner.

Comment 7

[Simon Fraser [no longer active]]

Conrad has dealt with some decomposed unicode issues in nsLocalFileOSX, IIRC.

Comment 8

[Jungshik Shin]

I read the draft of Thunderbird release note. It has the following on the issue:

Mac OS X: Attachment file names in certain charactersets (like Korean) do not
render quite right.

It'd be better to say 'file names containing certain characters like accented
letters and Korean characters' because 1) it has little to do with 'character
sets', 2) even Western European users would easily notice the problem.

In addition, it may be necessary to warn users that their correspondents on
Windows and Linux are  likely to have troubles dealing with those attachments.
That is, it's not just rendering but an interoperability issue as well.

Note to myself and others:

http://developer.apple.com/qa/qa2001/qa1235.html

According to the document, it's not until OS 10.2 that the normalization API was
introduced so that I guess we have to use our own normalization API if we have
to support Mac OS 10.1 or earlier.

Comment 9

[bart decrem, not checking bugmail]

Jungshik - yeah, I guess it's still readable.  I used to have a friend who'd
write  entire phone messages in this style.  And you're right - the recipient of
that attachment wasn't able to open it on her Windows machine.  

If you're interested in working on this issue but need access to an OSX box, and
if remote access is all you need, I'm happy to let you log onto my iMac remotely
- just send me a private email to [e-mail].  My system is running Panther.

Comment 10

[Jungshik Shin]

Attachment: a possible quick patch for mail attachment

This is a bit ad-hoc patch, but I guess it'll work.
My memory is so unreliable that I forgot that I had a 
patch for a related bug (bug 206252). The patch I'm uploading includes the
patch for bug 206252, not all of which is necessary for this bug. I'll see if I
can separate them out from each other.

Bart, do you have a Mac OS X build environment? If so, can you apply the patch
and see how it works there? It'll be easier that way because even if I can
access your machine, there may be a little 'learning curve' (although Mac OS X
is just another BSD Unix in a certain sense). Anyway, thank you for your offer.


Needless to say, it'd be nice if mscott could try the patch as well.

As for the form submission, I guess I could modify my tentative patch for bug
224820 to fix this bug.

Comment 11

[Jungshik Shin]

Attachment: patch with nsMsgSend.cpp (missed in the prev. attch.)

I forgot to include nsMsgSend.cpp in the previous patch. This attachment
includes it.

Comment 12

[Don Crandall]

jshin:

I can build Thunderbird with your patch and transfer the new build to my .Mac
account for Bart to test.

I'll have it uploaded by 17:00 PST.

If any problems appear, I'll let you know.

Don

Comment 13

[Jungshik Shin]

Comment on attachment 136929 [details] [diff] [review]
patch with nsMsgSend.cpp (missed in the prev. attch.)

macdoc, it's nice of you to do that. Just in case, this attachment has all you
need (nost just nsMsgSend.cpp I forgot to include in the prev. attachment)

Comment 14

[Don Crandall]

Bart:

Your test build, with this patch, can be found at:

 	
http://homepage.mac.com/macrxnapa2/FileSharing1.html

Don

Comment 15

[Jungshik Shin]

Because no Mac OS X API is used in my patch, I tested it on Linux with (XP_MAC
manually defined in two source files where nsIUnicodeNormalizer is used) and it
woked well. I made a file with its name in NFD and attached it to a mail to
myself. The filename was turned to NFC as I intended.

Comment 16

[Jungshik Shin]

Attachment: a new patch (with bug 206252 stuff removed)

Now that bug 206252 was fixed, I made a new clean patch. I'm not sure whether
we can make it in 1.6final, but I'm trying anyway.

Comment 17

[Jungshik Shin]

My test on Linux with XP_MAC defined (manually) worked well. So, I guess it
should work fine on Mac OS X. Nonetheless, Bart, can you post your test result
with Don's build? Once I get the confirmation from you, I'll ask for r/sr.
(we're approaching the 1.6 branching so that we need to do this asap if we want
this to be fixed in 1.6).

For a better tracking (for me), I'm assiging this to myself.

Comment 18

[Jungshik Shin]

Attachment: form submission patch

This is a patch for form submission. My tree is rotten and I'm still building
it. I'll test it with XP_MAC manually defined later.

Comment 19

[Jungshik Shin]

Comment on attachment 137537 [details] [diff] [review]
a new patch (with bug 206252 stuff removed)

Asking for r/sr. My 'simulation' with XP_MAC manually defined works fine and I
don't see any reason it doesn't on real Mac OS X. 


>Index: mailnews/compose/src/nsMsgCompose.cpp

>+#ifdef XP_MAC
>+    nsAutoString decomposedName;
>+    CopyUTF8toUTF16(nsDependentCString(leafName), decomposedName);
>+    nsCOMPtr<nsIUnicodeNormalizer> normalizer (do_GetService(NS_UNICODE_NORMALIZER_CONTRACTID));
>+    if (normalizer)
>+      normalizer->NormalizeUnicodeNFC(decomposedName, _retval);
>+    else
>+      _retval = decomposedName;
>+#else
>     CopyUTF8toUTF16(nsDependentCString(leafName), _retval);


  CopyUTF8toUTF16 accepts 'const char*' as the 1st argument so that I'll just
use CopyUTF8toUTF16(leafName, ......).

Comment 20

[David :Bienvenu]

Comment on attachment 137537 [details] [diff] [review]
a new patch (with bug 206252 stuff removed)

r=bienvenu, but is this a typo?:
+  // we have to convert NFD (decomposed Unicode) to NFC (precomosed Unicode)

I assume it should be precomposed.

Comment 21

[Jungshik Shin]

thanks for r and catching the typo. I've just fixed it in my tree.

Comment 22

[Don Crandall]

jshin;

A small problem.

With the reviewed patch in my tree, my build cannot add add an attachment to a
new email. The attachment pane is empty after adding an attachment. The JS error
I see is:

Error: [Exception... "Not enough arguments [nsIMsgCompose.AttachmentPrettyName]"
 nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  location: "JS frame ::
chrome://messenger/content/messengercompose/MsgComposeCommands.js ::
AddAttachment :: line 2234"  data: no]
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 2234

This is when creating a new email with an attachment

Don

Comment 23

[Jungshik Shin]

macdoc, thanks for testing.
You had that problem  because your tree is not up to date. The reviewed 
patch does NOT include my fix for bug 206252 (that was committed
yesterday). You have to update your tree (at least 
xpfe/ and mailnews/compose/) _before_ applying the reviewed patch.
BTW, while you're at it, it'd be nice if you could test my patch for the
form submission as well. Fixing that would help lure some Korean mac users
to switch to Mozilla from Safari that has the same problem.

Comment 24

[Scott MacGregor]

actually I think macdoc is up to date. That recent check in changed mail APIs
used by thunderbird without changing all callers (i.e. the thunderbird callers). :)

Comment 25

[Jungshik Shin]

sorry, macdoc, that I forgot it's TB.  It should work now (with mscott's check-in). 

btw, I've just tested attachment 137587 [details] [diff] [review] (form submission patch) on Linux (with
XP_MAC defined) and it worked fine. 

Comment 26

[Jungshik Shin]

Comment on attachment 137587 [details] [diff] [review]
form submission patch

asking for r/sr.

A more generic solutin in nsILocalFileMacOSX would be desirable, but it seems
like we can't avoid OS X version dependency (10.2 or later vs before) because
we can't use nsIUnicodeNormalizer in xpcom/io. In the meantime, this fix would
take care of the most conspicuous (to end-users) problem arising from OS X' use
of NFD in the file system.

Comment 27

[Don Crandall]

jshin,

Ok, I stand corrected. After backing out my last attempt, updating my tree, then
re-applying the patch, I find that I can now add an attachment to a new email
with no errors.

Posting this build for Bart's turn at testing.

Thank you
Don

Comment 28

[Jungshik Shin]

Don, actually, as mscott pointed out, it's my fault that I forgot to fix the
part of thunderbird that corresponds to Mozilla. Anyway, can you test it (bart
seems busy or away) yourself? You don't need a Korean file (although it's not
hard to make one) to test it. Can you make a file 'Göthe' (it's <Latin small
letter O with diaeresis  : small o-umlaut> ) and send it to me as an attachment?
I don't see any reason it doesn't work on Mac OS X because I've already tested
it on Linux with XP_MAC defined. Nonetheless, it'd be nice to confirm it on a
real Mac. 

Comment 29

[Jungshik Shin]

Comment on attachment 137587 [details] [diff] [review]
form submission patch

I realized that bryner is away until Dec. 30.

Comment 30

[Jungshik Shin]

Comment on attachment 137537 [details] [diff] [review]
a new patch (with bug 206252 stuff removed)

thank you all. fix checked into the trunk.

asking for 1.6a.

Affected platform : Mac OS X

Affected users: any one who wants to attach files whose names contain
decomposable characters (e.g. Latin/Greek/Cyrillic letters with diacritic marks
such as grave accent, diareses, caron, accent and Korean Hangul)

Test : done on Linux with 'Mac OS X simulated'. there should be no problem on
Mac OS X because no Mac OS X specific API is used in the patch. I'm using
Mozilla APIs (nsINormalizer) but turn it on only on Mac OS X.

Risk: low. there's a fallback code in case  nsINormalizer fails.

Comment 31

[Jungshik Shin]

Attachment: additional patch for mailnews : failure check

Sorry I forgot to check the return value of NormalizeToNFC(). This is to add
the check and fall back in case of the failure.

Comment 32

[Jungshik Shin]

Attachment: form submission patch with error checking added

This patch is to attachment 137587 [details] [diff] [review] what attachment 137873 [details] [diff] [review] is to attachment
137537 [details] [diff] [review]. I added the error checking for NormalizeToNFC.

Comment 33

[Jungshik Shin]

Comment on attachment 137587 [details] [diff] [review]
form submission patch

removing obsolete r/sr request

Comment 34

[Jungshik Shin]

Comment on attachment 137873 [details] [diff] [review]
additional patch for mailnews : failure check

asking for r/sr

Comment 35

[Jungshik Shin]

setting the target milestone to 1.6final

Comment 36

[Jungshik Shin]

Comment on attachment 137873 [details] [diff] [review]
additional patch for mailnews : failure check

heard that mscott is away. asking sspitzer  for sr.

Comment 37

[Jungshik Shin]

Comment on attachment 137873 [details] [diff] [review]
additional patch for mailnews : failure check

>Index: mailnews/compose/src/nsMsgCompose.cpp

> #ifdef XP_MAC

I realized that I have to use XP_MACOSX instead of XP_MAC. I'll do that in both
files.

Comment 38

[Jungshik Shin]

Attachment: alternative to attachment 137874

This patch is equivalent to attachment 137874 [details] [diff] [review] as far as the current Mozilla is
concerned. In this patch, the conversion to NFC is done at calling sites. It
takes a smaller change, but we have to apply the same fix if we have more
callers later. 
I'm not sure which one is better. 

darin and sfraser, would you review either this one or attachment 137874 [details] [diff] [review]? 
I really love to get this fixed in before 1.6 final. TIA.

Comment 39

[Jungshik Shin]

Comment on attachment 138055 [details] [diff] [review]
alternative to attachment 137874 [details] [diff] [review]

asking for r/sr.
There's no default owner for the form submission bug (actively working on
mozilla) so that I'm asking sfraser (it's a Mac issue) and darin for r/sr.

Comment 40

[Simon Fraser [no longer active]]

I need to see this tested on Mac before I'm willing to review.

Comment 41

[Jungshik Shin]

Well, I don't think anything can go wrong on Mac OS X (judging from my test
result : comment #15. A similar test was done for the form submission). However,
I understand your concern.

Don and Bart, can you help? Don, you can test it yourself (if Bart can't help) :

1.  make a file whose name contains accented letters (e.g.  'Göthe'  - it's
<Latin small
letter O with diaeresis  : small o-umlaut> )
2. Set View | Character coding to UTF-8 
3. Upload it here as an attachment :-) 

It'd be also helpful if you put up a binary with the patch applied (attachment
137874 [details] [diff] [review]) as you did before. Then, I'll ask Korean Mac users to test it. 

Comment 42

[bart decrem, not checking bugmail]

I'd be happy to test this on OSX, but I don't know how to make a custom build. 
Is there any way you can provide me with a binary of Thunderbird or Seamonkey
that includes your patch?

Comment 43

[Jungshik Shin]

I wish I could, but I don't have any Mac around (if I had, I would have tested
it myself :-)). Don's build is of no use because of my mistake (of using XP_MAC
instead of XP_MACOSX in my original patch). 

Judging from what's documented at http://www.mozilla.org/build/mac.html,
I don't think it's hard to build a Mozilla on Mac OS X. Can you give it a shot?
What's the spec of your iMac? If it has a reasonable amount of memory and disk
space and decent horse power ( > 500MHz, > 512MB, a few GB of free disk space),
it'd not take long to build a Mozilla. The patch to apply is attachment 138055 [details] [diff] [review].
If your system is not up to that, you may just begin the build and go to bed and
next morning it'll be ready :-) For an overnight build, it's a good idea to use
1.6b (release) source tar ball (instead of the trunk source from the CVS). I'll
upload  a diff. against 1.6b tree (for the form submission test, attachment
138055 [details] [diff] [review] is sufficient, but for the mail attachment testing, you need a combined
patch)

Comment 44

[Jungshik Shin]

Attachment: combined patch against 1.6 branch

Bart, here's the combined patch against 1.6branch (should be applied cleanly to
1.6b release source tar ball, I think).

Mozilla 1.6b source tar ball is avaialble at

ftp://ftp.mozilla.org/pub/mozilla.org/mozilla/releases/mozilla1.6b/src/

Thanks !

Comment 45

[Don Crandall]

jshin,

I cannot get a Thunderbird build from the recommended tree using the combined
patch. Is is possible to get a patch for a recent trunk so I can build Thunderbird?

OTOH, if a Mozilla build is good enough, I can try to get that for you.

Things I tried:

I did a CVS download of the MOZILLA_1_6b_RELEASE branch. The XUL app directories
are not included in this branch. A large number of errors appears similar to this:

cvs server:
mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.h is no
longer in the repository

This error, of course, appears for all of the file/subdirectories for the entire
toolkit dir.

I also tried to copy the necessary directories from my trunk tree, but this also
fails to build at the chrome dir.

Any further recommendations and/or suggestions are welcome.

Don

Comment 46

[Jungshik Shin]

Don, thanks a lot for your attempt. I'm afraid and sorry that you messed up your
local tree because you used to have only thunderbird source tree and then later
cvs-checkout/update with Mozilla 1.6b tag. My local tree was Mozilla only and
later I cvs-updated toolkit, mail, browser, calender directories manually so
that I didn't experience any problem.
The easiest way appears that you download 1.6b source tarball and uncompress in
a _separate_ directory . Then, you can appply attachment 136979 [details] [diff] [review] (that has gotten
landed into 1.6branch after 1.6b release) and attachment 138200 [details] [diff] [review].

Actually, Bart let me log onto his iMac and I successfully built a Mozilla
binary with two patches applied to 1.6b release source tar ball, but somehow the
disk image I made is over 40MB. (I thought it'd be around 20MB at most). I
disabled 'debug', enabled optimization with'-O2' and even explicitly specified
'enable-strip' in ~/.mozconfig. I also tried manually stripping with 'strip -x',
but that didn't make any difference. I may or may not have the disk quota to put
it up at my ISP (my quota is over 100MB, but I'm afraid it's almost full). I'll
check that out.
  

Comment 47

[Don Crandall]

Jungshik,

Actually, I did attempt it in a separate test tree I create for these occations.

If you would like to have a place to upload to, I can arrange to allow you place
it in my .Mac account. Just let me know.

Don

Comment 48

[bart decrem, not checking bugmail]

OK, thanks to Jungshik's kind help, I now have a 1.6b build that includes his
patch.  Unfortunately, the filenames still don't display correctly.  Rather than
bore you all with three screenshots, I'll email them directly to Jungshik so he
can analyze then and figure out what's going on.

Bart

Comment 49

[Jungshik Shin]

Thanks for testing. However, the symptom in your screenshots sent to me is not
what I'm dealing with here. That's another bug (complex script rendering issue
on Mac OS X. there are a couple of bugs filed on it). Can you send me an email
with a file with Korean names attached? Also, can you try to attach a file with
a Korean name here? 

Comment 50

[Jungshik Shin]

sfraser, can I get r for attachment 137874 [details] [diff] [review]?

I just confirmed that the name of a file Bart uploaded using an html form I set
up had been properly normalized to NFC. The html form I set up for him just
saved the file uploaded to my local disk with the name received from the client
side. The name is in NFC instead of NFD. 

As I wrote in my previous comment, what Bart mentioned is a different problem.
It's to be solved by using ATSUI and Korean AAT fonts with mort table for U+1100
Hangul Jamos. See, for instance, http://i18nl10n.com/korean/hunmin.html
Mozilla-Linux and Mozilla-Win render them correctly thanks to my patches for bug
177877, bug 176290 and bug 176315, but for the obvious reason, I couldn't port
it to Mac OS X. However, I don't think we need a similar patch on Mac OS X
because ATSUI and AAT fonts would solve the problem 'automagically'.  

Comment 51

[chris hofmann]

Comment on attachment 137537 [details] [diff] [review]
a new patch (with bug 206252 stuff removed)

1.6-  looks like we would need a different patch for the branch...  at this
point maybe we should just get this on the trunk for 1.7...  trying to shut
down work on 1.6

Comment 52

[Jungshik Shin]

Attachment: fixing nsLocalFileOSX with Mac OS X APIs : alternative

In this patch, I changed GetLeafName and GetPath to return leaf name and path
in NFC while leaving alone GetNativeLeafName and GetNativePath. As I wrote
earlier, CFStringNormalize is not available on Mac OS 10.1 or earlier. Not
knowing how to do it better, I check its availability with PR_FindSymbol() when
nsLocalFileOSX is initialized and invoke it only where it's available.	If
there's an Mac OS X API for the version checking (returning (10, 2, 1) or
100201 for 10.2.1), I'd love to use it. In nsLocalFileOSX, I guess the way it's
done in this patch is all right (PR_FindSymbol may be a bit expensive and slow
down the start-up time a bit, but it's invoked only once). However, for
nsFileSpec (xpcom/obsolete), I'm afraid that's too expensive because I have to
check it everytime GetLeafName() is called (or I have to use a global static
variable with locking). So, I'm looking for a cheap alternative (the OS version
checking API).

I've just found that uname is available on Mac OS X, but I'm a bit confused
about the relationship between Darwin kernel release (e.g. 7.2.0) and Mac OS
version number. Can I assume that 7.2.0 corresponds to 10.2.0?

Comment 53

[Conrad Carlen (not reading bugmail)]

I like the approach in attachment 138873 [details] [diff] [review]. It should catch all cases. A few
things about the patch, though:

+    CFIndex length = CFStringGetLength(inStr);
+    const UniChar* chars = CFStringGetCharactersPtr(inStr);
+
+    if (chars) 
+        aResult.Assign(chars, length);
+    else {
+        nsAutoArrayPtr<UniChar> buffer(new UniChar[length]);

Don't use nsAutoArrayPtr here. Use StBuffer instead since it can do this without
any allocation in most cases. See usage of StBuffer elsewhere in this file.
 
+        CFStringGetCharacters(inStr, CFRangeMake(0, length), buffer);
+        aResult.Assign(buffer, length);
+    }

 void nsLocalFile::GlobalInit()
 {
+    // CFStringNormalize was not introduced until Mac OS 10.2
+    if (PR_FindSymbol(NULL, "CFStringNormalize"))
+        gHasCFStringNormalize = PR_TRUE;

This won't work. The call to CFStringNormalize will cause linkage to that symbol
and, if it's not present, the app just won't launch - unless weak linking is
used. See http://developer.apple.com/technotes/tn2002/tn2064.html

Comment 54

[Conrad Carlen (not reading bugmail)]

> This won't work.

I take that back. The way it's done, the symbol should be lazy bound. Applying
the patch and running %nm -mg libxpcom.dylib gives:
(undefined [lazy bound]) external _CFStringNormalize (from Carbon)

Comment 55

[Jungshik Shin]

I wrote the following, but I've just found that you had taken back a part of
your comment :
<what I was about to submit>
Thanks a lot for the reference to Apple TN 2064. Because Mozilla has to be run
on Mac OS 10.1 (although it can't be compiled on 10.1), I guess I can't use weak
linking (that was introduced in 10.2). Instead, I have to use
CFBundleGetFunctionPointerForName to get the function pointer to
CFStringNormalize if available. I also have to figure out which framework
CFStringNormalize is included in (with 'otools' and 'nm')

BTW, fixing nsLocalFileOSX can't catch all cases because mailnews still relies
on nsFileSpec. I have to fix nsFileSpec as well, which was made easier thanks to
your help. 
</what ...>

TN 2064 has the following to say about the lazy binding:

<quote>
(c) Application runs but can't be prebound on older OS versions

Another problem with applications that bind to missing symbols is that their
prebinding will break (and can't be fixed) on OS versions that do not contain
all of the non-weak (described below) symbols (even lazy bound ones) that the
application uses. Broken prebinding for your application typically means that it
will take longer to launch. This means that lazy binding is not a good solution
for conditionalizing the use of APIs that might not be present at runtime - even
if you are careful to ensure that new APIs will never be called when the app is
run on an older OS version.
</quote>

In light of the above, wouldn't it be better to use
CFBundleGetFunctionPointerForName? Is it expensive? Or, don't we have to worry
about obsolete 10.1 as long as Mozilla can be run on it? 

Comment 56

[Jungshik Shin]

I found that CFBundleGetFunctionPointerForName is used at several places in the
tree. [1] In all of those cases, the following is used to get a bundle:

CFBundleRef quartzBundle =
getBundle(CFSTR("/System/Library/Frameworks/ApplicationServices.framework"))

Because CFStringNormalize is a part of Carbon framework, I guess I can repluace
Appl...Services with Carbon in the above. Having said that, I'm wondering if
'/System/Library/Frameworks/Carbon.framework' is guaranteed to work. 

A couple of sample codes at http://developer.apple.com uses a complicated code
(that I woulnd't claim to understand at the moment) to get a bundle reference.
For instance, see 

http://developer.apple.com/samplecode/Sample_Code/Cocoa/CocoaInCarbon/main.c.htm

Perhaps, that's for a private(?) bundle. For a system bundle, we can just get
away with the hard-coded path, can't we? (what if '/System/Library/....' become
subjected to 'localization'? Perhaps, that will not happen, but who knows...)

[1] http://lxr.mozilla.org/seamonkey/search?string=+CFBundleGetFunctionPo

Comment 57

[Conrad Carlen (not reading bugmail)]

Rather then hardcoding the path to the framework, you could try:
CFBundleRef bundle = CFBundleGetBundleWithIdentifier( CFSTR("com.apple.Carbon") );
The Carbon framework will be loaded in our context before this code is called,
so this should work quickly.

As far as fixing nsFileSpec, I wouldn't. It's been deprecated for several years.
IMO, the time would be better spent converting nsFileSpec usage that suffers
this problem to nsIFile, once this fix is in.

Comment 58

[Jungshik Shin]

Attachment: update to attachment 138873 (not yet include nsFileSpec fix)

I updated attachment 138873 [details] [diff] [review] per Conrad's comments and mine. 
I haven't yet figured out how to deal with nsFileSpec (I want to avoid the code
duplication). After making this patch, I began to wonder if it's worth the
trouble to avoid slow-down on Mac OS 10.1 while sacrificing the start-up time
on Mac OS 10.2 or later (assuming PR_FindSymbol is faster than what's done
here.)

Comment 59

[Jungshik Shin]

Ooops. I hadn't read Conrad's latest comment when I uploaded the latest patch.
I'll use CFBundleGetBundleWithIdentifier as suggested.

 As for nsFileSpec, there's no doubt we have to remove nsFileSpec asap. However,
it doesn't look like it's gonna happen soon. See bug 38122 and all the bugs
blocking it (nsFileSpec even got a new method a couple of weeks ago because it
was regarded as too much work to get rid of it in mailnews. Somebody has to work
on it fulltime for a week or so, I guess). Anyway, I'll try to fix bug 71535 to
resolve this bug.

Comment 60

[Jungshik Shin]

Attachment: update (to fix nsLocalFileOSX) per Conrad's comment

This patch doesn't hard-code the framework path any more. Thanks, Conrad.

Comment 61

[Jungshik Shin]

Comment on attachment 139019 [details] [diff] [review]
update (to fix nsLocalFileOSX) per Conrad's comment

Asking for r/sr.

Thanks to Bart, I was able to compile Mozilla with this patch on his Mac. Then,
I asked my friend to test it on his powerbook. He confirmed that it worked as
intended and expected. Filenames are rendered precomposed  _before_ as well as
after the submission, which indicates that nsLocalFileOSX passes to its
consumers filenames in NFC (composed) as I want it to.

Comment 62

[Jungshik Shin]

Comment on attachment 138055 [details] [diff] [review]
alternative to attachment 137874 [details] [diff] [review]

review request got obsolete..

Comment 63

[Conrad Carlen (not reading bugmail)]

I think this should all be confined to nsLocalFile::CopyUTF8toUTF16NFC.

Avoid
+typedef void (*StringNormalizer) (CFMutableStringRef, CFStringNormalizationForm);
and
+// CFStringNormalize was not available until Mac OS 10.2
+StringNormalizer nsLocalFile::gCFStringNormalizer = NULL;

These could be static inside CopyUTF8toUTF16NFC and then loading the bundle
could be done lazily. It's possible that we may not get a call for a non-native
path during startup in which case the laziness would help. Confining it all to
one place would be nice anyway. Move gCFStringNormalizer and then use another
static flag to keep track of whether you've attempted to load the bundle in
order to avoid repeatedly failing to load it on 10.1.
And then these checks:
+  if (!gCFStringNormalizer ||
can then be inside the function. Function call overhead is relatively nothing here.

Overall, looks great. Just some cut & paste...

Comment 64

[Jungshik Shin]

Attachment: update (nsLocalFileOSX)

Thanks for the comment. I updated the patch per your comment. (I had some
reservation about the thread safety, but it should be all right in this
particular case, I think).

Comment 65

[Conrad Carlen (not reading bugmail)]

Comment on attachment 139374 [details] [diff] [review]
update (nsLocalFileOSX)

>Index: xpcom/io/nsLocalFileOSX.cpp
>===================================================================

>+    if (chars) 
>+        aResult.Assign(chars, length);
>+    else {
>+        StBuffer<UniChar> buffer;
>+        if (!buffer.EnsureElemCapacity(length)) {
>+            CFRelease(inStr);
>+            CopyUTF8toUTF16(aSrc, aResult);
>+            return;

You don't need another CFRelease(inStr) and return here. Just let it fall thru
to the common cleanup below and r=ccarlen.

>+        }
>+        else {
>+            CFStringGetCharacters(inStr, CFRangeMake(0, length), buffer.get());
>+            aResult.Assign(buffer.get(), length);
>+        }
>+    }
>+    CFRelease(inStr);
>+    return;

You don't need the return statement here.

> }

Comment 66

[Jungshik Shin]

Comment on attachment 139374 [details] [diff] [review]
update (nsLocalFileOSX)

Asking for sr.
Thanks for r and catching my mistake (I shouldn't have worked at 5am in the
morning ;-)).

Comment 67

[Jungshik Shin]

Comment on attachment 139374 [details] [diff] [review]
update (nsLocalFileOSX)

asking darin for sr.

Comment 68

[Simon Fraser [no longer active]]

Comment on attachment 139374 [details] [diff] [review]
update (nsLocalFileOSX)

Add a comment above CopyUTF8toUTF16NFC to say what "NFC" means, and sr=sfraser
(Sorry for the delay, was on vacation)

Comment 69

[Jungshik Shin]

thanks for sr. fix checked into the trunk with reviewers' concerns addressed.

attachment 137537 [details] [diff] [review] doesn't seem to work for AppleDouble handling. 
I'm tempted to 'duplicate' the patch for nsFileSpec, but perhaps I have to
resist it and do the 'right thing' by migrating mailnews to nsI(Local)File,
which is a huge task. I'm making this bug depend on bug 71535. 

Comment 70

[Jungshik Shin]

Attachment: patch : nsFileSpec

I made this patch for nsFileSpec because getting rid of it in mailnews
(attachment/compose) is too much work for me at the moment. I wish I had more
time for that. Anyway, I built a 1.6 build (nsFileSpecUnix.cpp is identical in
1.6release and trunk) with this patch and attachment 139374 [details] [diff] [review] applied and asked
Mac users to test it. I've been just told that it worked well. 

What do you think of applying this patch as a temporary measure? If this is
checked in, we'd better get rid of attachment 137537 [details] [diff] [review] although we don't have to.

Comment 71

[Jungshik Shin]

A Jaguar (10.2) user reported that 1.6 build with attachment 140221 [details] [diff] [review] (patch for
nsFileSpec) didn't start and crashed. The crash log has the following:

OS Version: 10.2.8 (Build 6R73) 
Exception: EXC_BREAKPOINT (0x0006)
Code[0]: 0x00000001Code[1]: 0x8fe01220

Thread 0 Crashed:
#0 0x8fe01220 in halt
#1 0x8fe10654 in link_in_need_modules
#2 0x8fe10270 in bind_lazy_symbol_reference
#3 0x8fe00e60 in stub_binding_helper_interface
#4 0x0089c0b0 in NSGetModule
#5 0x0115ee2c in NSGetModule 
.......
#29 0x014ff5fc in NSGetModule
#30 0x01473db0 in 0x1473db0
#31 0x0000afac in getCountry(nsAString const&, nsAString&)
#32 0x0000bb0c in getCountry(nsAString const&, nsAString&)
#33 0x0000c260 in main
#34 0x00008aa4 in start
#35 0x00008918 in start 

Conrad, do you have any idea what's wrong? Seems like the dynamic linker
failed... BTW, as I wrote, on 10.3, there's no problem. 

Comment 72

[Conrad Carlen (not reading bugmail)]

(In reply to comment #71)
> A Jaguar (10.2) user reported that 1.6 build with attachment 140221 [details] [diff] [review] (patch for
> nsFileSpec) didn't start and crashed. The crash log has the following:

Given that stack, I'm not sure how it could be related. Can this person verify
that it works on his system without the patch?

I'm still against adding code to nsFileSpec. I realize the cleaning it from all
of mailnews is an overwhelming task but, short of that:
(1) remove it just from places involving attachments
(2) convert to a temp nsIFile, use the code you already added there, and convert
back to nsIFileSpec if you really need to. There are conversion functions like:
http://lxr.mozilla.org/seamonkey/source/xpcom/obsolete/nsIFileSpec.idl#197. I
think there's one to go the other way as well.

Comment 73

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I would support maintaining nsFileSpec if jshin is willing to do the work.  I
don't see why we should hurt users just because nobody is willing to do all the
work to convert all mailnews users to nsIFile.  Maintaining nsFileSpec shouldn't
have any effect on non-deprecated code or its users.

Comment 74

[Jungshik Shin]

re comment #72:

He had no problem running Mozilla 1.6 on his Mac. My build has two patches
applied (nsIFile patch and nsFileSpec patch) and I don't know whether it's both
or one of them. I asked him to try 1.7alpha nightly (with nsIFile patch
applied), but I'm afraid he may have already upgraded to Panther (he wrote to me
that he had ordered Panther). I also posted to macosx newsgroup and mozillazine
to ask for help. 

Bart, is your OS 10.2 or 10.3? Can you test it if it's 10.2? bzip2'd disk image
is in the 'usual' location on your mac. 

re fixing mailnews: I thought attachment 137573 [details] would do it because my
'emulation' on Linux worked well. What I overlooked was I have to take care of
AppleDouble on Mac OS (X). It's not yet clear to me which part of mailnews to
change in what way to handle AppleDouble, which is why I turned to fixing
nsFileSpec, instead. So, nsIFile <-> nsFileSpec conversion wouldn't help me. If
I had a Mac, it'd be a lot easier, but I have to work sorta 'blindly' although I
can build on Bart's Mac (I really appreciate him for his help in this regard).

Therefore, at the moment, I'm inclined to fix nsFileSpec (for the reason given
by dbaron). Needless to say, I have to make sure that it doesn't break mozilla
on 10.2. 

Comment 75

[Jungshik Shin]

I got a response in n.p.m.macosx newsgroup that 2004-01-31 build works fine on
Jaguar (10.2.8) while my build (built on Panther) crashed. 

I was referred to bug 224161. It's about a build made on Panther crashing on
Jaguar. Bart's machine runs Panther (uname string has  'Darwin 7.2' which
corresponds to Panther).  Because of the problem, all nightlies and release
builds have been built on Jaguar. With the the patch for bug 224161, a build
made on Panther can be run on Jaguar as well.  

In short, my patch for nsLocalFileOSX was confirmed to be fine on Jaguar and the
patch for nsFileSpec is likely to be fine as well. 

Comment 76

[Jungshik Shin]

Comment on attachment 140221 [details] [diff] [review]
patch : nsFileSpec

I was leaking mPath.GetLeaf('/') here.

>+#ifndef XP_MACOSX
>     return mPath.GetLeaf('/');
>+#else
>+    nsAutoString nameInNFC;
>+    CopyUTF8toUTF16NFC(nsDependentCString(mPath.GetLeaf('/')), nameInNFC);

The above line should be :

    char *name = mPath.GetLeaf('/');
    CopyUTF8toUTF16NFC(nsDependentCString(name), nameInNFC);
    nsCRT::free(name);

>+    return nsCRT::strdup(NS_ConvertUTF16toUTF8(nameInNFC).get());
>+#endif
> } // nsFileSpec::GetLeafName

Comment 77

[Jungshik Shin]

Comment on attachment 140221 [details] [diff] [review]
patch : nsFileSpec

Asking for r/sr (with the leak fixed as I wrote in the previous comment). 


The patch was confirmed to work fine on Jaguar. 

Conrad, unless you're absolutely against fixing nsFileSpec*, let me get away
with this for the now. I'll keep my eyes on future problems in nsFileSpecUnix.
Moreover, I hope I'll be able to convert mailnews to nsI*File.	
To avoid duplicating 'autobuffer' in nsLocalFileOSX.cpp, I'm just using
nsAutoPtr here. (btw, see bug 215421 for autobuffer)

Comment 78

[Conrad Carlen (not reading bugmail)]

Comment on attachment 140221 [details] [diff] [review]
patch : nsFileSpec

>Index: xpcom/obsolete/nsFileSpecUnix.cpp
>===================================================================
>+
>+    if (!sUnicodeNormalizer) {  // OS X 10.2 or earlier
>+        CopyUTF8toUTF16(aSrc, aResult);
>+        return;  
>+    }
>+

The comment above should say 10.1 and earlier since we'll have the converter on
10.2 and up.

r=ccarlen since it appears we have to use nsFileSpec in this case.

Comment 79

[Conrad Carlen (not reading bugmail)]

> To avoid duplicating 'autobuffer' in nsLocalFileOSX.cpp, I'm just using
nsAutoPtr here. (btw, see bug 215421 for autobuffer)

Also, see bug 161982. This reminds me - I need to check that in (sorry, I forgot
about it until now)

Comment 80

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 140221 [details] [diff] [review]
patch : nsFileSpec

>+#ifdef XP_MACOSX
>+#include "nsAutoPtr.h"
>+#endif

Don't ifdef includes of cross-platform files; just include unconditionally.

sr=dbaron

Comment 81

[Jungshik Shin]

Thanks for sr. As for nsAutoPtr.h, we need it only on Mac OS X, which is why I
#ifdef it. Or, there must be something I'm missing here... Anyway, nsAutoPtr
will be gone soon (once we fix bug 233485).

Comment 82

[Jungshik Shin]

thanks all ! finally fixed ! I also removed a call to UnicodeNormalizer() in
nsMsgAttachment (on Mac OS X) because we don't need it any more (at least on
10.2 or later)

Comment 83

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://mozilla.org/hacking/portable-cpp.html#dont_ifdef_include

You shouldn't ifdef cross-platform includes.  If someone writing code on Mac OS
X uses nsAutoPtr in non-ifdefed code, and doesn't notice that the include is
ifdefed, then the build will break when they check in.  This has happened a
number of times.

Comment 84

[Jungshik Shin]

Thanks for the explanation. I just trusted you and  got rid of #ifdef when
landing the patch. 

Comment 85

[Jungshik Shin]

Recently, I got a Mac and found that this issue is still present. It's odd that
I got the confirmation from a Korean mac user that it was fixed after I had
landed the patch. Some other check-ins since may have regressed it, but it's not
very likely. 

Besides, bug 238717 is a dupe of this bug. 

Comment 86

[Jungshik Shin]

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

Comment 87

[Jungshik Shin]

I regressed this while fixing bug 229872. I mistook |nsIURL->GetLeafName()| for
|nsIFile->GetLeafName()| (attachment 140940 [details] [diff] [review]). The latter (its implementation on
Mac OS X) takes care of NFD->NFC conversion while ther former doesn't. Given
this, we have to invoke our own normalizer (as we did in attachment 137537 [details] [diff] [review]).
I'm gonna upload a patch in a moment. 

Comment 88

[Jungshik Shin]

Attachment: patch

I've just tested this on Mac OS X and it worked well. 
I'm not very happy with shuffling back and forth between UTF-16 and UTF-8, but
this is not a perf-critical so that I guess it's Ok.

Comment 89

[Jungshik Shin]

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

asking for r/sr

Comment 90

[David :Bienvenu]

but, does this apply? nsIUnicodeNormalizer.h" is XP, right?

http://mozilla.org/hacking/portable-cpp.html#dont_ifdef_include

Comment 91

[Jungshik Shin]

Attachment: patch (alternative) that uses nsIFileURL and nsIFile

David, thank you for your quick review.  You're right that I shouldn't have
#ifdef'd  '#include <nsIU....>'.  

I came up with an alternative that uses nsIFileURL and nsIFile. This patch is
not only for Mac OS X but is XP. It dispense with explicit URL-unescaping and
conversion to NFC from NFD.  Instead, it relies on nsIFileURL and nsIFile. 
Because I changed nsLocalFileUnix (for OSX) such that  nsIFile->GetLeafName() 
returns file names in NFC.  

A problem with this patch is that  non-printable characters embedded in
filename might be a potential security concern. Currently, we keep them
escaped. 

David and Scott, which one do you like more?

Comment 92

[Jungshik Shin]

I think we have to go with a 'safer/easier' patch (Mac OS X only), that is
attahment 150449. Scott, can you give it a quick sr? Then, I'll try to get the
last minute a1.7

Comment 93

[Scott MacGregor]

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

ok..should we put this one on the branch and the alternative fix on the trunk ?

Comment 94

[Jungshik Shin]

thanks for sr.
for the now, let's just use the first patch for both trunk and branch(es). The
latter is cleaner but it has a drawback in that it doesn't escapes non-printable
characters. 

Comment 95

[Jungshik Shin]

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

asking for a1.7

affected users: a lot of non-English speaking users whose writing systems
include some decomposable characters (Latin/Cyrillc/Greek letters with
diacritics, Japanese and Korean among others. 

risk: A similar patch had been on mozill-tree back in February and it worked
fine. Anyway, it's only for Mac OS X.

Comment 96

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to 1.7

Comment 97

[Hiro]

Attachment: The screen shot after attachment 150449 was applied

In compase, it is attached by the right file name.
The screen shot before a patch is applied is as follows.
  http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2129&action=view

But the tool tip displayed when a mouse pointer is put on an attached file name
is not repaired yet.

Mac OS X 10.3.4
Thunderfox built from AVIARY_1_0_20040515_BRANCH today

Comment 98

[Jungshik Shin]

(In reply to comment #97)
Thanks for testing.

> But the tool tip displayed when a mouse pointer is put on an attached file name
> is not repaired yet.

That had better be dealt with in a separate bug. A similar problem manifests
itself when you browse local directories. There are two ways to fix it: 1) one
is to make Mozilla on OS X render strings in NFD exactly the same way as the
corresponding strings in NFC. 2) to implement nsIFileURL on OS X so that it can
take care of the unicode normalization. 

Comment 99

[Hiro]

(In reply to comment #98)
> That had better be dealt with in a separate bug.
O.K.
The problem of a tool tip may be reported separately.

I have the environment where the message created by Thnderbird of OSX can be
checked by Outlook of Windows.
Later, I think that I can transmit and check this test message in the windows
environment.
I think that it is probably OK. :-)

Comment 100

[Jungshik Shin]

(In reply to comment #99)

> Later, I think that I can transmit and check this test message in the windows
> environment.
> I think that it is probably OK. :-)

That's the very purpose of fixing this bug. When communicating with the external
world, Mac OS X should not expose NFD. I  tested it before landing the patch.

For the now, let's resolve this as fixed. We'll consider an alternative fix later.