crashes [@ CalcCharacterPositionAtoW][@ imm32.dll@0x3e24] involving Chinese IME

RESOLVED FIXED in mozilla1.9.2a1

Status

()

P1
critical
RESOLVED FIXED
11 years ago
7 years ago

People

(Reporter: dbaron, Assigned: masayuki)

Tracking

(5 keywords)

Trunk
mozilla1.9.2a1
x86
Windows XP
crash, inputmethod, topcrash, verified1.9.0.6, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 -
wanted1.9.1 +
blocking1.9 -

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments, 8 obsolete attachments)

There are some crashes showing up around rank #40 in the topcrash list (going back multiple betas, and in nightlies) where the top of the stack is the following (and the parts below vary a lot and may be garbage):

0  	CalcCharacterPositionAtoW  	
1 	InternalGetCompositionStringW 	
2 	ImmGetCompositionStringW 	
3 	nsWindow::OnIMEComposition(long) 	mozilla/widget/src/windows/nsWindow.cpp:6593

All of the user comments appear to be in Chinese, so it wouldn't surprise me if this is a problem with some specific Chinese IME.  (Maybe we could find out by looking at the Module List for the crashes?)  I'm not sure if Chinese usage of the betas is as high as it is for the releases, so this is a little worrying (I normally wouldn't file a topcrash this low on the list).


User comments include:

转换输入法,输入法是微软拼音.

使用智能ABC输入法时会崩溃

为什么会崩溃Add-ons: {972ce4c6-7e08-4474-a285-3208198ce6fd}:2.0 BuildID: 2008030317 Comments: 为什么会崩溃 CrashTime: 1206152924 InstallTime: 1205291856 ProductName: Firefox SecondsSinceLastCrash: 68225 StartupTime: 1206152905 Theme: classic/1.0 URL: http://www.baidu.com/ UserID: 4126b85b-edb4-46f2-855f-f1a6c396fb47 Vendor: Mozilla Version: 3.0b4 此报告也包含本应用程序崩溃时状态的技术信息.

怎么回事,怎么又这样? 郁闷……

我也没怎么动,只加了些扩展. 很喜欢它,第一次的时候好像还是1点几,希望3.0正式版早点出,祝你们身体健康



Running this whole chunk of comments through Google Translate yields

Conversions, Microsoft Pinyin input method.

Intelligent use of ABC input method will collapse

Why would collapse Add-ons: (972ce4c6-7e08-4474-a285-3208198ce6fd): 2.0 BuildID: 2008030317 Comments: Why would crumble CrashTime: 1206152924 InstallTime: 1205291856 ProductName: Firefox SecondsSinceLastCrash: 68,225 StartupTime: 1206152905 Theme: classic/1.0 URL: http://www.baidu.com/ UserID: 4126b85b-edb4-46f2-855f-f1a6c396fb47 Vendor: Mozilla Version: 3.0b4 this report also includes the collapse of the state of application of technical information.

How matter, how is this? Depressed……

How I did not move, and only a little expansion. Liked it, the first time like 1:00 or several, that the earlier 3.0 version, and I wish you good health




I looked at the module list for 5 crashes.  All 5 have had at least one "*.ime" file listed in the module list.  All of them have had WINABC.IME listed (this also seems to be mentioned in the comments) and also MSCTFIME.IME.  Some have also had WINPY.IME (probably the other one mentioned?), SogouPy.ime, and jpwb.IME, and WINZM.IME.
Flags: blocking1.9?
John, could you take a look at this and decide on blocking?

Comment 2

11 years ago
I don't have a clue about IME code.  Masayuki, any thoughts on whether this should block?
Looks like the bug of the IMEs. We are not using A string, we are only using W APIs. I guess that they are using A string in them. So, the out value computing logic has this crash bug.
I tested sobou_pinyin on latest trunk, however, I cannot reproduce this crash. We need to find the testers.
(In reply to comment #4)
> sobou_pinyin

sogou_pinyin, sorry.
+'ing this for now as this could really be a big deal if we don't have enough beta testers on this.  How can we get more eyes on this?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Have we pinged anyone from Mozilla Online about this?
So this is always a crash at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/windows/nsWindow.cpp&rev=3.734&mark=6605#6605 -- the last time this line was changed was in bug 332076, which was a cleanup patch for removing support for pre-win2k systems.  That particular line looks the same

The most worrying thing here in all the IME code is that errors are not handled; ImmGetCompositionData can return IMM_ERROR_NODATA and IMM_ERROR_GENERAL, and we never check the return value anywhere.  I don't know IME at all, so I'm not sure what the right thing to do would be if we ever did get an error (do we need to end IME or anything?).
Looking at that code again (current code at http://mxr.mozilla.org/seamonkey/source/widget/src/windows/nsWindow.cpp#6647 ), I'm wondering if there's something bogus going on with the size computation.  We're taking the return value, which according to the docs is always in bytes, and then dividing by sizeof(PRUint32).  This will lead to a too-short buffer if we ever get something back that's not a multiple of 4, but we allocate 32 uints of slop in the array anyway, so I don't think that's the problem.

What I'm wondering is if maybe some method is returning a bogus number here, maybe the length in longs, as opposed to length*4?  Wonder if we should just skip the division by 4?  Worst case we'll allocate a 4-times-too-big buffer, but I don't think this array ever gets too big.

Updated

11 years ago
Severity: normal → critical
Keywords: crash, topcrash

Comment 10

11 years ago
Given the lack of confirmation or understanding whether this is our bug taking it off the blocker list.  Masayuki if you have more info or thing this should change please post and re-nominate this bug. 
Flags: blocking1.9+ → blocking1.9-

Comment 11

10 years ago
Early RC1 data shows this as the #7 topcrash.

http://crash-stats.mozilla.com/topcrasher/byversion/Firefox/3.0

Comment 12

10 years ago
I am having crashes when typing Chinese into location bar. My IME is Google Pinyin.

I don't have this problem before the 3.0 official release, before which I had been using 3.0 beta nightlies without any problems.
While this the number 91 topcrash in Firefox 3.0.3, it's number 6 in Firefox 3.1b1. Since it's Windows-only in 3.1b1, it could possibly be caused by one user crashing a lot? In 3.0.3, it happens on both Windows and Mac.

Probably needs some more investigation for 1.9.1 anyway...
Flags: blocking1.9.1?
Err, ignore the Mac part of comment 13. I was looking at the wrong line...
Patterns like that could also be related to crashes that cause users to stop using Firefox; you'd see them as much lower frequency after a release had been out for a while.
Gen-san, do you know good testers for this?
This also appears to happen [@ imm32.dll@0x3e24], though the top three frames are different.

Sample stack [@ imm32.dll@0x3e24] from bp-e35907a1-a49f-11dd-922e-001321b13766:

Frame  	Module  	Signature [Expand]  	Source
0 	imm32.dll 	imm32.dll@0x3e24 	
1 	imm32.dll 	imm32.dll@0x53d1 	
2 	imm32.dll 	imm32.dll@0x54ee 	
3 	xul.dll 	nsWindow::OnIMEComposition 	widget/src/windows/nsWindow.cpp:6913
4 	xul.dll 	xul.dll@0x32d3c5 	
5 	xul.dll 	nsWindow::WindowProc 	widget/src/windows/nsWindow.cpp:1165
6 	xul.dll 	xul.dll@0x27dad1 	
7 	xul.dll 	nsMIMEHeaderParamImpl::DecodeParameter 	

Sample stack [@ CalcCharacterPositionAtoW] from bp-5c3a3478-a911-11dd-815b-001a4bd43ef6:

Frame  	Module  	Signature [Expand]  	Source
0 	imm32.dll 	CalcCharacterPositionAtoW 	
1 	imm32.dll 	InternalGetCompositionStringW 	
2 	imm32.dll 	ImmGetCompositionStringW 	
3 	xul.dll 	nsWindow::OnIMEComposition 	widget/src/windows/nsWindow.cpp:6913
4 	xul.dll 	xul.dll@0x32d3c5 	
5 	xul.dll 	nsWindow::WindowProc 	widget/src/windows/nsWindow.cpp:1165
6 	xul.dll 	xul.dll@0x27dad1 	
7 	xul.dll 	nsMIMEHeaderParamImpl::DecodeParameter 	

Separately, these crashes are the number 5 and 6 topcrashes. Combined, it puts them at #3 for Firefox 3.1b1.

David, good point on users potentially not using Firefox after crashing a lot.
Summary: crashes [@ CalcCharacterPositionAtoW] involving Chinese IME → crashes [@ CalcCharacterPositionAtoW][@ imm32.dll@0x3e24] involving Chinese IME

Comment 18

10 years ago
Adding Li to get help on simplified Chinese testing.
I found this:
http://www.firefox.net.cn/newforum/viewtopic.php?p=187665

If somebody can reproduce this bug, please tell me how to reproduce this bug. (what IME is used, where is the IME download site, the steps to reproduce)
(In reply to comment #17)
> This also appears to happen [@ imm32.dll@0x3e24], though the top three frames
> are different.

Those are just two slightly different versions of imm32.dll, one of which we have symbols for and one of which we don't. I would bet cash money that it's the same crash, and the stack in the one with symbols is correct.

imm32.dll	5.1.2600.5512	F7A5B5DB13324153B57AAF340C77EA512	imm32.pdb
imm32.dll	5.1.2600.2180	2C17A49C251B4C8EB9E2AD13D7D9EA162	imm32.pdb
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Priority: P2 → P1

Comment 21

10 years ago
    I'm Amax Guan from Mozilla Online. Is there anything I can help on this bug? I don't know anything about IME yet, how can I get a quickstart? If there is something I can do, please say it. And you can reach me at lguan@mozilla.com

Comment 22

10 years ago
Masayuki san,
    I just reproduced the bug. From the forum link you gave, and my crush experience, this bug is deeply releated with the chinese IME intelligent ABC. My Firefox crushes when I switch from other IME to ABC and do any inputs.

Comment 23

10 years ago
Firefox will not crush if ABC is my first IME in Chinese, but if I open another chinese IME first and then switch to ABC, it will crush everytime I type.
Can you please go to about:crashes and paste in a few crash report IDs so we can confirm this is the same crash?

Comment 25

10 years ago
The crush happens when I press the first key in ABC IME mode, it didn't wait until I input the whole word and press space to change them to chinese characters.
How do we install the intelligent ABC IME?

Comment 27

10 years ago
These are two crushes I just created
e4a27097-cbd6-4c7b-877b-bd3f20081119
020d5b1a-b1b9-47ff-8305-ed6420081119

intelligent ABC IME is embaded in WindowsXP simplified chinese version. So I think maybe you can install chinese support for your Windows XP, and find it there.(or install a chinese version windows xp instead) Another way is to download it from here http://dl.pconline.com.cn/html_2/1/77/id=6157&pn=0&linkPage=1.html. (click any link in the link list with an down arrow on the left, in the second column of the page. I can't send you the link directly, because it only works with a referer. Besides, this version may not be the same version embeded in my Windows XP, so if it's possible that this version is not bugos)

Comment 29

10 years ago
Cool! Is it the same bug we are talking about?
I'm sure this crash is the same as the chinese forum Masayuki san gave talk about.
The crash reports you reported are indeed this bug. Precise steps will help us narrow this down and the ones in comment 22 and comment 27 help. I'll try and investigate.
Unfortunately, I cannot install the IME of comment 27 :-(

However, comment 23 and comment 25 is very important hint. I'll create a test patch and test build.
https://build.mozilla.org/tryserver-builds/2008-11-19_22:17-masayuki@d-toybox.com-attachment349130 [review]/

The test build of the patch is here.  Lingfeng.Guan, would you test the build? However, please note that the build is a trunk build which is under development.

Comment 34

10 years ago
ok. I'm on it. But what's the expected result of the patch?

Comment 35

10 years ago
I've tested the patch. Unfortunately, the bug is still there.
But I found something else.
If I switch from google Pinyin IME or MS Pinyin to ABC, it will not crash.
It crashes if I switch from Sogou Pinyin to ABC.
Oh, really? Would you tell me the crash id on the test build?
Thank you for your mail. I installed ABC completely. And I could to reproduce this bug on my system. I'll try to fix this.

Comment 38

10 years ago
Perfect! And thank you very much for fixing this bug for Chinese users, it should be my responsibility to do this kind of things.
I'm new in Mozilla, and I don't know much about fixing bugs yet, but I want to learn about how to fix bugs like this very much. So if you have anything, just call me, I want to learn. I'll do it in spare time:)
Created attachment 349355 [details] [diff] [review]
Patch v1.0

Probably, this fixes this bug. However, I hope this patch should be tested before review. This patch uses A API only when the current keyboard code page is 936 (Simplified Chinese) and the composition string doesn't have one or more Unicode characters which are not in CP936. So, this patch may have impact for all IMEs for Simplified Chinese.
Assignee: nobody → masayuki
Attachment #349130 - Attachment is obsolete: true
Status: NEW → ASSIGNED

Comment 40

10 years ago
How do I apply this patch?

Comment 42

10 years ago
I've tested the patch, it seems to be working for me
(Assignee)

Updated

10 years ago
Attachment #349355 - Flags: review?(VYV03354)
Comment on attachment 349355 [details] [diff] [review]
Patch v1.0

Thank you for your test.

I need Kimura-san's review, first. Kimura-san, please check comment 39 for the detail of this patch.
I filed this bug to bugzilla-jp, too. Kimura-san, if you want to use Japanese language for the review, please comment to it.

http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=6438
Created attachment 349437 [details] [diff] [review]
Patch v2.0

Thank you, Kimura-san.

I'll build this patch on tryserver...
Attachment #349355 - Attachment is obsolete: true
Attachment #349437 - Flags: review?(VYV03354)
Attachment #349355 - Flags: review?(VYV03354)

Comment 46

10 years ago
Comment on attachment 349437 [details] [diff] [review]
Patch v2.0

>+static UINT GetCodePageForLangID(WORD aLangID)

This function looks terribly complicated. Why not just

  UINT cp;
  GetLocaleInfoA(MAKELCID(aLangID, SORT_DEFAULT), 
                 LOCALE_IDEFAULTANSICODEPAGE | LOCALE_RETURN_NUMBER,
                 (PSTR)&cp, sizeof(cp));
  return cp;
(In reply to comment #46)
> (From update of attachment 349437 [details] [diff] [review])
> >+static UINT GetCodePageForLangID(WORD aLangID)
> 
> This function looks terribly complicated. Why not just
> 
>   UINT cp;
>   GetLocaleInfoA(MAKELCID(aLangID, SORT_DEFAULT), 
>                  LOCALE_IDEFAULTANSICODEPAGE | LOCALE_RETURN_NUMBER,
>                  (PSTR)&cp, sizeof(cp));
>   return cp;

I'm not sure. But Gecko 1.8.x used the code in my patch.

Comment 49

10 years ago
(In reply to comment #47)
> I'm not sure. But Gecko 1.8.x used the code in my patch.

Probably a leftover from the time when Gecko still needed to support Windows 95, which didn't support LOCALE_RETURN_NUMBER. Nowadays the minimum requirement for Gecko 1.8.x is W98 / NT 4, so the shorter version should be safe.
Created attachment 349470 [details] [diff] [review]
Patch v3.0

O.K. Win95 didn't support LOCALE_RETURN_NUMBER may be correct reason of the code on Gecko1.8.x. Thank you.
Attachment #349437 - Attachment is obsolete: true
Attachment #349470 - Flags: review?(VYV03354)
Attachment #349437 - Flags: review?(VYV03354)
I posted the latest patch to tryserver, but I'll go to bed. Please find the new test build from here:

https://build.mozilla.org/tryserver-builds/?C=M;O=D
Comment on attachment 349470 [details] [diff] [review]
Patch v3.0

For non-Japanese readers:
I said this bug was the return of bug 125573 due to my fix of bug 330276.
It's my mistake, sorry.

> +    WORD langID = (WORD)((DWORD)gKeyboardLayout & 0xFFFF);
LOWORD(gKeyboardLayout)

> +      if (compClauseArraySize > sIMECompClauseArraySize) {
> +        if (sIMECompClauseArray) 
> +          delete [] sIMECompClauseArray;
> +        // Allocate some extra space to avoid reallocations.
> +        sIMECompClauseArray = new PRUint32[compClauseArraySize + 32];
> +        sIMECompClauseArraySize = compClauseArraySize + 32;
> +      }
Should be compClauseArrayLength (in PRUint32) rather than compClauseArraySize
(in byte).
I think these names are very confusing.
What about compClauseArrayByteCount instead of compClauseArraySize
and sIMECompClauseArrayCapacity instead of sIMECompClauseArraySize?

> +        DWORD prop = ::ImmGetProperty(gKeyboardLayout, IGP_PROPERTY);
> +        useA_API = !(prop & IME_PROP_UNICODE);
You could cache the Unicodeness of IME as Gecko 1.8 did.

> -    NS_ASSERTION(compClauseLen2 == compClauseLen, "strange result");
> -    if (compClauseLen > compClauseLen2)
> -      compClauseLen = compClauseLen2;
> +      NS_ASSERTION(compClauseArraySize2 == compClauseArraySize,
> +                   "strange result");
> +      compClauseArrayLength = compClauseArraySize2 / sizeof(PRUint32);
Don't remove size check. NS_ASSERTION will disappear in release build.
Attachment #349470 - Flags: review?(VYV03354) → review-
>     // somehow the "Intellegent ABC IME" in Simplified Chinese
>     // window listen to the caret position to decide where to put the
>     // candidate window
>     if (gKeyboardLayout == ZH_CN_INTELLEGENT_ABC_IME)
>     {
>       CreateCaret(mWnd, nsnull, 1, 1);
>       SetCaretPos(candForm.ptCurrentPos.x, candForm.ptCurrentPos.y);
>       DestroyCaret();
>     }

Oh, I find such code. The next patch can be simpler.
Created attachment 349548 [details] [diff] [review]
Patch v4.0

Ugh... ZH_CN_INTELLEGENT_ABC_IME is not correct on my Vista environment. We should not use the fixed keyboard layout values of IMEs, probably. I'll check them in bug 171065.
Attachment #349470 - Attachment is obsolete: true
Attachment #349548 - Flags: review?(VYV03354)
Created attachment 349549 [details] [diff] [review]
Patch v4.0

oops, sorry for the spam.
Attachment #349548 - Attachment is obsolete: true
Attachment #349549 - Flags: review?(VYV03354)
Attachment #349548 - Flags: review?(VYV03354)

Updated

10 years ago
Attachment #349549 - Flags: review?(VYV03354) → review-
Comment on attachment 349549 [details] [diff] [review]
Patch v4.0

I would like to separate a style cleanup bug to make review easier,
But I wouldn't insist here.

> +  WORD langID = LOWORD(sKeyboardLayout);
> +  ::GetLocaleInfoA(MAKELCID(langID, SORT_DEFAULT),
> +                   LOCALE_IDEFAULTANSICODEPAGE | LOCALE_RETURN_NUMBER,
> +                   (PSTR)&sCurrentKeyboardCP, sizeof(sCurrentKeyboardCP));
> +  sIMEProperty = ::ImmGetProperty(sKeyboardLayout, IGP_PROPERTY);
Why don't you add them into KeyboardLayout::LoadLayout()?
InitKeyboardLayout() is always called whenever KeyboardLayout::LoadLayout()
is called. Do you have any good reason adding yet another function?
IMO it's better to move sCurrentKeyboardCP and sIMEProperty into
KeyboardLayout class.
They should be considered to be parts of the keyboard layout.

> +      if (compClauseArrayByteCount > sIMECompClauseArrayCapacity) {
> +        if (sIMECompClauseArray) 
> +          delete [] sIMECompClauseArray;
> +        // Allocate some extra space to avoid reallocations.
> +        sIMECompClauseArray = new PRUint32[compClauseArrayByteCount + 32];
> +        sIMECompClauseArrayCapacity = compClauseArrayByteCount + 32;
> +      }
Not fixed. For example, if ImmGetCompositionString() returns 8 (i.e. 2
PRUint32s), it will allocate 8 (not 2) plus 32 PRUint32s. Although it isn't
fatal, it's clearly wrong.
I proposed renaming variables to prevent future confusions. But please fix a
current confusion first.
(In reply to comment #56)
> (From update of attachment 349549 [details] [diff] [review])
> I would like to separate a style cleanup bug to make review easier,

If so, I think that we should not rename the sIMEComp* in this bug.
(In reply to comment #57)
> > I would like to separate a style cleanup bug to make review easier,
> If so, I think that we should not rename the sIMEComp* in this bug.
As I have already stated, I won't persist the issue.
Feel free to select what you prefer.
Created attachment 349623 [details] [diff] [review]
Patch v5.0

(In reply to comment #56)
> > +  WORD langID = LOWORD(sKeyboardLayout);
> > +  ::GetLocaleInfoA(MAKELCID(langID, SORT_DEFAULT),
> > +                   LOCALE_IDEFAULTANSICODEPAGE | LOCALE_RETURN_NUMBER,
> > +                   (PSTR)&sCurrentKeyboardCP, sizeof(sCurrentKeyboardCP));
> > +  sIMEProperty = ::ImmGetProperty(sKeyboardLayout, IGP_PROPERTY);
> Why don't you add them into KeyboardLayout::LoadLayout()?
> InitKeyboardLayout() is always called whenever KeyboardLayout::LoadLayout()
> is called. Do you have any good reason adding yet another function?
> IMO it's better to move sCurrentKeyboardCP and sIMEProperty into
> KeyboardLayout class.
> They should be considered to be parts of the keyboard layout.

It's nice idea.

> > +      if (compClauseArrayByteCount > sIMECompClauseArrayCapacity) {
> > +        if (sIMECompClauseArray) 
> > +          delete [] sIMECompClauseArray;
> > +        // Allocate some extra space to avoid reallocations.
> > +        sIMECompClauseArray = new PRUint32[compClauseArrayByteCount + 32];
> > +        sIMECompClauseArrayCapacity = compClauseArrayByteCount + 32;
> > +      }
> Not fixed. For example, if ImmGetCompositionString() returns 8 (i.e. 2
> PRUint32s), it will allocate 8 (not 2) plus 32 PRUint32s. Although it isn't
> fatal, it's clearly wrong.
> I proposed renaming variables to prevent future confusions. But please fix a
> current confusion first.

O.K. I understand what you said in the previous review. This patch should fix the bug.

Please note that this patch removes special code for some IMEs which have |IME_PROP_SPECIAL_UI| or |IME_PROP_AT_CARET| in their property. Currently, we don't set to sIMEProperty (Gecko 1.8 already didn't set), therefore, the value is always 0, so, the special code of these IMEs are not run. I think that I should not change the behavior for such IMEs.

FYI: The code is written at:

> 3.117 <tague@netscape.com> 1999-03-19 15:36
> Added support for basic Japanese input on Win32

So, I'm not sure why the code is needed.
Attachment #349549 - Attachment is obsolete: true
Attachment #349623 - Flags: review?(VYV03354)
(Assignee)

Updated

10 years ago
Duplicate of this bug: 438772
Comment on attachment 349623 [details] [diff] [review]
Patch v5.0

Looks good. I also confirmed znabc did no longer crash with this patch.
I'm not an official widget/win32 reviewer. Please ask ere and/or roc for another round of review.
Attachment #349623 - Flags: review?(VYV03354) → review+
(Assignee)

Updated

10 years ago
Attachment #349623 - Flags: review?(emaijala)
Comment on attachment 349623 [details] [diff] [review]
Patch v5.0

Thank you, Kimura-san.

Ere: Would you review this?

The cause of this bug is we are always uses ::ImmGetCompositionStringW. But Intelligent ABC IME (Simplified Chinese IME) which doesn't support Unicode is crash by ::ImmGetCompositionStringW with GCS_COMPCLAUSE. However, of course, we should not kill Unicode support for other IMEs. Therefore, this patch only uses A API when:
* The current keyboard layout's code page is 936 (Simplified Chinese).
* IME doesn't support Unicode, we can check it by ImmGetProperty API.

Unfortunately, we already dropped the keyboard layout code page cache and IME property cache. This patch takes back them to KeyboardLayout.

And we can remove gKeyboardLayout from nsWindow. The same value is cached in KeyboardLayout class too.

Note that this patch removes two codes which checks nsWindow::sIMEProperty. Because sIMEProperty is not needed, the value is always zero. And we should remove the unused variable. Because this patch adds a same meaning variable to KeyboardLayout. So, it can be a cause for confusing other developers.

Comment 64

10 years ago
Comment on attachment 349623 [details] [diff] [review]
Patch v5.0

I wish the unicode support was mandatory in IME..
Nits: Shouldn't it be Intelligent intead of Intellegent? And Simplified instead of Simplefied.

ConvertToANSIString doesn't need to be static.
Attachment #349623 - Flags: review?(emaijala) → review+
Created attachment 350657 [details] [diff] [review]
Patch v5.1
Attachment #349623 - Attachment is obsolete: true
Attachment #350657 - Flags: superreview?(roc)
+      PRBool useA_API = gKbdLayout.GetCodePage() == 936 &&
+                        !(gKbdLayout.GetIMEProperty() & IME_PROP_UNICODE);

If the keyboard layout doesn't have IME_PROP_UNICODE, why would we want to use the W API on non-Chinese code pages?

+        nsCAutoString compANSIStr;
+        if (ConvertToANSIString(*sIMECompUnicode,
+                                gKbdLayout.GetCodePage(), compANSIStr)) {
+          PRUint32 maxlen = compANSIStr.Length();
+          sIMECompClauseArray[0] = 0; // first value must be 0
+          for (PRInt32 i = 1; i < compClauseArrayLength; i++) {
+            PRUint32 len = PR_MIN(sIMECompClauseArray[i], maxlen);
+            sIMECompClauseArray[i] =
+              ::MultiByteToWideChar(gKbdLayout.GetCodePage(), MB_PRECOMPOSED,
+                                    (LPCSTR)compANSIStr.get(), len, NULL, 0);
+          }
+        }

I'm confused. We pass the same string to each call to MultiByteToWideChar --- only the length 'len' changes. Shouldn't we be passing in a different string or offset each time?
(In reply to comment #66)
> Shouldn't we be passing in a different string or
> offset each time?

I'm not sure what you mean. sIMECompClauseArray has offsets of each clauses from start of composition string in ANSI string. But we need to calculate the offsets in Unicode string.
Created attachment 350751 [details] [diff] [review]
Patch v5.2
Attachment #350657 - Attachment is obsolete: true
Attachment #350751 - Flags: superreview?(roc)
Attachment #350657 - Flags: superreview?(roc)
Attachment #350751 - Flags: superreview?(roc) → superreview+
Comment on attachment 350751 [details] [diff] [review]
Patch v5.2

let's fix topcrash.
Attachment #350751 - Flags: approval1.9.1?
landed on trunk.
http://hg.mozilla.org/mozilla-central/rev/3367b9dd7db3
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needs 191 landing]
(Assignee)

Updated

10 years ago
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Updated

10 years ago
Whiteboard: [needs 191 landing] → [c-n: baking for 1.9.1]
Created attachment 351924 [details] [diff] [review]
Patch v5.2 for CVS trunk

This fixes a topcrash bug of Simplified Chinese language users. Only non-Unicode IME handling is affected by this patch, the risk should be low for most people who don't reproduce this bug.
Attachment #351924 - Flags: approval1.9.0.5?
Attachment #351924 - Flags: approval1.9.0.5? → approval1.9.0.6?
Comment on attachment 351924 [details] [diff] [review]
Patch v5.2 for CVS trunk

Moving approval request to 1.9.0.6 since 1.9.0.5 is frozen (i.e., only taking fixes for critical, newly introduced bugs).

That being said, we probably want this to land on 1.9.1 before taking it since that's where the bulk of the nightly users are.
Comment on attachment 350751 [details] [diff] [review]
Patch v5.2

a191=beltzner
Attachment #350751 - Flags: approval1.9.1? → approval1.9.1+
(Assignee)

Updated

10 years ago
Keywords: fixed1.9.1
Whiteboard: [c-n: baking for 1.9.1]
Duplicate of this bug: 470155
Marking as verified1.9.1 based on the lack of crash reports post-beta 2 (and post this patch landing) that match either of the two reported stacks.
Keywords: fixed1.9.1 → verified1.9.1
Comment on attachment 351924 [details] [diff] [review]
Patch v5.2 for CVS trunk

Approved for 1.9.0.6, a=dveditz for release-drivers.
Attachment #351924 - Flags: approval1.9.0.6? → approval1.9.0.6+
(Assignee)

Updated

10 years ago
Keywords: fixed1.9.0.6

Updated

10 years ago
Keywords: verified1.9.1

Comment 78

10 years ago
sorry for the inconvenience.  i meant to detect any bugs before the branch merge that were still RESO fixed, but had a verified1.9.1 keyword next to them.
Keywords: verified1.9.1

Comment 79

10 years ago
Masatoshi or Masayuki or Lingfeng, can you confirm that this crash no longer happens on trunk builds?   I will check to see crash-stats.mozilla.com still shows the stacktrace once that site is up and working again.   Thanks.
 Lingfeng.Guan, can you try to reproduce the bug on a nightly 1.9.0.6 build from ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla1.9.0/? It should be fixed now for the 3.0.6 release of Firefox.

Comment 81

10 years ago
Sure! Sorry for the late response. I intented to do this after the last post, but was dragged out of office by some personal affair. Doing it right now!

Comment 82

10 years ago
I've tested the patch. It's working fine on my WindowsXP with the IMEs caused the bug.
(In reply to comment #79)
> Masatoshi or Masayuki or Lingfeng, can you confirm that this crash no longer
> happens on trunk builds?   I will check to see crash-stats.mozilla.com still
> shows the stacktrace once that site is up and working again.   Thanks.

I cannot reproduce this bug on latest trunk with comment 23's steps.

(In reply to comment #80)
>  Lingfeng.Guan, can you try to reproduce the bug on a nightly 1.9.0.6 build
> from ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla1.9.0/? It should
> be fixed now for the 3.0.6 release of Firefox.

And also I cannot reproduce this bug on the latest 1.9.0.
Keywords: fixed1.9.0.6 → verified1.9.0.6
Thanks, it is all good then. Look for this when we release 3.0.6 to the public.
Keywords: inputmethod
Crash Signature: [@ CalcCharacterPositionAtoW] [@ imm32.dll@0x3e24]
You need to log in before you can comment on or make changes to this bug.