Closed Bug 1045958 Opened 10 years ago Closed 10 years ago

TB 31 jp does not display folder pane with OS X [Mac]

Categories

(Thunderbird :: Folder and Message Lists, defect)

31 Branch
x86
macOS
defect
Not set
major

Tracking

(thunderbird35 fixed, thunderbird_esr3135+ fixed)

RESOLVED FIXED
Thunderbird 35.0
Tracking Status
thunderbird35 --- fixed
thunderbird_esr31 35+ fixed

People

(Reporter: sh, Assigned: arai)

References

Details

(5 keywords, Whiteboard: [regression:TB31])

Attachments

(17 files, 20 obsolete files)

1.75 KB, text/plain
Details
249.63 KB, application/zip
Details
19.36 KB, patch
arai
: review+
Details | Diff | Splinter Review
1.02 KB, patch
arai
: review+
jaas
: feedback+
Details | Diff | Splinter Review
5.18 KB, patch
glandium
: review+
Details | Diff | Splinter Review
19.34 KB, patch
jaas
: review+
Details | Diff | Splinter Review
859 bytes, patch
jaas
: review+
Details | Diff | Splinter Review
19.41 KB, patch
jaas
: review+
Details | Diff | Splinter Review
514.88 KB, image/png
Details
920 bytes, patch
arai
: feedback-
Details | Diff | Splinter Review
28.76 KB, patch
Details | Diff | Splinter Review
19.24 KB, patch
Details | Diff | Splinter Review
3.60 KB, patch
Details | Diff | Splinter Review
10.00 KB, patch
standard8
: review+
Details | Diff | Splinter Review
19.29 KB, patch
jaas
: review+
Details | Diff | Splinter Review
10.13 KB, patch
hiro
: review+
Details | Diff | Splinter Review
1.95 KB, patch
hiro
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140716183446

Steps to reproduce:

OS: Mac OS X Mavericks 10.9.4

Upgrade to Thunderbird 31.


Actual results:

The left folder pane does not show up.

Some other people report the same problem with screenshots (in Japanese).

http://forums.mozillazine.jp/viewtopic.php?f=3&t=14887

By starting Thunderbird 31 in 32bit mode, we can avoid this problem as they say in the forum.


Expected results:

The left folder pane shows up.
sh, are you one of the posters at http://forums.mozillazine.jp/viewtopic.php?f=3&t=14887 ?

And does starting in safe mode help?
Flags: needinfo?(sh)
Summary: TB 31 does not display folder pane → TB 31 jp does not display folder pane with OS X 10.9 Mavericks
Please comment on the release notes not listing OS X 10.9. http://www.mozilla.jp/thunderbird/31.0/system-requirements/  Is it an error?

And also the issue reported here about folder pane, 32-bit, etc.
Thanks

Also reported at https://support.mozilla.org/en-US/questions/1012272
Component: Untriaged → Folder and Message Lists
Flags: needinfo?(masayuki)
Flags: needinfo?(hiikezoe)
Flags: needinfo?(bugzilla)
(In reply to Wayne Mery (:wsmwk) from comment #1)
> sh, are you one of the posters at
> http://forums.mozillazine.jp/viewtopic.php?f=3&t=14887 ?

No. I'm not.

> 
> And does starting in safe mode help?

Starting Thunderbird 31.0 in safe mode does not help.
I cannot reproduce this bug with Thunderbird nor Daily on Mavericks.

And also I don't know if it's a mistake of the release note...
Flags: needinfo?(masayuki)
Flags: needinfo?(sh)
I could reproduce this bug when I reboot Tb31 a couple of times.

Then, removing localstore.rdf helps to recover the window.

I don't see the cause from the error log. When this bug occurs, I cannot use some features, e.g., changing window layout from Options -> Layout.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hmm, I cannot reproduce this bug again...
I have created a new account on my iMac (Mavericks 10.9.4).

When I start Thunderbird not in 32bit mode in the account, the folder pane does not show up.

When I set English as preferred language in System Preferences on Mavericks,
and I start Thunderbird not in 32bit mode,
the folder pane shows up. 

I hope this would help.
(In reply to sh from comment #7)
> I have created a new account on my iMac (Mavericks 10.9.4).
> 
> When I start Thunderbird not in 32bit mode in the account, the folder pane
> does not show up.

and still nothing in error console?


> When I set English as preferred language in System Preferences on Mavericks,
> and I start Thunderbird not in 32bit mode,
> the folder pane shows up. 
> 
> I hope this would help.

awesome info.
Severity: normal → major
(In reply to Wayne Mery (:wsmwk) from comment #8)
> (In reply to sh from comment #7)
> > I have created a new account on my iMac (Mavericks 10.9.4).
> > 
> > When I start Thunderbird not in 32bit mode in the account, the folder pane
> > does not show up.
> 
> and still nothing in error console?

Nothing attract my attention.
I copy and paste messages in error console.
There is no difference of messages between when English selected and when Japanese selected
in System Preferences.

---- begin ------

Could not read chrome manifest 'file:///Applications/Thunderbird.app/Contents/MacOS/chrome.manifest'.

Could not read chrome manifest 'file:///Applications/Thunderbird.app/Contents/MacOS/extensions/%7B972ce4c6-7e08-4474-a285-3208198ce6fd%7D/chrome.manifest'.

mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create
resource://gre/components/steelApplication.js

line: 783


OpenGL compositor Initialized Succesfully.
Version: 2.1 NVIDIA-8.26.26 310.40.45f01
Vendor: NVIDIA Corporation
Renderer: NVIDIA GeForce GT 755M OpenGL Engine
FBO Texture Target: TEXTURE_2D


OpenGL compositor Initialized Succesfully.
Version: 2.1 NVIDIA-8.26.26 310.40.45f01
Vendor: NVIDIA Corporation
Renderer: NVIDIA GeForce GT 755M OpenGL Engine
FBO Texture Target: TEXTURE_2D

---- end -----
The problem is sometimes reproduced for me too, on following env:
  * Mac OS X 10.9.4
  * Thunderbird 31 ja-JP-mac
    Thunderbird 31 en-US
    Thunderbird 32 Beta ja-JP-mac
  * clean profile + single IMAP (icloud) account
Not yet happened in Daily 34.a1.

Error messages are shown when I set "javascript.options.showInConsole" true.

This message is shown every time I open mail window.
> Timestamp: 2014/08/01 20:41:39
> Error: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgFolder.compareSortKeys]
> Source File: chrome://messenger/content/folderPane.js
> Line: 2647

Seems to be dup of Bug 976183.

Another error message, this is shown every time I close mail window.
> Timestamp: 2014/08/01 20:43:00
> Error: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]
> Source File: chrome://messenger/content/specialTabs.js
> Line: 1220

Both messages above are no longer shown after folder tree is shown correctly.


About 1st error message, here is the call stack of sortFolderItems.
  http://mxr.mozilla.org/comm-release/source/mail/base/content/folderPane.js#2645
> sortFolderItems@chrome://messenger/content/folderPane.js:2660:3
> ftvItem.prototype.children@chrome://messenger/content/folderPane.js:2241:7
> ftv_recursivelyAddToMap@chrome://messenger/content/folderPane.js:965:1
> toggleRow@chrome://messenger/content/folderPane.js:1013:7
> openLevel@chrome://messenger/content/folderPane.js:1277:11
> ftv__persistOpenStates@chrome://messenger/content/folderPane.js:1287:5
> ftv__rebuild@chrome://messenger/content/folderPane.js:1366:5
> ftv_load@chrome://messenger/content/folderPane.js:191:5
> InitPanes@chrome://messenger/content/msgMail3PaneWindow.js:936:1
> LoadPostAccountWizard@chrome://messenger/content/msgMail3PaneWindow.js:493:3
> OnLoadMessenger@chrome://messenger/content/msgMail3PaneWindow.js:478:5
> onload@chrome://messenger/content/messenger.xul:1:1
The exception is thrown inside the sorter function in it. This function is called during rebuilding folder pane, so folder pane is not shown correctly if the error happens.


After I close and open the mail window again without quitting application, sometimes the problem is solved and folder tree is shown. But once the tree is shown, it does not disapper again with this action.
Attached file regression range
(Sorry, I was wrong, in my last comment #10, it's not "Thunderbird 32 Beta ja-JP-mac", but "Thunderbird 31 Beta ja-JP-mac")

Reproduced on Daily 30.0a1, and here is regression range.

Last good: https://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2014/02/2014-02-14-03-02-01-comm-central/
  http://hg.mozilla.org/mozilla-central/rev/6687d299c464
  https://hg.mozilla.org/comm-central/rev/1ce77c2f9bd0

First bad: https://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2014/02/2014-02-15-03-02-03-comm-central/
  http://hg.mozilla.org/mozilla-central/rev/b80f7eece913
  https://hg.mozilla.org/comm-central/rev/237c7a7e3f20

Pushlogs:
  http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6687d299c464&tochange=b80f7eece913
  http://hg.mozilla.org/comm-central/pushloghtml?fromchange=1ce77c2f9bd0&tochange=237c7a7e3f20


Then, this problem does not exist on Daily 34.0a1 (2014-07-23), so there should be a fix for it, here is the range.

Last bad: https://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2014/05/2014-05-23-03-02-03-comm-central/
  https://hg.mozilla.org/mozilla-central/rev/e9b2b72f4e6c
  https://hg.mozilla.org/comm-central/rev/e39749aee0ce

First good: https://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2014/05/2014-05-24-03-02-04-comm-central/
  https://hg.mozilla.org/mozilla-central/rev/f2f81e83f4ff
  https://hg.mozilla.org/comm-central/rev/7f20f6b479da

Pushlogs:
  http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e9b2b72f4e6c&tochange=f2f81e83f4ff
  http://hg.mozilla.org/comm-central/pushloghtml?fromchange=e39749aee0ce&tochange=7f20f6b479da


However, even the problem is fixed on Daily 32.0a1 (2014-05-24-03-02-04), the problem still exists on Earlybird 32.0a2 or newer. So, the regression happens again at least twice on 32.0a2 (2014-06-10-00-40-01) and 33.0a2 (2014-07-22-00-40-01).


I attached the result of some other builds.
UCCreateCollator sometimes returns paramErr (error in user parameter list), even exactly same parameters are passed.
  http://mxr.mozilla.org/comm-release/source/mozilla/intl/locale/src/mac/nsCollationMacUC.cpp#89
> err = ::UCCreateCollator(mLocale, opVariant, newOptions, &mCollator);

here, parameters are following (both for success and failure case):
  mLocale: 0x90 (ja_JP)
  opVariant: 0
  newOptions: 0000801e (kUCCollateStandardOptions | kUCCollatePunctuationSignificantMask | kUCCollateCaseInsensitiveMask | kUCCollateDiacritInsensitiveMask)
  mCollator: 0x0
  &mCollator: 0x10cfbffa8

UCCreateCollator is called inside nsCollationMacUC::EnsureCollator, here is call stack for it:
  nsCollationMacUC::EnsureCollator
  nsCollationMacUC::AllocateRawSortKey
  nsMsgDBFolder::CreateCollationKey
  nsMsgDBFolder::GetSortKey
  nsMsgDBFolder::CompareSortKeys

So, different from the discussion in Bug 976183, the error happens in earlier step in nsMsgDBFolder::CompareSortKeys, UCCompareCollationKeys is called later in it.
Anyway, this problem seems to be a trouble with CoreService framework.


Then, this problem seems to be related to memory allocation, especially MOZ_REPLACE_MALLOC and MOZ_MEMORY.
When I disable MOZ_REPLACE_MALLOC in latest comm-central, folder pane is not shown sometimes, same as comm-aurora and comm-release's case.
I'll look into it.
if I disable MOZ_MEMORY in comm-release, folder pane is always shown in 64bit mode.
This option was enabled only in 64bit mode, so the problem happened only in 64bit mode.
First regression on 2014-02-15-03-02-03-comm-central was caused by Bug 860254, memory poisoning.

Last good
  http://hg.mozilla.org/mozilla-central/rev/5b69776cb061
  https://hg.mozilla.org/comm-central/rev/237c7a7e3f20

First bad
  http://hg.mozilla.org/mozilla-central/rev/00ea960e8164
  https://hg.mozilla.org/comm-central/rev/237c7a7e3f20
(used same revision for comm-central)


Next, I'll look into the fix on 2014-05-24-03-02-04-comm-central, but I guess it's one of malloc related commits, especially replace-malloc.
Confirmed that this problem is fixed by replace-malloc, in Bug 999913.

Last bad
  http://hg.mozilla.org/mozilla-central/rev/0c5047d370fc
  https://hg.mozilla.org/comm-central/rev/7f20f6b479da

First good
  http://hg.mozilla.org/mozilla-central/rev/a9f95997fe57
  https://hg.mozilla.org/comm-central/rev/7f20f6b479da
(used same revision for comm-central)

However, it's enabled only in nightly (Daily), so this problem still exists in Earlybird, Beta and Release.
Hmm, this bug does not happen when I debug it slowly, so it's too hard to debug :(

On Thunderbird 31, u_terminateChars(*) in libicucore.A.dylib sometimes fails with U_BUFFER_OVERFLOW_ERROR, because |length > destCapacity|.
length seems to be random value (around 1000..4000), destCapacity is 157.
I haven't yet figured out why it happens.
(* I'm not sure the actual function name because it's stripped)

If this problem is caused by the bug inside jemalloc itself, or some confliction between jemalloc, CoreService and ICU, disabling it could be a hot-fix.
On the other hand, if this bug is caused by some other bug, it's not a good idea to avoid it by disabling jemalloc, and we should fix the bug itself.


Here is trace of the problem.
(note: in following, I use link to ICU source in mozilla-release for reference, but I guess it's different version than Maverick's built-in ICU)

1. __TERMINATE_STRING  macro sets *pErrorCode to U_BUFFER_OVERFLOW_ERROR
   because length > destCapacity
  http://hg.mozilla.org/releases/mozilla-release/file/cd52a7f89548/intl/icu/source/common/ustring.cpp#l1446
2. u_terminateChars fails with U_BUFFER_OVERFLOW_ERROR
  http://hg.mozilla.org/releases/mozilla-release/file/cd52a7f89548/intl/icu/source/common/ustring.cpp#l1458
3. _canonicalize fails with U_BUFFER_OVERFLOW_ERROR
  http://hg.mozilla.org/releases/mozilla-release/file/cd52a7f89548/intl/icu/source/common/uloc.cpp#l1861
4. uloc_getBaseName fails with U_BUFFER_OVERFLOW_ERROR
  http://hg.mozilla.org/releases/mozilla-release/file/cd52a7f89548/intl/icu/source/common/uloc.cpp#l2048
5. ures_open fails with U_ILLEGAL_ARGUMENT_ERROR, and returns NULL
   because *status is U_BUFFER_OVERFLOW_ERROR
  http://hg.mozilla.org/releases/mozilla-release/file/cd52a7f89548/intl/icu/source/common/uresbund.cpp#l2073
6. b is NULL in ucol_open_internal
  http://hg.mozilla.org/releases/mozilla-release/file/cd52a7f89548/intl/icu/source/i18n/ucol_res.cpp#l179
7. ures_getByKey fails with U_ILLEGAL_ARGUMENT_ERROR, and returns fillIn(NULL)
   because resB is NULL
  http://hg.mozilla.org/releases/mozilla-release/file/cd52a7f89548/intl/icu/source/common/uresbund.cpp#l1803
8. collations in NULL in ucol_open_internal
  http://hg.mozilla.org/releases/mozilla-release/file/cd52a7f89548/intl/icu/source/i18n/ucol_res.cpp#l182
9. ures_getByKeyWithFallback fails with U_ILLEGAL_ARGUMENT_ERROR
   because resB is NULL
  http://hg.mozilla.org/releases/mozilla-release/file/cd52a7f89548/intl/icu/source/common/uresbund.cpp#l1725
10. ucol_open_internal fails with U_INTERNAL_PROGRAM_ERROR
   because intStatus is U_ILLEGAL_ARGUMENT_ERROR
  http://hg.mozilla.org/releases/mozilla-release/file/cd52a7f89548/intl/icu/source/i18n/ucol_res.cpp#l198
11. ucol_open fails with U_INTERNAL_PROGRAM_ERROR
  http://hg.mozilla.org/releases/mozilla-release/file/cd52a7f89548/intl/icu/source/i18n/ucol_res.cpp#l356
12. SetICUCollator (in CarbonCore) returns with paramErr
    because of ucol_open failure
13. UCCreateCollator (in CarbonCore) returns paramErr
I found a use-after-free bug inside UCCreateCollator (already reported to bugreport.apple.com)

There, C string is freed before use, and if we enable memory poisoning, the content of the string will be initialized by 0x5a, and the length of it will be a random value, because of missing Null-terminator.
(not sure why this does not happen with replace-malloc)

Anyway, here is some solutions:
  1. use code in intl/locale/unix instead of intl/locale/mac (not sure whether it works or not)
  2. use raw ICU instead of UC* functions in CoreServices
  3. pass NULL as locale parameter for UCCreateCollator. this will change its behavior, but can avoid the buggy code path
  4. disable memory poisoning (this is not a actual fix, but situation will be same as Thunderbird 24.x)
  5. just wait for the fix on apple's side :)
  6. or any other ideas?

wsmwk, how do you think?
Flags: needinfo?(vseerror)
I'm not a good person to ask, and bug perhaps Steven or standard8
Flags: needinfo?(vseerror) → needinfo?(smichaud)
Flags: needinfo?(smichaud)
See Also: → 985026
hmm, I don't have authority to see bug 985026...
Let me know if there is anything else I can do :)
I just added everyone here to the CC list for bug 985026.
(In reply to Wayne Mery (:wsmwk) from comment #2)
> Please comment on the release notes not listing OS X 10.9.
> http://www.mozilla.jp/thunderbird/31.0/system-requirements/  Is it an error?

It's maybe just webdev (localization) error.
I've reported to mozilla.jp webdev.
Flags: needinfo?(bugzilla)
Group: core-security
I'm marking this as sec-other because it sounds like an OS-level issue that is going to have to be worked around on our side.
Keywords: sec-other
In MozillaZine forum (in comment #0), there reported that folders are sorted differently even if folder tree is shown.
I guess it's because of corrupted locale string passed to SetICUCollator, and they are sorted differently than "ja-JP"'s case.
Created draft patch for "2. use raw ICU instead of UC* functions in CoreServices" in comment #17.
This patch is not yet fully tested, but confirmed that folder pane is always shown.

There are some differences between current nsICollation and patched one,
so sort order will become different (or unstable) in some case, I'll fix them if needed.
  1. if 2 characters are totally different (e.g. 'a' and 'X'), current comapreString returns -2, but patched one returns -1
  2. if 2 characters are different only in their case (e.g. 'a' and 'A'), current comapreString returns -1 for all flags, but patched one returns 0 for case insensitive comparison, and -1 for case sensitive comparison
  3. same as 2 for accent (e.g. 'a' and 'ä')

I'll attach testcase and its logs in next comment.


Steven, is this approach acceptable?
or should we choose another approach?


Anyway, if it's permitted to disable memory poisoning (or entire MOZ_MEMORY), it should be the simplest hot-fix :)
Attachment #8481690 - Flags: feedback?(smichaud)
Comment on attachment 8481690 [details] [diff] [review]
Use raw ICU instead of CoreServices API in unicode collation on Mac OS X.

> Steven, is this approach acceptable?

I frankly don't know :-(

What bothers me is the changes it makes to the sort order.  But I know almost nothing about ICU code, so I can't evaluate how serious a problem this is.

Aside from that, the general idea seems fine.  As best I can tell, the ICU code is present on OS X (in /usr/lib/libicucore.dylib) at least as far back as OS X 10.6 (the earliest version we still support).  So we don't need to worry about it ever being absent.

But you should really ask someone who knows more about ICU than I do.  You'll also need someone with that expertise to review your patch.  Maybe Masatoshi Kimura?

> Anyway, if it's permitted to disable memory poisoning (or entire
> MOZ_MEMORY), it should be the simplest hot-fix :)

I doubt you'll get permission to do this :-)  I'd also be against it.

The simplest solution would be for Apple to fix their bug.  But they haven't yet done so, even though it was reported to them in the clearest terms (at bug 985026).  And even if they did fix it, it'd only be in the current version of OS X.
Attachment #8481690 - Flags: feedback?(smichaud)
Attachment #8481690 - Flags: feedback?(VYV03354)
Comment on attachment 8481690 [details] [diff] [review]
Use raw ICU instead of CoreServices API in unicode collation on Mac OS X.

I don't know much about the latest Mac platform. Josh?
Attachment #8481690 - Flags: feedback?(VYV03354) → feedback?(joshmoz)
Comment on attachment 8481690 [details] [diff] [review]
Use raw ICU instead of CoreServices API in unicode collation on Mac OS X.

Review of attachment 8481690 [details] [diff] [review]:
-----------------------------------------------------------------

I don't have any objection to using ICU instead of OS X core services if we can get a patch that works. We're already shipping with the ICU library and using it here gives us a lot of control over a very complex system. We can fix bugs when we want to instead of depending on Apple.

How much do the sorting result changes really matter?

(note, I took a high-level look at the patch but didn't actually review it)
Attachment #8481690 - Flags: feedback?(joshmoz) → feedback+
Thank you for feedbacks :)

(In reply to Josh Aas (Mozilla Corporation) from comment #28)
> How much do the sorting result changes really matter?

Changes to the sort order are following:

1. (as noted in 2 and 3 in comment #24) If two strings are same in specified strength, sorting order is depend on the order in the original list (and the stability of the sorting algorithm).

Folders are sorded with kCollationCaseInSensitive strength, so case and accent will be ignored.
For example, if there is 2 folders named "AAA" and "âãä", displayed order of them is unknown, and will be different than current Thunderbird in some case.

I guess this can be fixed by comparing them again with UCOL_TERTIARY or higher strength (corresponds to kCollationCaseSensitive). CoreServices API returns almost same value. (in that case, I need some trick to store 2 sort keys into single buffer in AllocateRawSortKey method)


2. Some different results between ICU and CoreServices (or I set wrong parameters...)
Currently the difference found only in ja_JP locale (tested en-US, de-DE, fr, sv-SE, but no difference found for now).

Here is an examples:

'あ' (U+3042: HIRAGANA LETTER A) and 'А' (U+0410: CYRILLIC CAPITAL LETTER A)
  ICU:          'あ' < 'А' on any strength
  CoreServices: 'あ' > 'А' on any strength

I guess CoreServices returns Unicode code point order for this case, and ICU returns JIS code point order ('あ'=2420, 'А'=2721).
I'm not good at unicode collation, but CoreServices API might pass extra rules to ICU internally.
Josh, do you have any idea?


Then, I think difference 1 in comment #24 (-2 returned on totally different characters) does not have any effect if the value is used only for sorting.


By the way, I found similar implementation in Intl API in SpiderMonkey.
I'll look into it (already found some differences between it and previous patch, now I'm testing fixed version)
  http://dxr.mozilla.org/mozilla-central/source/js/src/builtin/Intl.cpp#827
  http://dxr.mozilla.org/mozilla-central/source/js/src/builtin/Intl.cpp#958
Flags: needinfo?(joshmoz)
Sorry, there are more differences in many locales.
I'll post detail later (now comparing them...)
Attached test programs that sorts characters (test_sort.cc), and compares sorting orders (do_test.sh), and their output files (logs/*).

Tested following locales:
  en_US
  fr_CA
  ja_JP
  sv_SE

4 directories inside each locale dir corresponds to each strength:
  0: kCollationCaseSensitive
  1: kCollationCaseInsensitiveAscii
  2: kCollationAccentInsenstive
  3: kCollationCaseInSensitive

Inside those directories, each file contains the sorting order difference for each range:
  0.txt: U+0020...U+0FFF
  1.txt: U+1000...U+0FFF
  ...
  15.txt: U+F000...U+FFFF
  (not tested U+10000 or higher range)

x.txt contains the reduced sorting order difference in U+0020...U+FFFF range, every 8 characters.

In each file, left column is CoreServices's output, and right column is ICU's output.


There are so much differences in all locales :(
I'll try changing attribute and passing extra rules to emulate CoreServices API's output.
Assignee: nobody → arai_a
Flags: needinfo?(joshmoz)
(In reply to Tooru Fujisawa [:arai] from comment #31)

I'm not sure we really need to put a bunch of effort into emulating CoreServices. Doing something that seems correct seems more important than matching CoreServices, and I don't think we have any evidence that CoreServices is more correct than ICU.

Your testing seems to be iterating over all possible characters, but in real-world applications most characters are never used. We only need to get a relatively small subset of comparisons to match expectations in order to succeed with real-world data. I assume ICU does what I'd expect for 1-9 and a-z, and I have no reason to believe it does significantly worse for any other locale.

I'm not really an expert on this though, so don't assign more credibility to my comments than they deserve :)

Why don't you put together a patch that allows us to use ICU or CoreServices, selectable via a hidden pref. Set the pref to ICU by default to resolve this bug, land it, and if it turns out we need to do more work we can just flip the pref back to CoreServices until the ICU code is ready.
Sorry, I found a bug in the testcode in comment #31, strength setting for kCollationAccentInsenstive and kCollationCaseInsensitiveAscii are swpped in compareICU function.
Same bug exists in previous patch, I'll fix it in next patch.

(In reply to Josh Aas (Mozilla Corporation) from comment #32)
> I'm not sure we really need to put a bunch of effort into emulating
> CoreServices. Doing something that seems correct seems more important than
> matching CoreServices, and I don't think we have any evidence that
> CoreServices is more correct than ICU.

If it's okay to use ICU's default behavior, it's simpler solution :)
Also, the sorting order will get same as JavaScript Intl API's one, if we don't need extra customization to collation rule.

> Your testing seems to be iterating over all possible characters, but in
> real-world applications most characters are never used. We only need to get
> a relatively small subset of comparisons to match expectations in order to
> succeed with real-world data. I assume ICU does what I'd expect for 1-9 and
> a-z, and I have no reason to believe it does significantly worse for any
> other locale.

Yes, most of the differences are related only to accent (except CJK chars in ja_JP), so the sorting order will be almost same as berfore if there is no accent.

I also think that ICU's sorting order is not worse than CoreServices's one, just worried about the difference from previous versions (similar problem was already reported in the MozillaZine forum, as noted in comment #23, I'm not sure how serious it is though).

> Why don't you put together a patch that allows us to use ICU or
> CoreServices, selectable via a hidden pref. Set the pref to ICU by default
> to resolve this bug, land it, and if it turns out we need to do more work we
> can just flip the pref back to CoreServices until the ICU code is ready.

That's a nice idea, thanks :)
I'll fix the patch, and add automated tests.
Okay, here is updated patch.

I don't change anything about differences noted in comment #29.
About return value, 0 seems to be correct when 2 strings are same in specified strength.
If we want to make the sorting order stable, we should use case/accent sensitive comparison.

Testcases are copied from JavaScript Intl API's test (some tests are removed, because nsICollation does not support those parameters):
  http://dxr.mozilla.org/mozilla-central/source/js/src/tests/Intl/Collator/compare.js

About pref name, I use "intl.collation.mac.use_icu" for now, if there is some rule for pref naming, or a better name, please tell me :)

I confirmed that test_collation_mac_icu.js is passed on Mac OS X 10.9.4 64bit build (applied to mozilla-central), and folder pane is shown(applied to comm-central).
No other tests are done (I have to figure out which tests are affected).
Attachment #8481690 - Attachment is obsolete: true
Attachment #8481691 - Attachment is obsolete: true
Attachment #8485321 - Flags: review?(joshmoz)
Comment on attachment 8485321 [details] [diff] [review]
Use raw ICU instead of CoreServices API in Unicode collation on Mac OS X.

Sorry, previous strength setting was correct (I mis-understood Intl API's parameter).
I'll post fixed version soon.
Attachment #8485321 - Flags: review?(joshmoz)
Hmm, maybe I'm encountering some other bug around CoreServices API, and return value of UCCompareText is still wrong for some case, so the sorting order shown in attachment 8485148 [details] may totally wrong.
Fixed strength for kCollationCaseInsensitiveAscii and kCollationAccentInsenstive.
Also, make other attributes same as Intl.Collator's default.
Attachment #8485321 - Attachment is obsolete: true
Attachment #8485385 - Flags: review?(joshmoz)
Updated sorting order comparison, now it uses xpcshell in un-patched (CoreServices) and patched (ICU) build, and use nsICollation to compare strings.

Also, use only Latin1 range characters and Alphabet in each language for sorting.

Results are totaly different from previous one :O
I guess UCCompareText in previous testcase returns wrong value (I'm not sure why), and it's different from nsICollation's return value in current mozilla-central.

There is almost no critical difference.

In all locales, lower letter and upper letter are swapped in case insensitive sorting, and letter with accent and without accent are swapped in some case in accent insensitive sorting. But it depends on situation (order in original list, etc), because they are same.

In ja_JP, I use all Shift_JIS characters, for simplicity, but some of them are not actually used in almost case. Greek and Cyrillic alphabets are placed after all other characters, and a few marks are sorted differently, but I think it's not a big problem.
Attachment #8485148 - Attachment is obsolete: true
Comment on attachment 8485385 [details] [diff] [review]
Use raw ICU instead of CoreServices API in Unicode collation on Mac OS X.

Review of attachment 8485385 [details] [diff] [review]:
-----------------------------------------------------------------

::: intl/locale/mac/nsCollationMacUC.cpp
@@ +28,3 @@
>  {
> +  nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
> +  if (prefs)

Please always use curly braces with if blocks.

@@ +54,5 @@
> +}
> +
> +nsresult nsCollationMacUC::ConvertStrength(const int32_t aNSStrength,
> +                                           UCollationStrength *aICUStrength,
> +                                           UColAttributeValue *aCaseLevelOut)

* on the left

@@ +59,5 @@
> +{
> +  NS_ENSURE_ARG_POINTER(aICUStrength);
> +  NS_ENSURE_TRUE((aNSStrength < 4), NS_ERROR_FAILURE);
> +
> +  UCollationStrength strength;

You're declaring this without initializing it, then doing a non-exhaustive switch to assign to it. This is dangerous because you might end up returning an uninitialized value. Please either initialize when you declare it, and/or put a default in the switch statement and assert.

@@ +109,5 @@
> +  NS_ENSURE_TRUE(NS_SUCCEEDED(res) && !localeString.IsEmpty(),
> +                 NS_ERROR_FAILURE);
> +  NS_LossyConvertUTF16toASCII tmp(localeString);
> +  tmp.ReplaceChar('-', '_');
> +  char *locale = (char *)PR_Malloc(tmp.Length() + 1);

I think you want to use moz_malloc here, and make sure to free with moz_free. Please replace any other use of PR_Malloc in this file while you're at it.

@@ +150,5 @@
> +      ucol_close(mCollatorICU);
> +      mHasCollator = false;
> +    } else {
> +      err = ::UCDisposeCollator(&mCollator);
> +      NS_ENSURE_TRUE((err == noErr), NS_ERROR_FAILURE);

How come you're not setting mHasCollator to false in this case?

@@ +151,5 @@
> +      mHasCollator = false;
> +    } else {
> +      err = ::UCDisposeCollator(&mCollator);
> +      NS_ENSURE_TRUE((err == noErr), NS_ERROR_FAILURE);
> +    }

This is the second time I've seen this code, you might want to create a utility function called "CleanUpCollator" that calls the correct close/dispose and sets mHasCollator to false.

@@ +157,5 @@
>  
> +  if (mUseICU) {
> +    UErrorCode status;
> +    status = U_ZERO_ERROR;
> +    mCollatorICU = ucol_open(mLocaleICU, &status);

Do you need to check that mLocaleICU is valid here?

@@ +172,5 @@
> +    ucol_setAttribute(mCollatorICU, UCOL_ALTERNATE_HANDLING, UCOL_DEFAULT, &status);
> +    ucol_setAttribute(mCollatorICU, UCOL_NUMERIC_COLLATION, UCOL_OFF, &status);
> +    ucol_setAttribute(mCollatorICU, UCOL_NORMALIZATION_MODE, UCOL_ON, &status);
> +    ucol_setAttribute(mCollatorICU, UCOL_CASE_FIRST, UCOL_DEFAULT, &status);
> +    NS_ENSURE_TRUE(U_SUCCESS(status), NS_ERROR_FAILURE);

This is only checking the result of the last ucol_setAttribute call.

@@ +233,5 @@
> +
> +    int32_t keyLength = ucol_getSortKey(mCollatorICU, str, stringInLen, nullptr, 0);
> +    NS_ENSURE_TRUE((stringInLen == 0 || keyLength > 0), NS_ERROR_FAILURE);
> +
> +    uint8_t *newKey = (uint8_t *)PR_Malloc(keyLength + 1);

PR_Malloc -> moz_malloc

@@ +329,5 @@
>    NS_ENSURE_ARG_POINTER(result);
>    *result = 0;
>  
> +  if (mUseICU) {
> +    int32_t tmpResult = PL_strcmp((const char *)key1, (const char *)key2);

I dislike using PL_ functions, just use strcmp here. It's mac-only code, we don't need to worry much about cross-platform compatibility.
Attachment #8485385 - Flags: review?(joshmoz)
Thank you for reviewing!

Tested on Mac OS X 10.9.4 64bit Opt/Debug builds,
passed test_collation_mac_icu.js (applied to mozilla-central),
and folder pane is shown (applied to comm-central).

(In reply to Josh Aas (Mozilla Corporation) from comment #39)
> @@ +59,5 @@
> > +{
> > +  NS_ENSURE_ARG_POINTER(aICUStrength);
> > +  NS_ENSURE_TRUE((aNSStrength < 4), NS_ERROR_FAILURE);
> > +
> > +  UCollationStrength strength;
> 
> You're declaring this without initializing it, then doing a non-exhaustive
> switch to assign to it. This is dangerous because you might end up returning
> an uninitialized value. Please either initialize when you declare it, and/or
> put a default in the switch statement and assert.

I added initialization with UCOL_DEFAULT,
also, added default case with returning NS_ERROR_FAILURE.

> @@ +109,5 @@
> > +  NS_ENSURE_TRUE(NS_SUCCEEDED(res) && !localeString.IsEmpty(),
> > +                 NS_ERROR_FAILURE);
> > +  NS_LossyConvertUTF16toASCII tmp(localeString);
> > +  tmp.ReplaceChar('-', '_');
> > +  char *locale = (char *)PR_Malloc(tmp.Length() + 1);
> 
> I think you want to use moz_malloc here, and make sure to free with
> moz_free. Please replace any other use of PR_Malloc in this file while
> you're at it.

Replaced all PR_* to moz_*.

However, memory allocated in AllocateRawSortKey is freed outside of this class,
and some of them are freed with PR_FREEIF, is it okay?
For example:
  http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbView.cpp#747

> @@ +172,5 @@
> > +    ucol_setAttribute(mCollatorICU, UCOL_ALTERNATE_HANDLING, UCOL_DEFAULT, &status);
> > +    ucol_setAttribute(mCollatorICU, UCOL_NUMERIC_COLLATION, UCOL_OFF, &status);
> > +    ucol_setAttribute(mCollatorICU, UCOL_NORMALIZATION_MODE, UCOL_ON, &status);
> > +    ucol_setAttribute(mCollatorICU, UCOL_CASE_FIRST, UCOL_DEFAULT, &status);
> > +    NS_ENSURE_TRUE(U_SUCCESS(status), NS_ERROR_FAILURE);
> 
> This is only checking the result of the last ucol_setAttribute call.

ucol_setAttribute does not overwrite status with U_ZERO_ERROR when it is not U_ZERO_ERROR. So, if one of them failed, it could be checked by single U_SUCCESS() placed at the end of them.
But it may be better to check one by one, for debugging purpose :)
Attachment #8485385 - Attachment is obsolete: true
Attachment #8485660 - Flags: review?(joshmoz)
Comment on attachment 8485660 [details] [diff] [review]
Use raw ICU instead of CoreServices API in Unicode collation on Mac OS X.

Review of attachment 8485660 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. As for the allocation in AllocateRawSortKey, since it is freed elsewhere with PR_* functions, just allocate with PR_ as well.
Attachment #8485660 - Flags: review?(joshmoz) → review+
Changed moz_malloc to PR_Malloc in AllocateRawSortKey.

Sorry, one more change, I noticed that test_CoreServices function in test_collation_mac_icu.js fails on 64bit build when this patch is landed to mozilla-aurora (or mozilla-beta, etc), because replace-malloc is enabled only in nightly, and UCCreateCollator will fail for other branches.
So I'd like to run test_CoreServices only on 32bit build, okay?
Attachment #8485660 - Attachment is obsolete: true
Attachment #8487935 - Flags: review?(joshmoz)
Oops, I mis-understood about branch, sorry.
I thought that comm-release branch is used for building Thunderbird 31.

This patch does not work with comm-esr31 branch, because _INTL_API is not enabled in it.

Could the patch in bug 1017883 be backported to comm-esr31 branch?
  https://hg.mozilla.org/mozilla-central/rev/d5b0cce4d2b5
Flags: needinfo?(joshmoz)
(In reply to Tooru Fujisawa [:arai] from comment #42)

> Changed moz_malloc to PR_Malloc in AllocateRawSortKey.

Thanks

> Sorry, one more change, I noticed that test_CoreServices function in
> test_collation_mac_icu.js fails on 64bit build when this patch is landed to
> mozilla-aurora (or mozilla-beta, etc), because replace-malloc is enabled
> only in nightly, and UCCreateCollator will fail for other branches.
> So I'd like to run test_CoreServices only on 32bit build, okay?

I don't really understand this part.
Flags: needinfo?(joshmoz)
(In reply to Tooru Fujisawa [:arai] from comment #43)

> Could the patch in bug 1017883 be backported to comm-esr31 branch?
>   https://hg.mozilla.org/mozilla-central/rev/d5b0cce4d2b5

I don't know, you'll have to ask somebody else.
(In reply to Josh Aas (Mozilla Corporation) from comment #44)
> > Sorry, one more change, I noticed that test_CoreServices function in
> > test_collation_mac_icu.js fails on 64bit build when this patch is landed to
> > mozilla-aurora (or mozilla-beta, etc), because replace-malloc is enabled
> > only in nightly, and UCCreateCollator will fail for other branches.
> > So I'd like to run test_CoreServices only on 32bit build, okay?
> 
> I don't really understand this part.

Sorry for my unclear explanation.

1. "test_CoreServices" function tests existing part of nsCollationMacUC class
2. ICU part of nsCollationMacUC class could be disabled by setting "intl.collation.mac.use_icu" pref to false, and it flips back to existing part
3. Existing part of nsCollationMacUC hits the use-after-free bug on Mavericks
4. The use-after-free bug happens only when jemalloc (and memory poisoning) is enabled, and replace-malloc is disabled
5. jemalloc is enabled only in 64bit build
6. replace-malloc is enabled only in nightly build

For example, "test_CoreServices" function will fail on 64bit aurora build on Mavericks.

So "test_CoreServices" function should be called only when at least one of following conditions met.
 (a) jemalloc (or memory poisoning) is disabled
 (b) OS is not Mavericks
 (c) replace-malloc is enabled

I'd like to choose (a).
So I added following condition because jemalloc is disabled on 32bit build.
  if (Services.sysinfo.getProperty("arch") == "x86") {
    test_CoreServices();
  }

Is this approach okay?


(In reply to Josh Aas (Mozilla Corporation) from comment #45)
> (In reply to Tooru Fujisawa [:arai] from comment #43)
> 
> > Could the patch in bug 1017883 be backported to comm-esr31 branch?
> >   https://hg.mozilla.org/mozilla-central/rev/d5b0cce4d2b5
> 
> I don't know, you'll have to ask somebody else.

Okay, thanks.
glandium, could the patch in bug 1017883 be backported to comm-esr31 branch?
  https://hg.mozilla.org/mozilla-central/rev/d5b0cce4d2b5
I need ICU for fixing this bug.
Flags: needinfo?(mh+mozilla)
(In reply to Tooru Fujisawa [:arai] from comment #46)

> Is this approach okay?

Sure
Attachment #8487935 - Flags: review?(joshmoz) → review+
Standard8 shall we push this monday as well ?
Flags: needinfo?(standard8)
(In reply to Tooru Fujisawa [:arai] from comment #47)
> glandium, could the patch in bug 1017883 be backported to comm-esr31 branch?
>   https://hg.mozilla.org/mozilla-central/rev/d5b0cce4d2b5
> I need ICU for fixing this bug.

Technically, this would be backported to mozilla-esr31, not comm-esr31.
Note you don't /need/ this backported. You could work around it on comm-esr31 side, by adding --enable-intl-api to the subconfigure invocation in configure.in. That being said, it /shouldn't/ hurt mozilla-esr31 if it were backported there.
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #50)
> Technically, this would be backported to mozilla-esr31, not comm-esr31.
> Note you don't /need/ this backported. You could work around it on
> comm-esr31 side, by adding --enable-intl-api to the subconfigure invocation
> in configure.in. That being said, it /shouldn't/ hurt mozilla-esr31 if it
> were backported there.

Ah, thanks! :)

Yes, I was wrong, it's mozilla-esr31 branch.
If there is no other application which uses mozilla-esr31 branch, adding the option in comm-esr31 is enough.
I'll prepare it.
Thank you for reviewing!
Sorry, one more fix.
There is debug-only variable `res` in nsCollationMacUC destructor.
Attachment #8487935 - Attachment is obsolete: true
Attachment #8493616 - Flags: review?(joshmoz)
Attached patch Enable Intl API to use ICU. (obsolete) — Splinter Review
Added --with-intl-api to _SUBDIR_CONFIG_ARGS.

This patch is only for comm-esr31. No other branches needs this.
Attachment #8493620 - Flags: review?(mh+mozilla)
Attachment #8493616 - Flags: review?(joshmoz) → review+
Attachment #8493620 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8493616 [details] [diff] [review]
Use raw ICU instead of CoreServices API in Unicode collation on Mac OS X.

[Security approval request comment]
| How easily could an exploit be constructed based on the patch?

I have no idea how to make an exploit, but I guess it's not so easy.

Someone could notice that there is a problem around nsICollation on 64bit build, from a testcase which runs only on 32bit build, because it tests a hidden pref to switch to older code, which still has same problem on 64bit build.

The information about the actual use-after-free bug inside CoreServices API could not be obtained from this patch, because this patch does not fix it, but adds entirely different code path.

| Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

| Which older supported branches are affected by this flaw?

mozilla-esr31, release(32), beta(33) and aurora(34) are affected.
mozilla-esr24 is not affected.

| If not all supported branches, which bug introduced the flaw?

Bug 860254 introduces memory poisoning, and it hits an use-after-free bug inside CoreServices API (inside UCCreateCollator function) on Mac OS X 10.9.* Mavericks.

| Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Same patch is applicable for mozilla-aurora,
and patches are ready for mozilla-beta, mozilla-release and mozilla-esr31.
(almost all differences are come from folder tree change)

comm-esr31 needs extra patch (attachment 8493620 [details] [diff] [review]) to enable ICU.

| How likely is this patch to cause regressions; how much testing does it need?

It may need some manual testing about user interface.

This patch changes some sorting order for list and tree view (or any other place which uses nsICollation for sorting), because of the difference of default sorting order between CoreServices API and ICU. So items will be sorted differently than un-patched builds, depending on selected locale.
attachment 8485387 [details] shows some example differences between un-patched and patched sorting order.

For other things, tbpl should be enough.
Attachment #8493616 - Flags: sec-approval?
Comment on attachment 8493616 [details] [diff] [review]
Use raw ICU instead of CoreServices API in Unicode collation on Mac OS X.

As a sec-other this doesn't need sec-approval to land but I'll give it. If you want it on the ESR31 and other branches, so it doesn't just ride the trains from trunk, you should nominate branch patches for approval.
Attachment #8493616 - Flags: sec-approval? → sec-approval+
Thanks!
I'll push the patch (attachment 8493616 [details] [diff] [review]) to try server.
Almost green on try run: https://tbpl.mozilla.org/?tree=Try&rev=9034ecc26022

dt(browser_projecteditor_menubar_02.js) failed once and passed twice on opt OS X 10.6 Build. But I guess it's not related to this patch.
Keywords: checkin-needed
checkin-needed for attachment 8493616 [details] [diff] [review], to mozilla-central.

I'll post patches for other branches later.
For what it's worth, Apple tells me the underlying bug here has been fixed in the Yosemite GM developer preview (which just appeared at https://developer.apple.com/devcenter/mac/index.action).  They don't say anything about fixing the bug in other (earlier) versions of OS X.
Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/01bbf35c82bd



(In reply to Ludovic Hirlimann [:Usul] from comment #49)
> Standard8 shall we push this monday as well ?

This needs to ride the trains a bit first (and we need to get the other patch on esr if we think its suitable).
Flags: needinfo?(standard8)
Flags: needinfo?(hiikezoe)
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 35.0
Thanks!
I'll rebase and test my patches and post them soon.
Sorry, I didn't know that the code is also used by B2G.
Can we use ICU on B2G Desktop OS X build?
If not, I'll try adding `#ifndef MOZ_B2G` around ICU related code.
Flags: needinfo?(joshmoz)
Plan A: Enable ICU on B2G Desktop OS X build.
Commit this patch (set _INTL_API=yes for cocoa b2g) before attachment 8495885 [details] [diff] [review].

Try run is running: https://tbpl.mozilla.org/?tree=Try&rev=e1f9578c40e9
Attachment #8499596 - Flags: feedback?(joshmoz)
Flags: needinfo?(joshmoz)
Plan B: Use CoreServices API instead of ICU for B2G Desktop OS X build.
Use this attachment (adds `#ifndef MOZ_B2G` around all new codes) instead of attachment 8495885 [details] [diff] [review].

Try run is running: https://tbpl.mozilla.org/?tree=Try&rev=fc795e24286e


To solve Thunderbird (and other Desktop applications)'s problem as a quick-fix, Plan B is enough, it has less impact than Plan A. We can remove `#ifndef MOZ_B2G` after bug 866301 was fixed and ICU is enabled on all B2G builds.
Attachment #8499597 - Flags: feedback?(joshmoz)
Attachment #8499597 - Flags: feedback?(joshmoz) → feedback-
Comment on attachment 8499596 [details] [diff] [review]
Part1: Enable ICU on B2G Desktop OS X build.

Review of attachment 8499596 [details] [diff] [review]:
-----------------------------------------------------------------

I prefer this approach. Maybe you can simplify further by just saying to use ICU on OS X no matter what the product?
Attachment #8499596 - Flags: feedback?(joshmoz) → feedback+
(In reply to Josh Aas (Mozilla Corporation) from comment #68)
> Comment on attachment 8499596 [details] [diff] [review]
> Plan A: Enable ICU on B2G Desktop OS X build.
> 
> Review of attachment 8499596 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I prefer this approach.

Thanks!

> Maybe you can simplify further by just saying to use
> ICU on OS X no matter what the product?

Sorry, I'm not sure I understand what you mean.
Changing commit message and comment in the configure.in?
Flags: needinfo?(joshmoz)
(In reply to Tooru Fujisawa [:arai] from comment #69)

> Sorry, I'm not sure I understand what you mean.
> Changing commit message and comment in the configure.in?

Don't worry about it
Flags: needinfo?(joshmoz)
Okay, thanks.

I'll ask r? after try run finished without any error.

Also, stopped try run for Plan B to reduce queue length :)
r=josh, don't worry about asking again if your try run is good
Attachment #8499597 - Attachment is obsolete: true
Comment on attachment 8499596 [details] [diff] [review]
Part1: Enable ICU on B2G Desktop OS X build.

https://tbpl.mozilla.org/?tree=Try&rev=e1f9578c40e9
Green on try run :)

Applying r=josh in comment #72.
Attachment #8499596 - Attachment description: Plan A: Enable ICU on B2G Desktop OS X build. → Part1: Enable ICU on B2G Desktop OS X build.
Attachment #8499596 - Flags: review+
Attachment #8495885 - Attachment description: Use raw ICU instead of CoreServices API in Unicode collation on Mac OS X. (fixed conflict) → Part2: Use raw ICU instead of CoreServices API in Unicode collation on Mac OS X. (fixed conflict)
Keywords: checkin-needed
checkin-needed for following patches:
  * attachment 8499596 [details] [diff] [review] Part1: Enable ICU on B2G Desktop OS X build.
  * attachment 8495885 [details] [diff] [review] Part2: Use raw ICU instead of CoreServices API in Unicode collation on Mac OS X.
I cannot find checkin? flag in those attachments :/

Standard8, would you checkin them?
Flags: needinfo?(standard8)
(In reply to Tooru Fujisawa [:arai] from comment #74)
> checkin-needed for following patches:
>   * attachment 8499596 [details] [diff] [review] Part1: Enable ICU on B2G Desktop OS X build.
Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/6fc196832671

>   * attachment 8495885 [details] [diff] [review] Part2: Use raw ICU instead of CoreServices API in Unicode collation on Mac OS X.
Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/bc30d29aaa7d
Status: NEW → ASSIGNED
Keywords: checkin-needed
Thanks!
Flags: needinfo?(standard8)
Ported Part2 to mozilla-esr31 branch.
I'll ask approval after some nightly builds.

I guess Part1 is not needed, and attachment 8493620 [details] [diff] [review] is still enough for mozilla-esr31 branch (because b2g desktop is not listed in mozilla-esr31 try), right?

By the way, Tracking Flags are set to esr31 only. Do I have to post those patches for aurora/beta/release? (they are ready :)
Flags: needinfo?(joshmoz)
(In reply to Tooru Fujisawa [:arai] from comment #78)

> By the way, Tracking Flags are set to esr31 only. Do I have to post those
> patches for aurora/beta/release? (they are ready :)

I would just let this sit on Nightly for a while, its a big change and we want it to get tested as much as possible. This isn't marked sec-critical or sec-high and its a big change. If this gets a higher security rating at some point and we need to respond sooner we can think about it then.
Flags: needinfo?(joshmoz)
(In reply to Josh Aas (Mozilla Corporation) from comment #81)
> (In reply to Tooru Fujisawa [:arai] from comment #78)
> 
> > By the way, Tracking Flags are set to esr31 only. Do I have to post those
> > patches for aurora/beta/release? (they are ready :)
> 
> I would just let this sit on Nightly for a while, its a big change and we
> want it to get tested as much as possible. 

So how long does it estimate?
Is it possible to get this patch in ESR 31.2.0? Even if security risk is not so high, I think this issue is critical for current Thunderbird users on Mac. I am not a Mac user though.
Bug 1080910 blocks this, and I guess patch for mozilla-esr31 also needs the patch in the bug.
Depends on: 1080910
Patch in bug 1080910 (except reverting change in configure.in) needs be uplifted to mozilla-esr31 branch, before landing attachment 8501883 [details] [diff] [review].
In that case, attachment 8493620 [details] [diff] [review] is not needed for comm-esr31 branch.
Ported the patch in bug 1080910 to mozilla-esr31 branch.
I guess it's better to land this patch and attachment 8501883 [details] [diff] [review] together, as Part1 and Part2, so I post here,
If I should post to bug 1080910, please tell me.

I'll ask review and approval after bug 1080910 gets RESOLVED FIXED, and after some nightly builds.
I confirmed that folder pane is always shown on Earlybird 35.0a2 (2014-10-17) on Mac OS X 10.9.5, with adding a gmail account to clean profile (rebooted 20 times, and shown 20 times),
and sorting order of folder tree is slightly changed, as noted in comment #31.

Also, confirmed that the behavior can be reverted by changing intl.collation.mac.use_icu to false.
Folder pane is sometimes shown (rebooted 20 times, and shown 3 times), and sorting order gets reverted to previous one.
Comment on attachment 8506174 [details] [diff] [review]
(mozilla-esr31) Part1: Add USE_ICU variable separated from ENABLE_INTL_API, and enable it on Mac OS X.

Would you review the change in toolkit/library/libxul.mk ?
I guess it corresponds to toolkit/library/moz.build in following commit:
  https://hg.mozilla.org/mozilla-central/rev/e945f3ffeb7e
For other places, almost same as it.
Attachment #8506174 - Flags: review?(mh+mozilla)
Comment on attachment 8506174 [details] [diff] [review]
(mozilla-esr31) Part1: Add USE_ICU variable separated from ENABLE_INTL_API, and enable it on Mac OS X.

Sorry, I didn't realize I had a r? pending in a security bug.
Attachment #8506174 - Flags: review?(mh+mozilla) → review+
Just adding "Part2:" in commit message, as Part1 is inserted.

This patch corresponds to following commit in mozilla-central.
  https://hg.mozilla.org/mozilla-central/rev/bc30d29aaa7d
Differences are only in their directory structure.

Tested locally with Part1 and Part2 pushed onto following commit,
  http://hg.mozilla.org/releases/mozilla-esr31/rev/a0524e2a9b06
Builded firefox correctly, and passed test_collation_mac_icu.js.

Also, builded thundebird correctly with following commit,
and Folder pane is always shown with 64bit build on Mac OS X 10.9.5.
  https://hg.mozilla.org/releases/comm-esr31/rev/9f538ce73c06


Josh, it's already two weeks passed from patches landed to mozilla-central,
and currently no major problem reported (except bug 1080910),
especially in it's stability and sorting order change.
So, I'd like to get those patches in Thunderbird 31.3.0, is it okay?
(r?ing just for asking this, I guess it does not need extra review, since almost no change made from original patch)
Attachment #8501883 - Attachment is obsolete: true
Attachment #8512567 - Flags: review?(joshmoz)
Comment on attachment 8493620 [details] [diff] [review]
Enable Intl API to use ICU.

This patch is no more needed, because ICU will be enabled in mozilla-esr31's side, by attachment 8506174 [details] [diff] [review].
Attachment #8493620 - Attachment is obsolete: true
Comment on attachment 8512567 [details] [diff] [review]
(mozilla-esr31) Part2: Use raw ICU instead of CoreServices API in Unicode collation on Mac OS X.

Review of attachment 8512567 [details] [diff] [review]:
-----------------------------------------------------------------

Fine with me, but I have no knowledge of what's going on in Thunderbird.
Attachment #8512567 - Flags: review?(joshmoz) → review+
Comment on attachment 8506174 [details] [diff] [review]
(mozilla-esr31) Part1: Add USE_ICU variable separated from ENABLE_INTL_API, and enable it on Mac OS X.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  - Main problem caused by this bug happens in Thunderbird 31.x, which uses mozilla-esr31 branch.
User impact if declined:
  - Folder pane is not shown in Thunderbird 31.2.0 64bit build on Mac OS X 10.9.x, especially with ja-JP locale, and users have to switch to 32bit build manually.
Fix Landed on Version:
  - Part1: 36 (mozilla-central)
  - Part2: 35 (mozilla-aurora)
Risk to taking this patch (and alternatives if risky):
  - Sorting order will be slightly changed in tree and list controls, and any other places which uses nsICollation, on Mac OS X.
String or UUID changes made by this patch:
  - None
Attachment #8506174 - Flags: approval-mozilla-esr31?
Comment on attachment 8512567 [details] [diff] [review]
(mozilla-esr31) Part2: Use raw ICU instead of CoreServices API in Unicode collation on Mac OS X.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  - Main problem caused by this bug happens in Thunderbird 31.x, which uses mozilla-esr31 branch.
User impact if declined:
  - Folder pane is not shown in Thunderbird 31.2.0 64bit build on Mac OS X 10.9.x, especially with ja-JP locale, and users have to switch to 32bit build manually.
Fix Landed on Version:
  - Part1: 36 (mozilla-central)
  - Part2: 35 (mozilla-aurora)
Risk to taking this patch (and alternatives if risky):
  - Sorting order will be slightly changed in tree and list controls, and any other places which uses nsICollation, on Mac OS X.
String or UUID changes made by this patch:
  - None


(In reply to Josh Aas (Mozilla Corporation) from comment #92)
> Comment on attachment 8512567 [details] [diff] [review]
> (mozilla-esr31) Part2: Use raw ICU instead of CoreServices API in Unicode
> collation on Mac OS X.
> 
> Review of attachment 8512567 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Fine with me, but I have no knowledge of what's going on in Thunderbird.

Thanks!
Maybe I have to contact to release management people?
(or asking approval is sufficient?)
Attachment #8512567 - Flags: approval-mozilla-esr31?
Tooru, what is the potential impact on Firefox?
The patch is very big for esr. Don't you have a smaller way to fix it?
Flags: needinfo?(arai_a)
(In reply to Sylvestre Ledru [:sylvestre] from comment #95)
> Tooru, what is the potential impact on Firefox?

Same as above.
Sorting order will be slightly changed in tree and list controls, and any other places which uses nsICollation, on Mac OS X.

This is caused by the difference between ICU's default sorting order and CoreServices' default sorting order.
Difference depends on selected locale (because sorting order depends on it).

(should I investigate where it's used, and list up the affected UI's?)


> The patch is very big for esr. Don't you have a smaller way to fix it?

I'm afraid I don't have it.


FWIW, Currently there are some workaround for this.
I guess many Japanese people uses one of them, because this bug mainly happens on Japanese locale.
(reported in MozillaZine forum and Twitter)

1. Use Thunderbird 32bit build
  This is the bug in Mac OS X 10.9 itself, and when we enable memory poisoning, we hit it.
  But memory poisoning is disabled on current 32bit build.

2. Use Thunderbird 24.x
  Memory poisoning is not yet introduced for 24.x.
  But support was already dropped now, right?


Then, I listed some solutions in comment #17, but I think they are not better than this.

1. Use code in intl/locale/unix instead of intl/locale/mac.
  The bug exists in the API used in intl/locale/mac, and intl/locale/unix uses
  another API.
  But I'm not sure it's possible or not, and it won't be so smaller than this.
  Also, sorting order will be changed more than this (because it does not support comparing without accent).

(2. this patch)

3. Pass NULL as locale parameter for UCCreateCollator to avoid the buggy code path in CoreServices
  Change in source code will be small enough (one line)
  But sorting order will be significantly changed.
  (I think it's not a good solution)

4. Disable memory poisoning on Thunderbird 64bit build on Mac OS X (rejected by Steven)
  Change in source code will be small enough (a few line in configure.in),
  and no significant impact on user's side,
  but it will make debugging harder (especially in via Crash Reporter)

5. Wait for the fix on Apple's side (not a fix on our side)
  It's fixed in Mac OS X 10.10 Yosemite (comment #61).
  But currently the fix is not yet backported to Mac OS X 10.9 or older.

In addition to them

6. Show popup message about workaround-1
  When Thunderbird hit the bug, show popup message "Please turn [Open in 32-bit mode] checkbox on", or something like that.
  It's not a fix, yes.

7. Make memory poisoning suspendable, and suspend it while calling broken CoreServices API
  Less impact than 4 for debugging,
  but this would be a bigger change than this.

8. Apply this patch in comm-esr31's side (store this patch in comm-esr31, and apply it dynamically to mozilla subdirectory)
  Have no impact on Firefox's side.
  But I don't know it's possible or not, with existing build-scripts.
Flags: needinfo?(arai_a)
(In reply to Tooru Fujisawa [:arai] from comment #96)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #95)
> > Tooru, what is the potential impact on Firefox?
> 
> Same as above.
[...]
 
> (should I investigate where it's used, and list up the affected UI's?)
Yes, please. I have no clue where this is being used in Firefox...
Okay, give me some time.
Attached image bug1045958-esr31-ui-compare.png (obsolete) —
For now, following components are confirmed to be affected (it uses nsICollation):
* XUL element which uses sorting
    tree uses sorting when clicking column header
* <xsl:sort lang="{LOCALE}">
    not used in mozilla-esr31
* String.prototype.localeCompare (when using XPConnect only?)
    I don't see any effect for now
* SQLite COLLATE
    used only in Places
* nsCharsetMenu (charset menu)
    not used in mozilla-esr31 (comm-esr31 only)
* History (nsNavHistory)
* Bookmark (nsNavBookmarks)


In UI, following window/pane are affected:
* Prefereces - Content - Block pop-up windows - Exceptions (XUL: tree)
* Prefereces - Applications (XUL: tree)
* Prefereces - Privacy - History - Accept cookies from sites - Exceptions (XUL: tree)
* Prefereces - Privacy - History - Show Cookies (XUL: tree)
* Prefereces - Security - Warn me when wites try to install add-ons - Exceptions (XUL: tree)
* Prefereces - Security - Passwords - Remember passwords for sites - Exceptions (XUL: tree)
* Prefereces - Security - Passwords - Saved Passwords (XUL: tree)
* Prefereces - Advanced - Network - Offline Web Content and User Data - Exceptions (XUL: tree)
* Prefereces - Advanced - Certificates - View Certificates (XUL: tree)
* Prefereces - Advanced - Certificates - Security Devices (XUL: tree)
* Sidebar - History (nsNavHistory)
* Library window (XUL: tree)
* Page Info - General - Meta (XUL: tree)
* Page Info - Media (XUL: tree)

Attached screenshot of Library window and History Sidebar, in ja-JP locale.

Following are not affected:
* Sidebar - Bookmarks - Context Menu - Sort By Name
* Local Directory listing (file:///..../)

I'll continue investigating.
Sorry, I was wrong, String.prototype.localeCompare is not affected.
str_localeCompare function is not used because EXPOSE_INTL_API is defined.
(In reply to Tooru Fujisawa [:arai] from comment #100)
> Sorry, I was wrong, String.prototype.localeCompare is not affected.
> str_localeCompare function is not used because EXPOSE_INTL_API is defined.

*on Firefox ESR 31.x (mozilla-esr31).

(EXPOSE_INTL_API is not defined on comme-esr31)
From my understanding, if "intl.collation.mac.use_icu" is false, the sorting behaviors (and also something else) are the same as before. Right?
If so, we should set the preference value false by default, and set true in thunderbird side (comm-central/mailnews/mailnews.js).
(In reply to Hiroyuki Ikezoe (:hiro) from comment #102)
> From my understanding, if "intl.collation.mac.use_icu" is false, the sorting
> behaviors (and also something else) are the same as before. Right?
> If so, we should set the preference value false by default, and set true in
> thunderbird side (comm-central/mailnews/mailnews.js).

That's a good point, thanks a lot :)
Yes, the patch does not affect to sorting behavior when the pref value is false or it doesn't exist,
(also, it does not fix anything in that case)
and we can avoid the effect to Firefox ESR 31.x's side.

I'll prepare the patch and test it soon.
To reduce the impact on Firefox ESR 31.x's side,
disabled the newly added raw-ICU code by default, by setting "intl.collation.mac.use_icu" pref value false.

Differences between attachment 8512567 [details] [diff] [review] are following:
  * intl.collation.mac.use_icu is set to false by default
  * "test_default" function is not executed on 64bit build in test_collation_mac_icu.js
    Because it will fail by hitting the CoreServices' bug (Same reason as why test_CoreServices is not executed)

And set the pref value true in comm-esr31's side.

Build succeeded with mozilla-esr31 and comm-esr31 locally,
and it passed test_collation_mac_icu.js.

I confirmed that sorting order is same as before with mozilla-esr31 (Firefox),
and it's fixed with comm-esr31 (Thunderbird).
Attachment #8514083 - Flags: review?(joshmoz)
Patch for comm-esr31's side.
it sets intl.collation.mac.use_icu pref value true by default.
Attachment #8514089 - Flags: review?(joshmoz)
Attachment #8512567 - Flags: approval-mozilla-esr31?
Attachment #8514089 - Flags: review?(joshmoz) → review+
Attachment #8514091 - Flags: review?(joshmoz) → review+
Attached image comm-esr31-ui-diff.png
Thank you Josh.

Attached screenshots of Thunderbird 24.8.1, 31.2.0, comm-esr31 with this patch, 64/32bit build for each.
* Tested on Mac OS X 10.9.5.
* System locale is Japanese (result will be different with other locale).
* Used GMail IMAP account.
* In the screenshot, folder named single "А" is a CYRILLIC CAPITAL LETTER A, not a LATIN CAPITAL LETTER A.

(1) It's already reported that sorting order is different than before (24.8.1) on 31.2.0 64bit build, even if folder pane is shown (in MozillaZine forum)
This is the another aspect of this bug (comment #23).
(I closed and opened mailer window about 10 times to make folder pane shown)
So, current *correct* sorting order is 31.2.0 32bit build's one.

This is fixed by this patch.

(2) This is the actual impact of this patch, on Thunderbird.
Sorting order is slightly different between 32.1.0 32bit build and patched 64bit/32bit build.
This is caused by the difference between default sorting order of ICU and CoreServices API.


I'll post the list of affected UI's in Thunderbird later (maybe tonight or tomorrow), then ask approval again.
Comment on attachment 8512567 [details] [diff] [review]
(mozilla-esr31) Part2: Use raw ICU instead of CoreServices API in Unicode collation on Mac OS X.

Obsolete previous patch.
Attachment #8512567 - Attachment is obsolete: true
Attachment #8506174 - Flags: approval-mozilla-esr31?
Attachment #8513621 - Attachment is obsolete: true
Comment on attachment 8506174 [details] [diff] [review]
(mozilla-esr31) Part1: Add USE_ICU variable separated from ENABLE_INTL_API, and enable it on Mac OS X.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  - Main problem caused by this bug happens in Thunderbird 31.x, which uses mozilla-esr31 branch.
User impact if declined:
  - Cannot solve following problem on Thunderbird 31.2.0 64bit build on Mac OS X 10.9.x
    * Folder pane is not always shown, especially with ja-JP locale, and users have to switch to 32bit build manually.
    * Even if folder pane is shown on 64bit, the sorting order is different than before (24.x) or 31.2.0 32bit build
Fix Landed on Version:
  - Part1: 36 (mozilla-central)
  - Part2: 35 (mozilla-aurora)
    this feature is enabled on mozilla-aurora, but is disabled in this patch.
Risk to taking this patch (and alternatives if risky):
  - None (unless user modify "intl.collation.mac.use_icu" pref value to true manually. for that case, see comment #99)
String or UUID changes made by this patch:
  - None
Attachment #8506174 - Flags: approval-mozilla-esr31?
Comment on attachment 8514091 [details] [diff] [review]
(mozilla-esr31) Part2: Use raw ICU instead of CoreServices API in Unicode collation on Mac OS X, disabled by default.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  - Main problem caused by this bug happens in Thunderbird 31.x, which uses mozilla-esr31 branch.
User impact if declined:
  - Cannot solve following problem on Thunderbird 31.2.0 64bit build on Mac OS X 10.9.x
    * Folder pane is not always shown, especially with ja-JP locale, and users have to switch to 32bit build manually.
    * Even if folder pane is shown on 64bit, the sorting order is different than before (24.x) or 31.2.0 32bit build
Fix Landed on Version:
  - Part1: 36 (mozilla-central)
  - Part2: 35 (mozilla-aurora)
    this feature is enabled on mozilla-aurora, but is disabled in this patch.
Risk to taking this patch (and alternatives if risky):
  - None (unless user modify "intl.collation.mac.use_icu" pref value to true manually. for that case, see comment #99)
String or UUID changes made by this patch:
  - None
Attachment #8514091 - Flags: approval-mozilla-esr31?
Comment on attachment 8514089 [details] [diff] [review]
(comm-esr31) Use raw ICU instead of CoreServices API in Unicode collation on Mac OS X.

[Approval Request Comment]
Regression caused by (bug #):
  - bug 860254 (it hits the bug in CoreServices API)
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  - Sorting behavior on Thunderbird 31.2.0 64bit build is corrupted (see User impact, and comment #108 for detail).
User impact if declined:
  - Following problem happens on Thunderbird 31.2.0 64bit build on Mac OS X 10.9.x
    * Folder pane is not always shown, especially with ja-JP locale, and users have to switch to 32bit build manually.
    * Even if folder pane is shown on 64bit, the sorting order is different than before (24.x) or 31.2.0 32bit build
Fix Landed on Version:
  - Patch with same effect was landed on 35 (mozilla-aurora)
Testing completed (on c-c, etc.):
  - No regression happened on comm-central/comm-aurora
  - Tested manually with comm-esr31 (restart 20 times, and folder pane is shown everytime)
Risk to taking this patch (and alternatives if risky):
  - Sorting order will be slightly changed in following UI, and any other places (including Add-ons) which uses unicode collation in Thunderbird 31.2.0 on Mac OS X.
    * Mail Window - Folder pane
    * Mail Window - Thread pane
    * Mail Window - Gloda search result
    * Search Messages Window - Search for message in (menu)
    * Search Messages Window - Result
    * Account Setting Window - Server Settings - Server Settings - Move it to this folder (menu)
    * Account Setting Window - Copies & Folders - (all group) - Other (menu)
    * Account Setting Window - Junk Settings - Destination and Retention - Other (menu)
    * Account Setting Window - Synchronization & Storage - Message Synchronizing - Advanced
    * Message Filters Window - Filter list
    * Message Filters Window - New/Edit - Perform these actions - Move Messages - Choose Folder (menu)
    * Message Filters Window - New/Edit - Perform these actions - Copy Messages - Choose Folder (menu)
    * Message Filters Window - Run selected filter(s) on (menu)
    * Address Book Window - listing on top-right tree view
    * Preference Window - Display - Formatting - Font & Colors - Advanced - Character Encoding - Outgoing Mail
    * Preference Window - Display - Formatting - Font & Colors - Advanced - Character Encoding - Incoming Mail
    * Preference Window - Privacy - Mail Content - Allow remote content in messages - Exceptions
    * Preference Window - Privacy - Web Content - Accept cookies from sites - Exceptions
    * Preference Window - Privacy - Web Content - Show Cookies
    * Preference Window - Security - Passwords - Saved Passwords
    * Preference Window - Attachments - Incoming
    * Preference Window - Advanced - General - Config Editor
    * Preference Window - Advanced - Certificates - View Certificates
    * Preference Window - Advanced - Certificates - Security Devices
    * Addon Manager - Addon listing
String or UUID changes made by this patch:
  - None

Sylvestre, if there is something missing, please notify me.
Attachment #8514089 - Flags: approval-comm-esr31?
I guess regressionwindow-wanted is no more needed.
Attachment #8514091 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Attachment #8514091 - Flags: approval-mozilla-esr31+ → approval-mozilla-esr31?
Attachment #8514091 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Kamil,  Once this lands can you verify no impact on Firefox?
Flags: needinfo?(kamiljoz)
Component: Folder and Message Lists → Build Config
Flags: approval-comm-esr31?
Product: Thunderbird → Core
Target Milestone: Thunderbird 35.0 → ---
Version: 31 → 31 Branch
Component: Build Config → Folder and Message Lists
Product: Core → Thunderbird
Target Milestone: --- → Thunderbird 35.0
Version: 31 Branch → 31
Thank you for approval and landing.

It seems that the patch for comm-esr31 was rejected, right? (or postponed?)
Comment on attachment 8514089 [details] [diff] [review]
(comm-esr31) Use raw ICU instead of CoreServices API in Unicode collation on Mac OS X.

The approval request has somehow gone...

Please see comment #112.
Attachment #8514089 - Flags: approval-comm-esr31?
This is waiting for approval from TB's Driver I just pinged on IRC so someone should weigh in soon
FWIW, using crash-stats statistics as an approximation (IF crash-stats and breakpad aren't broken somewhere), less than 0.002% of ja users are Mac
Attachment #8514091 - Flags: approval-mozilla-esr31+ → approval-mozilla-esr31-
Attachment #8506174 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31-
Based on the feedback in Comment 119 I'm withdrawing approval for this. Ryan can you back out the landed patch?
Flags: needinfo?(kamiljoz) → needinfo?(ryanvm)
In the future, it would save needless busy work and code churn if the data could be collected and evaluated prior to approving patches for uplift.

Also, looks like this bug still needs some flag cleanup (approval requests, tracking, etc).

https://hg.mozilla.org/releases/mozilla-esr31/rev/63800a010259
That's a pity....
It's been four months since Thunderbird 31 released. We might lose some of those users. It will be more than half of year until next Thunderbird. We will lost more those users.

:arai, I am sorry I cloud not help you at all.
Attachment #8514089 - Flags: approval-comm-esr31?
(In reply to Wayne Mery (:wsmwk) from comment #119)
> FWIW, using crash-stats statistics as an approximation (IF crash-stats and
> breakpad aren't broken somewhere), less than 0.002% of ja users are Mac

Just to be clear - this isn't real usage data.  I offere it as an approximation because I don't have access to real data by OS by locale.   Also, I wasn't suggesting it isn't an important patch, an unimportant population, nor suggesting that we shouldn't take the patch in an ESR.  My only point is that, if the approximation is correct, missing this ESR and puttting it in the next ESR might not be a huge loss in terms of numbers of users impacted.
Okay, I don't have any objection for rejecting this patch for ESR if the risk is greater than the benefit.
Thank you all for your hard work :)

Then, in that case, I'd like to suggest announcing this problem in release notes as Known Problem,
and providing the documentation about some workarounds (switching to 32-bit mode, switching system locale, upgrading to Yosemite, etc) on mozilla.jp or support.mozilla.org.

Of course there is already MozillaZine.jp topic and some blog posts which have almost same information,
but having a document on official website would be nice.

Also, it should be better to suggest switching back to 64-bit mode when upgrading to Thunderbird 38.
(some features are disabled on 32-bit build, especially jemalloc)

Who is responsible for those release notes/documentation thing?
Since this is still a security-related problem, it should be better to discuss before creating a documentation on SUMO.
Mark Banner:

Could you add this issue and workaround to the release note of Thunderbird 31.x? If so, Japanese localizers will translate it.
Flags: needinfo?(standard8)
Keywords: relnote
further clarification - by "next ESR" I don't mean 38. I mean an available point release like (for thunderbird) 31.3.0, 31.4.0, etc.  That is, if the risk/reward is acceptable - which others must judge.  But hiro is correct that this is critical for some Thunderbird users on Mac becuase thunderbird is unusable for affected population.  And I think no one has yet agreed or disagreed that the risk is low.

Does anyone in Mozilla Japan know the usage stats for this population of users for Firefox, or Thunderbird?
We have no such stats in Japan. I've heard rkent has some stats about Thunderbird users at Thunderbird summit.

My wild guess is that Thunderbird users is greater than Firefox desktop users in Japan.

Note that about Mac users in Japan, they are mostly some kind of geeks, so once they post negative message on social platforms, it will be very influential. I am worrying about that.
Flags: needinfo?(standard8) → needinfo?(kent)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #127)
> We have no such stats in Japan. I've heard rkent has some stats about
> Thunderbird users at Thunderbird summit.
> 
> My wild guess is that Thunderbird users is greater than Firefox desktop
> users in Japan.
> 
> Note that about Mac users in Japan, they are mostly some kind of geeks, so
> once they post negative message on social platforms, it will be very
> influential. I am worrying about that.

The data that was given to me shows usage per day per country code, but is not divided per operating system. So I don't think it is useful here.
Flags: needinfo?(kent)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #122)
> That's a pity....
> It's been four months since Thunderbird 31 released. We might lose some of
> those users. It will be more than half of year until next Thunderbird. We
> will lost more those users.
Yes, we agree that it is a pity but we cannot take the chance to break Firefox ESR when our commitment to users is about stability and security fixes.
Moreover, we have low user test coverage on the ESR channel.

Since it seems to really matter to you, I suggest you work around it in Thunderbird build system (like applying the patch on top of Firefox sources at build time).
(In reply to Sylvestre Ledru [:sylvestre] from comment #129)
> Yes, we agree that it is a pity but we cannot take the chance to break
> Firefox ESR when our commitment to users is about stability and security
> fixes.

Eh, this bug *is* a use-after-freed security bug. So I too do find the decision not to take it after all strange.
And FWIW, the number of Japanese Mac users is bound to be more than that... + as I understand it the fix only really affects Japanese users anyway.
(In reply to Magnus Melin from comment #130)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #129)
> > Yes, we agree that it is a pity but we cannot take the chance to break
> > Firefox ESR when our commitment to users is about stability and security
> > fixes.
> 
> Eh, this bug *is* a use-after-freed security bug. So I too do find the
> decision not to take it after all strange.
This is a sec-other. Moreover, afaik, it does not impact Firefox itself.

> And FWIW, the number of Japanese Mac users is bound to be more than that...
> + as I understand it the fix only really affects Japanese users anyway.
Yes but the patch could have side effect on Firefox itself.
(In reply to Sylvestre Ledru [:sylvestre] from comment #129)
> Since it seems to really matter to you, I suggest you work around it in
> Thunderbird build system (like applying the patch on top of Firefox sources
> at build time).

Thanks for the suggestion. Here is another workaround (not build system change) without any modifications against mozilla-central.

:arai, I do not understand this problem exactly but I think this patch will solve disappearing folders in folder pane. What do you think?
Attachment #8522783 - Flags: feedback?(arai_a)
We don't have try-comm-esr, so I pushed the patch to try-comm-central.
https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-central&revision=4373a118420f
Oops, there are still other compareSortKeys in mailnews/ and mail/. We should handle those.
Comment on attachment 8522783 [details] [diff] [review]
Another variant of workaround for comm-central

Review of attachment 8522783 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, yeah, folder pane will be shown with it,
however, with this way, we need more try-catches in several places,
I listed affected UIs in comment #112, it means that all of them uses nsICollation's implementation.
(I'm not sure all of them could be addressed from comm-esr31's side)

For example, when Thunderbird hit the bug,
gloda search also does not work, with same error.
> ERROR: 2014-11-14 17:55:06  gloda.ds.qfq  ERROR  Exception: [Exception... "Failure"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource:///modules/gloda/gloda.js :: gloda_folder_comparator :: line 1214"  data: no]
gloda_folder_comparator function calls String.prototype.localeCompare,
and it uses nsICollation internally.


Also, there could be some Add-ons which uses nsICollation (explicitly or implicitly),
in that case, it might be better to notify users that something goes wrong in early step (as a visibility of folder pane),
instead of hiding the bug from users.
Attachment #8522783 - Flags: feedback?(arai_a) → feedback-
Thanks for the explanation. This problem is more complicated than I imagined. I did misunderstand firefox is not affected by this problem... My suggestion in comment #102 was a mistake.
Actually, it's possible to hit this bug on Firefox ESR 31,
by just evaluating following code in browser console (needs chrome debugging option).

  for (var i = 0; i < 100; i++) { // since this bug happens sometimes...
    var localeService = Components.classes["@mozilla.org/intl/nslocaleservice;1"].
      getService(Components.interfaces.nsILocaleService);
    var localeCollator = Components.classes["@mozilla.org/intl/collation-factory;1"].
      getService(Components.interfaces.nsICollationFactory).
      CreateCollation(localeService.newLocale("ja-JP"));
    for (var strength of [0, 1, 2, 3]) {
      localeCollator.compareString(strength, "A", "B");
    }
  }

But, somehow, currently we don't find any actual problem related to this on Firefox ESR 31 itself.
Looking at the Thunderbird usage data that I have per country, Japan user count is about equal to US user count, and will probably be our #2 country (after Germany) soon.

For January - June 2014, average daily user count (using blocklist data) for Japan was 760,000 users. Since 1) most real users don't use Thunderbird every day, and 2) some users are behind firewalls that are missed in counts, typical practice is to up this number by 200% to 300% to give an estimate of actual users. So using a 250% multiplier, I estimate current Japan Thunderbird users to be about 1,900,000.  Since overall Mac users are about 5% of Thunderbird users, this give an estimated count of Japan Thunderbird Mac users of 95,000.

The denial of this patch was made on the basis of what is clearly an erroneous number, "less than 0.002% of ja users are Mac". That would imply there are only 38 Mac users of Thunderbird in all of Japan. If that were true, then the denial of this patch was correct. But there is no reasonable way that Mac Thunderbird usage in Japan is 4 orders of magnitude less than typical, so the lesson from that is that we need to understand what is wrong with the crash data.

Please let's not make the mistake of saying that once a decision is made, regardless of what misunderstanding was the basis of that decision, that the decision is now fixed for all time. The facts were wrong, the decison was made on incorrect facts, so let's go back in time to where we were at comment 118 where this patch was on the road to approval.

Reopening on that basis.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Tooru Fujisawa [:arai] from comment #10)
> The problem is sometimes reproduced for me too, on following env:
>   * Mac OS X 10.9.4
>   * Thunderbird 31 ja-JP-mac
>     Thunderbird 31 en-US
>     Thunderbird 32 Beta ja-JP-mac
>   * clean profile + single IMAP (icloud) account
> Not yet happened in Daily 34.a1.


(In reply to sh from comment #7)
> I have created a new account on my iMac (Mavericks 10.9.4).
> 
> When I start Thunderbird not in 32bit mode in the account, the folder pane
> does not show up.
> 
> When I set English as preferred language in System Preferences on Mavericks,
> and I start Thunderbird not in 32bit mode,
> the folder pane shows up. 

To really nail this down, the issue shows only on Mac OS X 19.9 (no other versions of Mac), where ja is the language set for Mac, for ALL locales/language versions of Thunderbird (i.e. not limited to ja-JP Thunderbird). 

Correct?
Flags: needinfo?(arai_a)
(In reply to Wayne Mery (:wsmwk) from comment #139)
> To really nail this down, the issue shows only on Mac OS X 19.9 (no other
> versions of Mac), where ja is the language set for Mac, for ALL
> locales/language versions of Thunderbird (i.e. not limited to ja-JP
> Thunderbird). 
> 
> Correct?

The issue seems to exist on 10.6 to 10.9, so all supported versions.
Not sure for 10.5 or lower, but they are not supported.

I confirmed this issue on 10.9.4 and 10.9.5 (sorry I don't have another supported versions...).
Also, in MozillaZine.jp forum, most of reports are based on 10.9.4 or 10.9.5,
but there are some reports with other versions:
  10.6.8: 3 users
  10.6.?: 1 user
  10.7.5: 3 users
  10.8.5: 1 user


For other things, I think you are correct.
Here are my tests and results on 10.9.5.

Steps:
  1. startup Thunderbird 31.2.0 64bit build with clean profile
  2. setup GMail account
  3. open Inbox, and make sure all folders are shown in folder pane
  4. shutdown Thunderbird
  5. repeat following 20 times
    5-1. startup Thunderbird again
    5-2. check folder pane is shown or not
    5-3. shutdown Thunderbird

Results:
  Mac's system locale | Thunderbird's locale | Result (shown/total)
  --------------------+----------------------+-----------------------
  English             | ja-JP-mac            | 20/20
  English             | en-US                | 20/20
  English             | de                   | 20/20
  Deutsch             | ja-JP-mac            | 20/20
  Deutsch             | en-US                | 20/20
  Deutsch             | de                   | 20/20
  Japanese            | ja-JP-mac            | 4/20
  Japanese            | en-US                | 1/20
  Japanese            | de                   | 12/20
Flags: needinfo?(arai_a)
Aral, thanks for comment 140. So the user population of all using Mac users with system locale set to Japanese (not just a subset of Mac users) will be signficant, a subset of comment 138. 

So status-thunderbird_esr31=wontfix seems premature.  The question are, do we have a patch we can take on comm-central (aral), and can it be done (standard8)?
Flags: needinfo?(standard8)
Flags: needinfo?(arai_a)
Comment on attachment 8522783 [details] [diff] [review]
Another variant of workaround for comm-central

>+    try {
>+      order = a._folder.compareSortKeys(b._folder);
>+    } catch(e) {
>+      oreder = 0;

misspelled
(In reply to Wayne Mery (:wsmwk) from comment #141)
> So status-thunderbird_esr31=wontfix seems premature.  The question are, do
> we have a patch we can take on comm-central (aral), and can it be done
> (standard8)?

Sorry If I misunderstood something.

If you mean comm-esr31, attached patches for mozilla-esr31 and comm-esr31 are still applicable.

If it's required to apply only to comm-esr31 (so, applying patches for mozilla directory on build time automatically, as sylvestre wrote in comment #129),
I have a draft patch (now testing locally)
It would work well with simple building/updating workflow.
But if someone is working in comm-esr31 directory to create another patch under hg or git control,
applying patches will interacts badly with it.
Flags: needinfo?(arai_a)
In the case we need to apply patches only to comm-esr31.

Added actions to apply/reverse patches in client.py,
and apply/reverse them on checkout automatically.

This patch contains patches for mozilla-esr31 (Part1/Part2),
not the patch for comm-esr31 to enable it (attachment 8514089 [details] [diff] [review]).

Tested locally, by running "hg clone ...", preparing mozconfig, running "python client.py checkout", and "./mozilla/mach build",
then build done successfully and folder pane is always shown (20/20).
The patch is very impressive! I've never thought using client.py for applying patch against mozilla-central.
As you all know, most of exploit codes target conditions which is not checked properly. In case of memory poisoning the '0x5a' can be passed NULL checking so exploit can target the condition.
Fortunately '0x5a' could be used as exploit on limited conditions. It highly depends on the implementation after the NULL checking.
But, in this issue case, the implementation is out of our controls. It's inside a library distributed by OS. For this reason, I'd suggest we should also apply arai's patches in mozilla-esr31.

This is an opinion from an exploit code author. I had experiences in writing a couple of exploit code on Android (One of the codes was used to gain root privilege on the first FirefoxOS device in the world).
Comment on attachment 8506174 [details] [diff] [review]
(mozilla-esr31) Part1: Add USE_ICU variable separated from ENABLE_INTL_API, and enable it on Mac OS X.

Re-requesting esr31 approval, per previous comments.
Attachment #8506174 - Flags: approval-mozilla-esr31- → approval-mozilla-esr31?
Attachment #8514091 - Flags: approval-mozilla-esr31- → approval-mozilla-esr31?
(For trunk this is resolved, so marking fixed again.)
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
(In reply to Tooru Fujisawa [:arai] from comment #144)
> Added actions to apply/reverse patches in client.py,
> and apply/reverse them on checkout automatically.

It looks like it might work, however I've a couple of concerns:

- It would need to not fail if the patches weren't applied (e.g. fresh checkout, clobber build).
- iirc, the builders update the mozilla directory themselves (due to the way the build process works), so it would have to be verified that leaving a patch applied doesn't affect that update in an adverse way.

You can look through the logs for what's happening. The logs for releases are stored alongside the candidate builds in the nightly or candidates directory on ftp.

- We'd need at least releng sign-off for if the general process is reasonable or not.
Flags: needinfo?(standard8)
Sorry, I overlooked "do_apply_patches" function in client.py,
I guess it's for another purpose (it seems to be for local patches, instead of in-tree patches),
but "apply_patches" function in previous patch is confusing.
I renamed them to do_apply_intree_patches/do_reverse_intree_patches,
also, changed os.system to check_call_noisy/subprocess.call.


(In reply to Mark Banner (:standard8) from comment #149)
> (In reply to Tooru Fujisawa [:arai] from comment #144)
> > Added actions to apply/reverse patches in client.py,
> > and apply/reverse them on checkout automatically.
> 
> It looks like it might work, however I've a couple of concerns:
> 
> - It would need to not fail if the patches weren't applied (e.g. fresh
> checkout, clobber build).

It checks that patches are already applied/reversed before applying/reversing,
so it works well with fresh checkout.

If clobber build means "./mozilla/mach clobber && ./mozilla/mach build",
it also works because patches are applied in client.py, not in mach.
My explanation in comment #143 is somewhat incorrect,
it applies patched on checkout time (fresh checkout or update), not build time.

> - iirc, the builders update the mozilla directory themselves (due to the way
> the build process works), so it would have to be verified that leaving a
> patch applied doesn't affect that update in an adverse way.
> 
> You can look through the logs for what's happening. The logs for releases
> are stored alongside the candidate builds in the nightly or candidates
> directory on ftp.

In that case, build script also needs same fix,
however, it seems that client.py is executed without "--skip-mozilla" option,
so I think attachment 8524083 [details] [diff] [review] also works there.
  https://ftp.mozilla.org/pub/mozilla.org/thunderbird/candidates/31.2.0-candidates/build2/logs/release-comm-esr31-macosx64_build-bm85-build1-build1.txt.gz

let me know if I misunderstand something.
Attachment #8524083 - Attachment is obsolete: true
Thanks hiro and Magnus for explanation and re-requesting.
If those patches can be landed to mozilla-esr31, the fix on comm-esr31's side will get simpler :)

Then, in attachment 8514091 [details] [diff] [review], the fix is disabled on mozilla-esr31's side (see comment #104),
If we need to apply the fix also to mozilla-esr31, attachment 8512567 [details] [diff] [review] in comment #94 should be used instead for Part2.
(and no patch is needed for comm-esr31)

Magnus, which version do we need?
Flags: needinfo?(mkmelin+mozilla)
Ideally it should land on mozilla-esr31. The "apply patch at checkout" version would be a very special case, and there are always things that can go wrong then.
Flags: needinfo?(mkmelin+mozilla)
Comment on attachment 8512567 [details] [diff] [review]
(mozilla-esr31) Part2: Use raw ICU instead of CoreServices API in Unicode collation on Mac OS X.

Use previous "enabled by default" version instead
Attachment #8512567 - Attachment is obsolete: false
Attachment #8512567 - Flags: approval-mozilla-esr31?
Attachment #8514091 - Flags: approval-mozilla-esr31?
> approval-mozilla-esr31?

It may be noteworthy that there are still no regressions reported thus far against the fixes landed 2014-10-08
Blocks: 860254
Flags: needinfo?(bkerensa)
Summary: TB 31 jp does not display folder pane with OS X 10.9 Mavericks → TB 31 jp does not display folder pane with OS X [Mac]
Whiteboard: [regression:TB31]
(In reply to Wayne Mery (:wsmwk) from comment #154)
> > approval-mozilla-esr31?
> 
> It may be noteworthy that there are still no regressions reported thus far
> against the fixes landed 2014-10-08

Will not be able to take this sorry :(
Flags: needinfo?(bkerensa)
Attachment #8512567 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31-
Attachment #8506174 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31-
The approval request against mozilla-esr31 has now declined. I suppose sec-team checked there is no security risk in the library in MacOSX with memory poisoning. (I hope disassembling MacOSX libraries is not illegal..)

So we have to deal with this issue in comm-central.
Mark, could you please give us some feedbacks about attachment 8524649 [details] [diff] [review]?
Flags: needinfo?(standard8)
I think we should land something like attachment 8522783 [details] [diff] [review] on esr, so at least you get a mainly working product. Further steps can always be taken later if we deem them necessary.
There might be another solution,
since nsICollation is XPCOM, so perhaps we can alter it while Thunderbird's startup process
by re-registering fixed versions of nsICollation and nsICollationFactory with same contract IDs.

Things needed for this will be following:
  * copy some files under intl/locale and intl/locale/mac to comm-esr31
    (since I'm planning to alter it dynamically, I guess windows/unix version is not required)
  * apply patch for it
  * write code for re-registering it dynamically

I'll try it.
(not sure it's really possible though. mainly for timing problem)

your comments are welcome :)
one more possible? solution.
modifying build system to link against patched version of nsCollationMacUC.o,
which is compiled from nsCollationMacUC.cpp stored in comm-esr31
(In reply to Tooru Fujisawa [:arai] from comment #158)
> There might be another solution,
> since nsICollation is XPCOM, so perhaps we can alter it while Thunderbird's
> startup process
> by re-registering fixed versions of nsICollation and nsICollationFactory
> with same contract IDs.

I like this approach very much!
The registration of the modified nsICollationFactory is really necessary?
Flags: needinfo?(standard8)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #160)
> (In reply to Tooru Fujisawa [:arai] from comment #158)
> > There might be another solution,
> > since nsICollation is XPCOM, so perhaps we can alter it while Thunderbird's
> > startup process
> > by re-registering fixed versions of nsICollation and nsICollationFactory
> > with same contract IDs.
> 
> I like this approach very much!

thanks :)

> The registration of the modified nsICollationFactory is really necessary?

I thought I have to call nsIComponentRegistrar.registerFactory to replace component,
so I need fixed version of nsCollation impl and the factory for it.
Is there any other way?
(In reply to Tooru Fujisawa [:arai] from comment #161)

> > The registration of the modified nsICollationFactory is really necessary?
> 
> I thought I have to call nsIComponentRegistrar.registerFactory to replace
> component,
> so I need fixed version of nsCollation impl and the factory for it.
> Is there any other way?

I supposed nsIComponentRegistrar.registerFactory can be used for registration of the modified nsICollation.
Ah, I was misunderstanding about nsICollationFactory.
nsICollationFactory is not a factory for nsICollation.

So, yes, what I need to register is only nsICollation.
Maybe we need to re-register nsICollationFactory too,
because nsCollationFactory creates nsCollation by calling CallCreateInstance with CID, not ContractID.
>  res = CallCreateInstance(kCollationCID, &inst);
and there is no way to unregister original factory (which is needed to register fixed factory with same CID).
There is also one more concern.
We have to enable building ICU, but adding "--with-intl-api" option (attachment 8493620 [details] [diff] [review]) won't work when "--without-intl-api" is specified (bug 1080910).
(to fix it, we need to modify configure scripts in mozilla-esr31)

Could that situation (specifiying "--without-intl-api") happen in comm-esr31?
(In reply to Tooru Fujisawa [:arai] from comment #164)
> Maybe we need to re-register nsICollationFactory too,
> because nsCollationFactory creates nsCollation by calling CallCreateInstance
> with CID, not ContractID.

Ah, I am sorry I did not read the implementation of nsICollationFactory.
You were right. We need to implement nsICollationFactory.

> >  res = CallCreateInstance(kCollationCID, &inst);
> and there is no way to unregister original factory (which is needed to
> register fixed factory with same CID).

http://mxr.mozilla.org/comm-central/source/mailnews/test/resources/MockFactory.js
I think this mock implementation is helpful to you.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #166)
> > >  res = CallCreateInstance(kCollationCID, &inst);
> > and there is no way to unregister original factory (which is needed to
> > register fixed factory with same CID).
> 
> http://mxr.mozilla.org/comm-central/source/mailnews/test/resources/
> MockFactory.js
> I think this mock implementation is helpful to you.

Thank you for the nice example :D

Then, sorry maybe my previous comment was not clear.
There is no way to get original factory object, which is needed to call unregisterFactory (as a second argument).
(except I call nsComponentManagerImpl::FindFactory method directly. however, which is not provided in XPCOM interface)


BTW, the draft patch is almost working locally :)
still needs some brush up though.

Also, one more good(?) news.
if we add "--with-intl-api", String.prototype.localeCompare directly uses ICU instead of nsICollation.
so the problem will become simpler.
oops, sorry, I was wrong.
it's shown in the example
> let originalFactory = Cm.getClassObject(Cc[contractID], Ci.nsIFactory);

I'll try it.
Thanks again for the example!
Now I can re-register fixed version of nsICollation implementation with same CID, and no need for re-registering nsICollationFactory.

Draft patch series are consisted of following 3 parts.

Part1:
  1. Clone nsCollationMacUC implementation to comm-esr31,
     with same name for simplicity,
     adding extra namespace to avoid name conflict with original one

Part2:
  1. Add nsIFactory implementation for nsCollationMacUC
  2. Unregister original nsCollationMacUC and register fixed nsCollationMacUC

Part3 (almost same as the patch Part2 for mozilla-esr31):
  1. Add --with-intl-api configure option
  2. Apply ICU patch to nsCollationMacUC
  3. Set intl.collation.mac.use_icu to true


Then, Part2-2 should be executed in following timing:
  A. After the registration of original nsCollationMacUC's registration
     (otherwise the registration of original one will fail, since it uses same CID)
  B. Before the first usage of nsICollation.
     (otherwise original implementation is used for the call)

Fixed version is registered in nsMailModule's constructor,
and original one is registered when registering nsI18nModule.

nsMailModule and nsI18nModule are static modules, so they're registered in
nsComponentManagerImpl::Init method.
  http://mxr.mozilla.org/comm-esr31/source/mozilla/xpcom/components/nsComponentManager.cpp#351
>    for (uint32_t i = 0; i < sStaticModules->Length(); ++i)
>        RegisterModule((*sStaticModules)[i], nullptr);

On the other hand,
Module's constructor is called on the first usage of the component in the module,
which should be happened after those registrations.
So, condition A is met.


Then, nsComponentManagerImpl::Init is called by NS_InitXPCOM2
  http://mxr.mozilla.org/comm-esr31/source/mozilla/xpcom/build/nsXPComInit.cpp#637
>    rv = nsComponentManagerImpl::gComponentManager->Init();

and the first usage of the component in nsMailModule also happens in NS_InitXPCOM2
  http://mxr.mozilla.org/comm-esr31/source/mozilla/xpcom/build/nsXPComInit.cpp#655
>    nsDirectoryService::gService->RegisterCategoryProviders();
which uses "@mozilla.org/mail/dir-provider;1".

So, unless nsICollation is used in nsComponentManagerImpl::Init or XPTInterfaceInfoManager::GetSingleton,
and currently I don't see any usage there.
So, condition B is also met.


I did same test as comment #140, and folder pane is shown 20/20 times with patched version.
Attachment #8532794 - Flags: review?(hiikezoe)
this patch contains almost same code as attachment 8493620 [details] [diff] [review] and attachment 8512567 [details] [diff] [review],
except it applies the fix to the cloned code in comm-esr31.
Quick work!

Though I can review only part2 patch,  I was thinking we should implement our own nsICollationFactory in JS module after comment #166.

I was thinking that:

1. Create a JS module to register our fixed nsICollationFactory on profile-after-change.
 cf.http://mxr.mozilla.org/comm-central/source/mailnews/extensions/offline-startup/js/
2. Implement the fixed nsICollationFactory in the JS module.
3. The fixed nsICollationFactory creates ICU version nsCollationMacUC only for specific locales (ja or some others I don't know). So the change of sort order will not affect on most locales. (I actually think the change will not happen on ja locale in most cases though.)
 
With this approach we do not need to register our nsCollationMacUC as the same CID and same contract ID of the original one, and not need to modify nsMailModule.cpp. So we don't need to worrying about the timing of the registration of the original one.

arai, could you please try this approach? I am not 100% sure this approach cloud work as expected though.
Attached patch Rough js moduleSplinter Review
arai, this is for you.
I hope this module will work except locale comparison.
Thank you for your feedback and the js module code :)

Then, sorry I should've noted that the bug does not depend on the CreateCollation's parameter,
but Mac OS X's System Locale.
Passing "en-US" also fails when System Locale is Japanese,
and passing "ja-JP" does not fail when System Locale is English.
(not sure what happens internally)

System locale could be retrieved by nsILocaleService.getSystemLocale,
and it should not be changed at least until logout,
so locale check could be made simpler, and I guess we don't even need to replace
nsICollationFactory on non-Japanese locale.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #172)
> Quick work!
> 
> Though I can review only part2 patch,  I was thinking we should implement
> our own nsICollationFactory in JS module after comment #166.

Okay, I see.
but if we don't replace nsICollation itself, following code will call original one
(of course, this case is not found in comm-esr31. so this should be a minor problem...):

---
var localeService = Components.classes["@mozilla.org/intl/nslocaleservice;1"].getService(Components.interfaces.nsILocaleService);
var localeCollator = Components.classes["@mozilla.org/intl/collation;1"].createInstance(Components.interfaces.nsICollation);
localeCollator.initialize(localeService.newLocale("ja-JP"));
---

> I was thinking that:
> 
> 1. Create a JS module to register our fixed nsICollationFactory on
> profile-after-change.
>  cf.http://mxr.mozilla.org/comm-central/source/mailnews/extensions/offline-
> startup/js/
> 2. Implement the fixed nsICollationFactory in the JS module.
> 3. The fixed nsICollationFactory creates ICU version nsCollationMacUC only
> for specific locales (ja or some others I don't know). So the change of sort
> order will not affect on most locales. (I actually think the change will not
> happen on ja locale in most cases though.)
> 
> With this approach we do not need to register our nsCollationMacUC as the
> same CID and same contract ID of the original one, and not need to modify
> nsMailModule.cpp.

sorry if I'm wrong, you mean the module constructor part, right?
I guess I should add the CID/ContractID/Constructor entries for fixed nsCollationMacUC there.
If there is another way to register a binary component, please tell me :)

> So we don't need to worrying about the timing of the
> registration of the original one.

In that case, my concern is condition B (Before the first usage of nsICollation).
I think many code can be executed before (or in) profile-after-change event,
including add-on's code, and some of them might call original nsICollation implementation.
Is there any reason for waiting for it?
If not, I'd like to register on app-startup event to reduce the risk.


Then, I found another problem with registration with JS component,
JS module is not loaded in xpcshell (it's loaded only in thunderbird),
so I'll try to port it to binary component.
(In reply to Tooru Fujisawa [:arai] from comment #174)
> Thank you for your feedback and the js module code :)
> 
> Then, sorry I should've noted that the bug does not depend on the
> CreateCollation's parameter,
> but Mac OS X's System Locale.
> Passing "en-US" also fails when System Locale is Japanese,
> and passing "ja-JP" does not fail when System Locale is English.
> (not sure what happens internally)

Oh, I did misunderstand this issue again... I am so sorry.

> System locale could be retrieved by nsILocaleService.getSystemLocale,
> and it should not be changed at least until logout,
> so locale check could be made simpler, and I guess we don't even need to
> replace
> nsICollationFactory on non-Japanese locale.

This sounds a good idea to me. Let' take this approach in either case JS or C++ implementation.

> (In reply to Hiroyuki Ikezoe (:hiro) from comment #172)
> > Though I can review only part2 patch,  I was thinking we should implement
> > our own nsICollationFactory in JS module after comment #166.
> 
> Okay, I see.
> but if we don't replace nsICollation itself, following code will call
> original one
> (of course, this case is not found in comm-esr31. so this should be a minor
> problem...):

Yes, I've also thought it will not actually be problem. 

> > 1. Create a JS module to register our fixed nsICollationFactory on
> > profile-after-change.
> >  cf.http://mxr.mozilla.org/comm-central/source/mailnews/extensions/offline-
> > startup/js/
> > 2. Implement the fixed nsICollationFactory in the JS module.
> > 3. The fixed nsICollationFactory creates ICU version nsCollationMacUC only
> > for specific locales (ja or some others I don't know). So the change of sort
> > order will not affect on most locales. (I actually think the change will not
> > happen on ja locale in most cases though.)
> > 
> > With this approach we do not need to register our nsCollationMacUC as the
> > same CID and same contract ID of the original one, and not need to modify
> > nsMailModule.cpp.
> 
> sorry if I'm wrong, you mean the module constructor part, right?
> I guess I should add the CID/ContractID/Constructor entries for fixed
> nsCollationMacUC there.
> If there is another way to register a binary component, please tell me :)

I think NSMODULE_DEFN in nsCollationMacUC.cpp can register the nsCollationMacUC.

> In that case, my concern is condition B (Before the first usage of
> nsICollation).
> I think many code can be executed before (or in) profile-after-change event,
> including add-on's code, and some of them might call original nsICollation
> implementation.
> Is there any reason for waiting for it?
> If not, I'd like to register on app-startup event to reduce the risk.

Because I did not know that nsIComponentRegistrar.registerFactory can be used on app-startup. ;-)

> Then, I found another problem with registration with JS component,
> JS module is not loaded in xpcshell (it's loaded only in thunderbird),
> so I'll try to port it to binary component.

I forgot the xpcshell test. The only way I know is running following tricky codes before test runs.

  let collationMacICUStartup =
    Cc["@mozilla.org/messenger/collation-mac-icu-startup;1"]
      .getService(Ci.nsIObserver);
  collationMacICUStartup.observe(null, "app-startup", "");

But as far as I know, there is no way to run these codes before all xpcshell tests in mailnews/ run. So we have no other way than the binary component.
(In reply to Tooru Fujisawa [:arai] from comment #170)
> Created attachment 8532795 [details] [diff] [review]
> (comm-esr31: clone) Part2: Register cloned nsCollationMacUC.

arai, could you modify this patch to use NSMODULE_DEFN in nsCollationRegisterer.cpp and register nsFixedCollationMacUCFactory on app-startup? The name nsCollationRegisterer should be nsCollationRegistrar, IMO.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #176)
> (In reply to Tooru Fujisawa [:arai] from comment #170)
> > Created attachment 8532795 [details] [diff] [review]
> > (comm-esr31: clone) Part2: Register cloned nsCollationMacUC.
> 
> arai, could you modify this patch to use NSMODULE_DEFN in
> nsCollationRegisterer.cpp and register nsFixedCollationMacUCFactory on
> app-startup? The name nsCollationRegisterer should be nsCollationRegistrar,
> IMO.

it uses same CID, do I need to use different CID, or leave it same?
(if we can use same CID, the risk of invoking original collation will gets lower :)

Also, I'm going to try using xpcom-startup (not yet tested though).
removed unused include path.
Attachment #8532794 - Attachment is obsolete: true
Attachment #8532794 - Flags: review?(hiikezoe)
Okay, fixed :)
Now it uses same CID/ContractID.

Part1: same as before (except removing unused include path)

Part2:
  1. Add nsIFactory implementation for nsCollationMacUC
     (nsFixedCollationMacUCFactory)
  2. Add component for registering fixed factory
     (nsCollationRegistrar)
  3. Add static module for nsCollationRegistrar,
     and register nsCollationRegistrar for xpcom-startup event
  4. In xpcom-startup event, if system locale starts with "ja",
     Unregister original nsCollationMacUC and register fixed one

Part3: same as before


Confirmed that fixed version is registerd both in xpcshell and thunderbird.
Attachment #8532795 - Attachment is obsolete: true
Attachment #8532795 - Flags: review?(hiikezoe)
Attachment #8532924 - Flags: review?(hiikezoe)
Comment on attachment 8532924 [details] [diff] [review]
(comm-esr31: clone) Part2: Register cloned nsCollationMacUC.

Review of attachment 8532924 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/intl/mac/nsCollationRegistrar.cpp
@@ +61,5 @@
> +  rv = registrar->RegisterFactory(NS_COLLATION_CID, "Fixed nsCollationMacUC",
> +                                  NS_COLLATION_CONTRACTID, fixedFactory);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  return NS_OK;

This should be 
 return registrar->RegisterFactory();
But I am OK if you do want to get the message on debug build.
Attachment #8532924 - Flags: review?(hiikezoe) → review+
Comment on attachment 8532920 [details] [diff] [review]
(comm-esr31: clone) Part1: Clone nsCollationMacUC to comm-esr31.

Review of attachment 8532920 [details] [diff] [review]:
-----------------------------------------------------------------

Magnus, could you please review this?

::: mailnews/intl/mac/nsCollationMacUC.cpp
@@ +12,5 @@
> +namespace mailnews {
> +
> +NS_IMPL_ISUPPORTS(nsCollationMacUC, nsICollation)
> +
> +nsCollationMacUC::nsCollationMacUC() 

I know this file is just copy-n-pasted from mozille-central but please remove a tailing space on line end.
Attachment #8532920 - Flags: review?(mkmelin+mozilla)
Attachment #8532925 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8532920 [details] [diff] [review]
(comm-esr31: clone) Part1: Clone nsCollationMacUC to comm-esr31.

Review of attachment 8532920 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, build stuff is not my thing. Try standard8 or jcranmer?
Attachment #8532920 - Flags: review?(mkmelin+mozilla)
Attachment #8532925 - Flags: review?(mkmelin+mozilla)
Removed trailing spaces.

Part1:
  1. Clone nsCollationMacUC implementation to comm-esr31,
     with same name for simplicity,
     adding extra namespace to avoid name conflict with original one
Attachment #8532920 - Attachment is obsolete: true
Attachment #8534795 - Flags: review?(standard8)
Thank you for reviewing :)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #181)
> Comment on attachment 8532924 [details] [diff] [review]
> (comm-esr31: clone) Part2: Register cloned nsCollationMacUC.
> 
> Review of attachment 8532924 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/intl/mac/nsCollationRegistrar.cpp
> @@ +61,5 @@
> > +  rv = registrar->RegisterFactory(NS_COLLATION_CID, "Fixed nsCollationMacUC",
> > +                                  NS_COLLATION_CONTRACTID, fixedFactory);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  return NS_OK;
> 
> This should be 
>  return registrar->RegisterFactory();
> But I am OK if you do want to get the message on debug build.

Fixed.
Attachment #8532924 - Attachment is obsolete: true
Attachment #8534797 - Flags: review+
rebased.

Part3 (almost same as the patch Part2 for mozilla-esr31):
  1. Add --with-intl-api configure option
  2. Apply ICU patch to nsCollationMacUC
  3. Set intl.collation.mac.use_icu to true
Attachment #8532925 - Attachment is obsolete: true
Attachment #8534798 - Flags: review?(joshmoz)
Attachment #8534798 - Flags: review?(joshmoz) → review+
Comment on attachment 8534795 [details] [diff] [review]
(comm-esr31: clone) Part1: Clone nsCollationMacUC to comm-esr31.

Review of attachment 8534795 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for these patches. Lets get these landed on esr31, so we can get some testing in.

Are you able to land these yourself?

Also, if there's an issue with the patches, where are the most likely places for an issue to be seen?
Attachment #8534795 - Flags: review?(standard8)
Attachment #8534795 - Flags: review+
Attachment #8534795 - Flags: approval-comm-esr31+
Attachment #8534797 - Flags: approval-comm-esr31+
Attachment #8534798 - Flags: approval-comm-esr31+
(In reply to Mark Banner (:standard8) from comment #187)
> Comment on attachment 8534795 [details] [diff] [review]
> (comm-esr31: clone) Part1: Clone nsCollationMacUC to comm-esr31.
> 
> Review of attachment 8534795 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for these patches. Lets get these landed on esr31, so we can get some
> testing in.
> 
> Are you able to land these yourself?
> 
> Also, if there's an issue with the patches, where are the most likely places
> for an issue to be seen?

Thank you!
Landed them:
  https://hg.mozilla.org/releases/comm-esr31/rev/d19dca0662b9
  https://hg.mozilla.org/releases/comm-esr31/rev/1e96cbec4b24
  https://hg.mozilla.org/releases/comm-esr31/rev/bb2d21180a2e

If an issue happens with those patches, it would be seen in UI control with sorting feature, such as Folder Pane, Message pane, Gloda search restuls, and some XUL 'tree' or 'menu' elements, as wrong sorting order or missing contents.

I'll test it on Mac OS X 10.9.5, with several System Locales. It would be nice if anyonce can test it on other OS versions, especially on Yosemite.
Oops, the test was wrong.
"@mozilla.org/messenger/collation-registrar;1" is always registered. It checks the System Locale, and replace "@mozilla.org/intl/collation;1" if it starts with "ja".

We should test whether the "@mozilla.org/intl/collation;1" is replaced or not.

I'll prepare the patch soon.
Unfortunatelly, there is no way to know whether the nsCollation factory is actually replaced by nsCollationRegistrar::Register or not.

The difference between original one and cloned one is only in their "aName" parameters of nsIComponentRegistrar.RegisterFactory, however, "aName" parameter is just ignored in RegisterFactory method:
  http://mxr.mozilla.org/comm-esr31/source/mozilla/xpcom/components/nsComponentManager.cpp#1482

Then, the difference appears in Part 3, in their sorting order (comment #29).
so I'd like to remove test_collation_registrar.js from Part 2,
and add Part 4 to test nsCollationRegistrar::Register.
Removed test_collation_registrar.js and the change to xpcshell.ini
Attachment #8534797 - Attachment is obsolete: true
Attachment #8544032 - Flags: review?(hiikezoe)
Attachment #8544032 - Attachment description: Part2: Register cloned nsCollationMacUC. → (comm-esr31: clone) Part2: Register cloned nsCollationMacUC.
Added test_collation_registrar.js,
which tests the registration by checking the sorting order difference between ICU and CoreServices.

Both changes are only in xpcshell tests.
Attachment #8544034 - Flags: review?(hiikezoe)
Since there is no change in application binary, I tested the binary in previous commit:
  http://ftp.mozilla.org/pub/mozilla.org/thunderbird/tinderbox-builds/comm-esr31-macosx64/1420462779/

Steps are in comment #140.

Results:
  Mac's system locale | Thunderbird's locale | Result (shown/total)
  --------------------+----------------------+-----------------------
  English             | en-US                | 20/20
  Deutsch             | en-US                | 20/20
  Japanese            | en-US                | 20/20

I'll do some more tests.
Attachment #8544032 - Flags: review?(hiikezoe) → review+
Comment on attachment 8544034 [details] [diff] [review]
(comm-esr31: clone) Part4: Add test for cloned nsCollationMacUC registration.

Review of attachment 8544034 [details] [diff] [review]:
-----------------------------------------------------------------

Please use let instead of var.
Attachment #8544034 - Flags: review?(hiikezoe) → review+
Comment on attachment 8544032 [details] [diff] [review]
(comm-esr31: clone) Part2: Register cloned nsCollationMacUC.

Removed broken test from previous Part 2.
Attachment #8544032 - Flags: approval-comm-esr31?
Comment on attachment 8544034 [details] [diff] [review]
(comm-esr31: clone) Part4: Add test for cloned nsCollationMacUC registration.

Added test for nsCollationRegistrar which was added in Part 2.
Attachment #8544034 - Flags: approval-comm-esr31?
Attachment #8544032 - Flags: approval-comm-esr31? → approval-comm-esr31+
Attachment #8544034 - Flags: approval-comm-esr31? → approval-comm-esr31+
Group: core-security → core-security-release
Group: core-security-release
Blocks: 1269246
You need to log in before you can comment on or make changes to this bug.