nsCollationMac needs to be modernized, 64-bit compatible

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: jaas, Assigned: jaas)

Tracking

Trunk
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

24.60 KB, patch
smontagu
: review+
Details | Diff | Splinter Review
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.
Assignee: smontagu → jdaggett
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...
FWIW I tried to do this a little while ago in bug 421219, and ran into crashes with the "modern" collation APIs.
Posted patch fix v1.0 (obsolete) — Splinter Review
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.
Assignee: jdaggett → joshmoz
Attachment #373722 - Flags: review?(smontagu)
Specifically, the target buffer for the ::UCGetCollationKey call should be "mBuffer" not "key", that is why the nasty crashes.
Posted patch fix v1.1Splinter Review
Add back a buffer size optimization I removed during debugging.
Attachment #373722 - Attachment is obsolete: true
Attachment #373728 - Flags: review?(smontagu)
Attachment #373722 - Flags: review?(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.
Attachment #373728 - Flags: review?(smontagu) → review+
Blocks: 255192
Attachment #373728 - Flags: superreview?(roc)
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.
Hardware: x86 → x86_64
Attachment #373728 - Flags: superreview?(roc) → superreview+
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/3e5fe06a9d68
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(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 ;-)
(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 :)
Confirming that the testcase from bug 128323 does not crash.
You need to log in before you can comment on or make changes to this bug.