nsCollationMac needs to be modernized, 64-bit compatible

RESOLVED FIXED

Status

()

Core
Internationalization
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

Trunk
x86_64
Mac OS X
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
(Assignee)

Description

9 years ago
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)

Updated

9 years ago
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.
(Assignee)

Comment 3

9 years ago
Created attachment 373722 [details] [diff] [review]
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.
Assignee: jdaggett → joshmoz
Attachment #373722 - Flags: review?(smontagu)
(Assignee)

Comment 4

9 years ago
Specifically, the target buffer for the ::UCGetCollationKey call should be "mBuffer" not "key", that is why the nasty crashes.
(Assignee)

Comment 5

9 years ago
Created attachment 373728 [details] [diff] [review]
fix v1.1

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
(Assignee)

Updated

9 years ago
Attachment #373728 - Flags: superreview?(roc)
(Assignee)

Comment 7

9 years ago
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.
(Assignee)

Updated

9 years ago
Hardware: x86 → x86_64
Attachment #373728 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 8

9 years ago
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/3e5fe06a9d68
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 9

9 years ago
(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.