488320 - nsCollationMac needs to be modernized, 64-bit compatible

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Josh Aas]

nsCollationMac needs to be modernized and made 64-bit compatible. Things like "GetScriptVariable" and "smScriptSort" and "CompareText" are not available in 64-bit. Hopefully we can stop using old-school resources here altogether too.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Do we have a plan of stop the support of 10.4? 10.5 and later have new keyboard/IME processing APIs and we should use them, but 10.4 doesn't have...

Comment 2

[Simon Montagu :smontagu]

FWIW I tried to do this a little while ago in bug 421219, and ran into crashes with the "modern" collation APIs.

Comment 3

[Josh Aas]

Attachment: fix v1.0

This turns on the UC collation code and fixes the big crash, which was caused by using the wrong buffer to get a collation key. There isn't anything wrong with the collation APIs themselves as far as I can tell, someone just set up a temporary buffer for the key and then didn't use it so we wrote all over the wrong memory.

Comment 4

[Josh Aas]

Specifically, the target buffer for the ::UCGetCollationKey call should be "mBuffer" not "key", that is why the nasty crashes.

Comment 5

[Josh Aas]

Attachment: fix v1.1

Add back a buffer size optimization I removed during debugging.

Comment 6

[Simon Montagu :smontagu]

Comment on attachment 373728 [details] [diff] [review]
fix v1.1

r=me and thank you so much!
(In reply to comment #4)
> Specifically, the target buffer for the ::UCGetCollationKey call should be
> "mBuffer" not "key", that is why the nasty crashes.

So obvious as soon as somebody else sees it ;-)

Have you tested this with Korean data, as in bug 128323? Adding some unit tests would be great.

Comment 7

[Josh Aas]

Comment on attachment 373728 [details] [diff] [review]
fix v1.1

I haven't tested this much beyond looking through the character encoding menu. I really just cleaned up the pre-existing code and fixed the crash, made sure the character encoding menus looked reasonable. My knowledge of intl text is pretty thin, I'm probably not a good person to write tests in this area. I really just wanted to port to something that'll work in 64-bit Mac OS X.

Comment 8

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/3e5fe06a9d68

Comment 9

[[e-mail]]

(In reply to comment #6)
> (In reply to comment #4)
> > Specifically, the target buffer for the ::UCGetCollationKey call should be
> > "mBuffer" not "key", that is why the nasty crashes.
> So obvious as soon as somebody else sees it ;-)
He's saying that as the reviewer of record for the bogus code ;-)

Comment 10

[Simon Montagu :smontagu]

(In reply to comment #9)
> > So obvious as soon as somebody else sees it ;-)
> He's saying that as the reviewer of record for the bogus code ;-)

I think you misunderstood what I wrote. 

I don't find this kind of finger-pointing constructive, so I won't ask who wrote the bogus code ;-) but Neil's comment does point to the answer to a question that was bothering me: the Unicode collation code was originally backed out because of crashes that only occurred in sorting Korean text (bug 128323), so why were Jungshik and I both seeing crashes much more often when we tried to re-enable it (bug 255192 and 421219 respectively)?

The crash that we were seeing must have been what is described in comment 4, so the crashes from bug 128323 are very likely still going to happen. Stay tuned for the results of testing :)

Comment 11

[Simon Montagu :smontagu]

Confirming that the testcase from bug 128323 does not crash.