548480 - [10.6] crash [@ CFHash] SMTP password prompt with IME enabled

Status: VERIFIED FIXED

(MailNews Core :: Security, defect)

Description

[Michael A. Pasek]

crash [@ CFHash ] SMTP password prompt

#38 in Mac 3.0.1 Top 100, not in top 100 overall (total 43, all Mac, Jan29-Feb24)
3.0b4 - 1 occurrences, MacOS 10.6.2 (10C540) [says "No extensions", but it's Eudora 8.0b8]
3.0 - 5 occurrences:
  1 MacOS 10.5.8 (9L31a)
  1 MacOS 10.6.0 (10A432)
  3 MacOS 10.6.2 (10C540)
3.0.1 - 37 occurrences, all MacOS 10.6.2 (10C540)
(no occurrences for 1.9.2 or 1.9.3 branches)

Last thing done in TB was to put up a Modal dialog for an SMTP password prompt.

bp-dd7773a8-6568-4991-8d65-f1a542100127  v3.0.1  Build20100111130305
comment: [Japanese] When I try to send, crash.
0	CoreFoundation	CFHash	
1	CoreFoundation	___CFBasicHashFindBucket1	
2	CoreFoundation	CFBasicHashFindBucket	
3	CoreFoundation	CFDictionaryGetValueIfPresent	
4	HIToolbox	FindDefIMInstanceElem	
5	HIToolbox	GetDefIMInstanceTheInstance	
6	HIToolbox	SendTextInputEvent	
7	HIToolbox	-[IMKInputSession _attributesFromRangeViaGetSelectedText:]	
8	CoreFoundation	__invoking___	
9	CoreFoundation	-[NSInvocation invoke]	
10	CoreFoundation	-[NSInvocation invokeWithTarget:]	
11	CoreFoundation	___forwarding___	
12	CoreFoundation	__forwarding_prep_0___	
13	CoreFoundation	__invoking___	
14	CoreFoundation	-[NSInvocation invoke]	
15	Foundation	-[NSConnection dispatchInvocation:]	
16	Foundation	-[NSConnection handleRequest:sequence:]	
17	Foundation	-[NSConnection handlePortCoder:]	
18	Foundation	-[NSConcretePortCoder dispatch]	
19	Foundation	__NSFireMachPort	
20	CoreFoundation	__CFMachPortPerform	

bp-408610d5-31cf-40fd-85c8-e68c62100210  v3.0b4  Build20100105093006
0	CoreFoundation	CFHash	
1	CoreFoundation	___CFBasicHashFindBucket1	
2	CoreFoundation	CFBasicHashFindBucket	
3	CoreFoundation	CFDictionaryGetValueIfPresent	
4	HIToolbox	FindDefIMInstanceElem	
5	HIToolbox	GetDefIMInstanceTheInstance	
6	HIToolbox	SendTextInputEvent	
7	HIToolbox	-[IMKInputSession _attributesFromRangeViaGetSelectedText:]	
8	CoreFoundation	__invoking___	
9	CoreFoundation	-[NSInvocation invoke]	
10	CoreFoundation	-[NSInvocation invokeWithTarget:]	
11	CoreFoundation	___forwarding___	
12	CoreFoundation	__forwarding_prep_0___	
13	CoreFoundation	__invoking___	
14	CoreFoundation	-[NSInvocation invoke]	
15	Foundation	-[NSConnection dispatchInvocation:]	
16	Foundation	-[NSConnection handleRequest:sequence:]	
17	Foundation	-[NSConnection handlePortCoder:]	
18	Foundation	-[NSConcretePortCoder dispatch]	
19	Foundation	__NSFireMachPort	
20	CoreFoundation	__CFMachPortPerform

Comment 1

[Makoto Kato [:m_kato]]

This occurs on 3.1b2pre + 10.6.3
bp-b5ad1867-f977-46dc-84eb-6675d2100408

Comment 2

[Makoto Kato [:m_kato]]

On my test environment, this can produce on following step.  rate is 100% by following.

- Env
Mac OS X 10.6.3 + Lanikai 3.1b2pre 20100407

- Step
1. Open composer window (Select [Write] button)
2. Input some address to To: field
3. Set focus to body
4. Turn on IME
5. Click [Send] button

- Result
Crash when password dialog appears.

- Note
When clicking [send] after turned off IME, this doesn't occurs.  Of course, password dialog doesn't show up due to stored password, this doesn't occur.

Comment 3

[Ludovic Hirlimann [:Usul]]

Adding a mac guru and a i18 guru

Comment 4

[Makoto Kato [:m_kato]]

It seems that this doesn't occurs on 1.9.3 based (3.2a1pre).

Nakano-san, do you know anything (or fixed bug) about this crash?   Although this crash is in CoreFoundation, this only occurs with IME on and doesn't occurs on 1.9.3 based.

Comment 5

[Makoto Kato [:m_kato]]

Query on Firefox.  Most comment is password dialog on Firefox, not Thunderbird.

http://crash-stats.mozilla.com/report/list?product=Firefox&platform=mac&query_search=signature&query_type=exact&query=CFHash&date=&range_value=1&range_unit=weeks&process_type=all&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&signature=CFHash

Comment 6

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

bug 513952?

Comment 7

[Steven Michaud [:smichaud] (Retired)]

I can't reproduce any crashes using the STR from comment #2, testing
with today's Lanikai nightly on OS X 10.6.3.

> 4. Turn on IME

I'm not entirely sure what this means.  I tried all of the following,
none of which triggered a crash:

a) Use the Flags menu to select Kotoeri Hiragana input.

b) Use command-space to select Kotoeri Hiragana input.

c) Use either of the above to select Kotoeri Hiragana input and type
   something, without hitting Return (that is without doing anything
   to "commit" the text).

Comment 8

[Steven Michaud [:smichaud] (Retired)]

> I tried all of the following, none of which triggered a crash

None of which triggered a crash at step 5.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

But oops, I discovered something very strange!  Hiragana/Katakana
input don't work in the Composer window (in Lanikai) until I've
connected to the server (and provided my password).  This may be why I
don't crash on Lanikai.

But I also don't crash on the comm-1.9.1 branch (3.0.5pre).  And there
Hiragana and Katakana input work fine (in the Composer window) even
before I've logged in to the server.

Comment 10

[Makoto Kato [:m_kato]]

(In reply to comment #6)
> bug 513952?

Humm... It seems not to occurs after this (I tested 2009-10-08-09-comm-central-trunk.  For 1 month at that time, comm-central build wasn't built due to some issue). 

This is the core issue (not thunderbird!) and this seems to reproduce on Firefox using password dialog of Software Security Device (there is a post in Japanese forum http://forums.mozillazine.jp/viewtopic.php?p=33872).

Comment 11

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

Can you log the IME code with |#define DEBUG_IME 1| in nsChildView.mm?

Comment 12

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

(In reply to comment #11)
> Can you log the IME code with |#define DEBUG_IME 1| in nsChildView.mm?

Ah, looks like it's not useful for this bug. Sorry for the spam.

Comment 13

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

The IME code between 1.9.0 and 1.9.2.x was using some hacky way because we needed to support 10.4. I guess that such hacky code causes this bug. So, it seems that we cannot fix this bug easily if my guess is right...

Comment 14

[Makoto Kato [:m_kato]]

It is only 1.9.1 and 1.9.2 that Fx/Tb crashes by nsXULWindow::ShowModal().  But like bp-0c21ee29-751d-4285-9c5a-a31af2100327, same crash occurs on Firefox 3.7 (1.9.3) by nsAppShell::ProcessNextNativeEvent.

Both stacks have ActivateTSMDocument_GetMenu_TimerHandler.  Although I believe that this is OS bug, this may be fixed that IME is turned off before [NSApp nextEventMatchingMask:NSAnyEventMask].  But this is not good idea...

Comment 15

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

(In reply to comment #14)
> Both stacks have ActivateTSMDocument_GetMenu_TimerHandler.  Although I believe
> that this is OS bug, this may be fixed that IME is turned off before [NSApp
> nextEventMatchingMask:NSAnyEventMask].  But this is not good idea...

The IME turning off is bad thing on Mac because the IME state is shared on all applications in default settings.

Comment 16

[Ludovic Hirlimann [:Usul]]

This is now our top crasher for 3.1 :-(

Comment 17

[Steven Michaud [:smichaud] (Retired)]

This appears to happen only on OS X 10.6.X, so it may be an OS bug.

I'll try to find time to work on this after Whistler.

Comment 18

[Makoto Kato [:m_kato]]

Humm, 

http://mxr.mozilla.org/mozilla1.9.2/source/widget/src/cocoa/nsChildView.mm#2019
2019     case nsIWidget::IME_STATUS_PASSWORD:
2020       nsTSMManager::SetRomanKeyboardsOnly(PR_TRUE);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2021       nsTSMManager::EnableIME(PR_FALSE);
2022       break;

nsTSMManager::SetRomanKeyboardsOnly causes this crash.  When comment out this, this doesn't occur.  Maybe, I think KeyScript() is root cause.

Thuderbird 3.2 / Gecko 1.9.3 (2.0) replaces with TIS based code, so this problem doesn't occur

Comment 19

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

http://mxr.mozilla.org/mozilla-central/source/widget/src/cocoa/nsCocoaTextInputHandler.mm#819

Looks like we can port the new method to the branch easily. Should I do it after summit? Or does somebody do it?

Comment 20

[Steven Michaud [:smichaud] (Retired)]

> Should I do it after summit?

Please do :-)

Comment 21

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

Attachment: Patch v1.0

I wrote this patch but I cannot test this until I get home after the summit.

Comment 22

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

Attachment: Patch v1.0

Oops, sorry, I posted a wrong patch.

Comment 23

[Makoto Kato [:m_kato]]

> +CFStringRef kOurTSMDocumentEnabledInputSourcesPropertyTag = NULL;

This is TSMDocumentPropertyTag type, not CFStringRef type, isn't this?

Also, TSMDocumentEnabledInputSourcesPropertyTag = 'enis', so, is it unnecessarily to import value from dylib?

Comment 24

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

Thanks, I'll update it in next patch.

BTW, I cannot reproduce this bug on Fx on both 10.5 and 10.6 by following STR:

1. Open Error Console
2. Input following code:
> var arg1 = new Object(), arg2 = new Object(); Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(Components.interfaces.nsIPromptService).promptPassword(window, "title", "text", arg1, "msg", arg2);
3. Press Enter

I tested both IME on and off cases...

Comment 25

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

Attachment: Patch v1.1

Kato-san, would you test this patch?

Comment 26

[Makoto Kato [:m_kato]]

Comment on attachment 457267 [details] [diff] [review]
Patch v1.1

Humm, still crash...

When I comment out TSMSetDocumentProperty, it doesn't crash...

When clicking [Send] button, progress window is shown up, then password dialog appears.   I have to try creating test case without Tb.

Comment 27

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

Ugh, I guess that I need to use the timer hack...

Comment 28

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

Attachment: Patch v2.0 (just ported from trunk)

Comment 29

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

Attachment: Patch v2.0 (guaranteed that the KeyScript() isn't called during focus moving)

Comment 30

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

I hope that these patches fixes this bug. Kato-san, would you test them?

I found a bug on trunk, this bug might be on trunk too. That is IsFocused() doesn't check [window isSheet]. So, password fields on sheet dialog is never disabled the non-Roman keyboard layouts. That might just hide this bug on trunk.

I'll file the bug for trunk.

Comment 31

[Makoto Kato [:m_kato]]

Both still crashes... Humm, I have no idea.

Also, I found a workaround that mailnews.show_send_progress=false.

Comment 32

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

Um, the fix of bug 582893 might cause this bug on trunk??

Comment 33

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

I fixed bug 582893 on trunk. If we can reproduce this bug on trunk too, we need to rethink the cause of this bug.

Comment 34

[Wayne Mery (:wsmwk)]

#1 crash for v3.1.2, which is a huge thing, given this is as Mac-only crash.
Ideal would be to get this in time for v3.1.3, but there's only a couple days to get that done? 

Has this been tested on branch (tryserver?)?  And is there any way to get this in for v3.1.3?

Comment 35

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

First, we should confirm whether this bug is also on trunk or not because bug 582893 was fixed. Before that, the keyboard layout limitation didn't work on trunk at the situation of this bug. If we can reproduce this bug on trunk too, we should look for another approach. Otherwise, we should look for the minimum change for back-port the new code from trunk.

Comment 36

[Ludovic Hirlimann [:Usul]]

Makoto could you try to see if you can crash on trunk ?

Comment 37

[Makoto Kato [:m_kato]]

(In reply to comment #36)
> Makoto could you try to see if you can crash on trunk ?

I cannot repro this on trunk. See comment #4 and comment #10.

Comment 38

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

(In reply to comment #37)
> (In reply to comment #36)
> > Makoto could you try to see if you can crash on trunk ?
> 
> I cannot repro this on trunk. See comment #4 and comment #10.

Kato-san, se comment 35. The IME state change on the sheet dialog didn't work on trunk until fixing bug 582893. So, this bug might be able to be reproduced on trunk too. If we can reproduce this bug on trunk too, I need to find a new wrong point in trunk. Otherwise, I should retry the fix around KeyScript() on the branch.

Comment 39

[Makoto Kato [:m_kato]]

(In reply to comment #38)
> (In reply to comment #37)
> > (In reply to comment #36)
> > > Makoto could you try to see if you can crash on trunk ?
> > 
> > I cannot repro this on trunk. See comment #4 and comment #10.
> 
> Kato-san, se comment 35. The IME state change on the sheet dialog didn't work
> on trunk until fixing bug 582893. So, this bug might be able to be reproduced
> on trunk too. If we can reproduce this bug on trunk too, I need to find a new
> wrong point in trunk. Otherwise, I should retry the fix around KeyScript() on
> the branch.

Although I tested my build machine, before bug 513952, it always crash.  After bug 513952, I cannot reproduce this.

Comment 40

[Makoto Kato [:m_kato]]

We need merge bug 582893's fix into patch v2.  TSMGetActiveDocument() doesn't return correct value.

Comment 41

[Makoto Kato [:m_kato]]

- bug 513952 with calling KeyScript() instead of TSMSetDocumentProperty() will always crash
- only bug 513952 doesn't change to ASCII into PASSWORD field.
- bug 513952 + bug 582893 can change to ASCII into PASSWORD field and no crash
- calling TSMSetDocumentProperty() instead of KeyScript() after [[NSInputManager currentInputManager], It seems to no crash.

I will verify this today.

Comment 42

[Makoto Kato [:m_kato]]

Attachment: fix v3

Comment 43

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

Comment on attachment 471802 [details] [diff] [review]
fix v3

Thanks, I think that this is correct approach.

Comment 44

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

(In reply to comment #43)
> Comment on attachment 471802 [details] [diff] [review]
> fix v3
> 
> Thanks, I think that this is correct approach.

Comment 45

[Makoto Kato [:m_kato]]

Comment on attachment 471802 [details] [diff] [review]
fix v3

This is the top crasher of Thunderbird 3.1.2

Comment 46

[Mark Banner (:standard8)]

(note removed blocking-thunderbird3.1 only as I'm tracking this via the [tb31needs] in the whiteboard as this is really a core bug)

Comment 47

[Daniel Veditz [:dveditz]]

Comment on attachment 471802 [details] [diff] [review]
fix v3

Approved for 1.9.2.11, a=dveditz for release-drivers

Comment 48

[Mark Banner (:standard8)]

I landed this on the 1.9.2 branch as its one of the bugs we really need for Thunderbird:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d1974266d4d7

Comment 49

[Al Billings [:abillings - ex-MoCo]]

Do we actually have a working repro scenario for 1.9.2? I read all the comments above and I keep seeing that people cannot reproduce the problem (and a lack of specificity in the steps).

Comment 50

[Makoto Kato [:m_kato]]

I verified by Lanikai/3.1.5pre Gecko/20100921.  Could anyone change status (I don't know whiteboard status that Tb team uses)?

Comment 51

[Al Billings [:abillings - ex-MoCo]]

I'll mark it verified1.9.2. Thank you!

Comment 52

[Wayne Mery (:wsmwk)]

so far, this isn't appearing in thunderbird v3.1.5