279816 - global IME cannot be activated

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jungshik Shin]

On Windows ME (EN), the Korean global IME cannot be activated in suite 1.8a6,
and recent nightlies of firefox (2005-01-17, 01-19, 01-25). 

In firefox 1.0, the Korean globale IME works just fine. I guess the Japanese
global IME and the Chinese(SC/TC) global IME have the same problem (on
non-Japanese/non-Chinese Windows 9x/ME). We need help of CJK users on CJK
Windows 9x/ME as opposed to non-CJK Windows 9x/ME.

I'll try CJK IMEs  on Win 2k later. The severity should be escalated to critical
if it doesn't work on CJK Windows 9x/ME, either. It has to be upgrated to the
blocker if it doesn't work with CJK IMEs on Win 2k/XP.

A related, but very different bug (for non-English keyboards) was reported and
fixed in bug 279105.

Comment 1

[Jungshik Shin]

I went as far back as 2004-05-24 and it still had the problem. It seems like the
regression was introduced sometime very early in the 1.8 development cycle. I
also confirmed that it's not just the Korean global IME but also SC/TC/JA have
the same issue. I wanted to try a couple of nightlies in the early 1.8 cycle,
but archive.mozilla.org wasn't accessible. I'll try later. 

I also asked for help of the Korean mozilla community (because I rarely  boot up
Win ME and I don't have Korean Win ME/9x). The help of Japanese mozilla
community would help, too. 

Fortunately(?), on windows 2k (and the same should be true of Win XP), CJK IMEs
don't have this problem. 

Comment 2

[Jungshik Shin]

In Korean Windows 98/ME, there's no problem. On Windows 2k, CJK IMEs just work
fine regardless of the default system locale. So, this problem is only about
global IMEs on non-CJK Windows. Phew.. this is gonna be very tough to tackle ..
Visual C++ has to be installed on Win ME and a debug build has to be made there.

Comment 3

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

I can reproduce on WinMe-Ja with Global IME.

Comment 4

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

This bug can be reproduced on 1.8a1(20040520).

Comment 5

[Jungshik Shin]

Thanks for testing. We have to go further back along 1.8 development to figure
out the cause. It should be between 2004-04-09 and 2004-05-20. There were so
many check-ins in the period. We need to narrow it down. Masayuki, I don't
usually use Windows ME. Do you have a dedicated machine with Win ME or VMware?
If you can narrow it down, it'd be great.
 

Comment 6

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

This is regression of bug 241993.
2004-05-04 05:00:00 works fine. but 2004-05-04 06:00:00 is not so.
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-05-04+05%3A00%3A00&maxdate=2004-05-04+06%3A00%3A00&cvsroot=%2Fcvsroot

But I don't know the reason of this bug.

Comment 7

[Jungshik Shin]

Thanks for tracking it down. Why did that break? Does our IME code rely on the
Window class name somehow? 

Comment 8

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

Attachment: Patch rv1.0

Comment 9

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

Comment on attachment 173549 [details] [diff] [review]
Patch rv1.0

Oops.. Please wait.

Comment 10

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

Attachment: Patch rv1.1

Comment 11

[Ere Maijala (slow)]

Comment on attachment 173551 [details] [diff] [review]
Patch rv1.1

Is there a reason for changing the registration order (general, ui -> ui,
general)? If there is, please add a comment documenting the reason.

I cannot test it so I think you should ask review from someone who can.

Comment 12

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

Comment on attachment 173551 [details] [diff] [review]
Patch rv1.1

> Is there a reason for changing the registration order (general, ui -> ui,
> general)? If there is, please add a comment documenting the reason.
> 
There is no reason changing the order.
But this order is same as below statement order.
And |generic| is initialized just before using.

Comment 13

[Jungshik Shin]

Thanks for making a patch.

I built mozilla with your patch on Win2k, made a 'zip' file (by running 'make'
in packager directory) and move it to Win ME (en-US version). Global IME(Active
IME) was activated on Mozilla, but once I tried to enter a Korean character,
mozilla crashed. It may as well be my tree, but it's rather up to date and I
didn't make any change in widget-win code other than applying your patch...

I'll be off-line until Friday or Saturday so that somebody else needs to take a
look.

Comment 14

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

I have looked the crash when inputting a hangul charachter.
But it is not this bug.

I try to fix the crash.

Comment 15

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

Attachment: Patch rv.2.0

I fixed the crash. It is regression of 253035.
The Korean global IME always returns 0 when GCS_COMPCLAUSE, GCS_COMPATTR and
GCS_CURSOPOS. Currently code didn't assume this case. 

Simon, can you review it?

Comment 16

[Asa Dotzler [:asa]]

Aaron, can you help out on this regression?

Comment 17

[Aaron Leventhal]

(In reply to comment #16)
> Aaron, can you help out on this regression?

Is this something I need to help with? There is already a patch. Was this bug
just a regression from bug 253035 as the comment says? Or is that fixing a
different problem?

From comment #15
> Created an attachment (id=173613) [edit]
> Patch rv.2.0
>
> I fixed the crash. It is regression of 253035.
> The Korean global IME always returns 0 when GCS_COMPCLAUSE, GCS_COMPATTR and
> GCS_CURSOPOS. Currently code didn't assume this case. 
>
> Simon, can you review it?

Comment 18

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

There are two problems.
1. We cannot activate global IME on Win9x.
2. If it is fixed, mozilla is crashed by using Korean IME.

The "1" is regression of bug 241993. And the "2" is regression of bug 253035.
Both problem is fixed latest patch.

Comment 19

[Aaron Leventhal]

Comment on attachment 173613 [details] [diff] [review]
Patch rv.2.0

Why did you switch the order of the registration of these two, that seems odd:

-    wc.lpszClassName = kWClassNameGeneral;
+    wc.lpszClassName = kWClassNameUI;
     if (!nsToolkit::mRegisterClass(&wc)) {
       nsWindow::sIsRegistered = FALSE;
     }

-    wc.lpszClassName = kWClassNameUI;
-    if (!nsToolkit::mRegisterClass(&wc)) {
+    wc.lpszClassName = kWClassNameGeneral;
+    ATOM general = nsToolkit::mRegisterClass(&wc);
+    if (!general) {
       nsWindow::sIsRegistered = FALSE;
     }

Comment 20

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

see comment 12.

Comment 21

[Aaron Leventhal]

(In reply to comment #20)
> see comment 12.

Okay, thanks -- sorry I missed that :)

I think we need Ere (emaijala) to review this, especially if we still don't hear
from Simon. Has anyone tried reaching him? This is a blocker.

Comment 22

[Ere Maijala (slow)]

See comment 11.

Comment 23

[Aaron Leventhal]

We seem to be at a standstill. Ere says he will review it when we can get
someone to give the fix a thorough testing. Doesn't anyone have IME installed
who can test a patch?

Comment 24

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

Jungshik will return.
He can test it.

Comment 25

[Jungshik Shin]

Sorry for taking long. I tested the patch on Win ME to find it fixes the problem. 

Comment 26

[Jungshik Shin]

Comment on attachment 173613 [details] [diff] [review]
Patch rv.2.0

Give it to Roy. (welcome back !)

Comment 27

[Roy Yokoyama]

Comment on attachment 173613 [details] [diff] [review]
Patch rv.2.0

Jungshik and all: Thanks for the warm welcome :) 
It looks like this bug is a blocker for aviary1.1 which needs an immidiate 
attention.

I wasn't very active on mozilla project for few years and I need 
to get myself up to speed on the i18n Bugs. (Incidently I already 
have over 120 bugs...)

I don't have the testing environment.  Can someone else review the patch?  
I'll keep myself cc'ed.

Comment 28

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

Comment on attachment 173613 [details] [diff] [review]
Patch rv.2.0

Roy:
Hi, welcome back Roy.
I'm very glad for you to return.

Jungshik:
Since nobody(except you) can test the patch, I want your review.

Comment 29

[Jungshik Shin]

Comment on attachment 173613 [details] [diff] [review]
Patch rv.2.0

r=jshin (in that it works and doesn't break anything as far as I can tell)

I don't feel very comfortable because
I don't  understand why the first part of the patch fixes issue #1 (ref.
comment #12: 'there is no reason.......).

As for issue #2 and the second part of the patch, I'm sure that's the right
thing to do.

Comment 30

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

> I don't  understand why the first part of the patch fixes issue #1 (ref.
> comment #12: 'there is no reason.......).

The return value of |nsToolkit::mRegisterClass| is ATOM, not boolean.
The ATOM value is used boolean value too before bug 241993.
> -        nsWindow::sIsRegistered = nsToolkit::mRegisterClass(&wc);
> +        BOOL succeeded =  nsToolkit::mRegisterClass(&wc) != 0;
> +        nsWindow::sIsRegistered = succeeded; 

But bug 241993 changed the boolean value is true or false.
>      BOOL succeeded =  nsToolkit::mRegisterClass(&wc) != 0;
>      nsWindow::sIsRegistered = succeeded;

So, |nsWindow::sIsRegistered| is not ATOM value, it is boolean value.

Therefore I get the ATOM value using |general|, and I'm using the value to
|FilterClientWindows| parameter.
> +    wc.lpszClassName = kWClassNameGeneral;
> +    ATOM general = nsToolkit::mRegisterClass(&wc);
> +    if (!general) {
>        nsWindow::sIsRegistered = FALSE;

> -if (nsToolkit::gAIMMApp)
> -  nsToolkit::gAIMMApp->FilterClientWindows((ATOM*)&nsWindow::sIsRegistered,1);
> +if (nsToolkit::gAIMMApp && general)
> +  nsToolkit::gAIMMApp->FilterClientWindows(&general, 1);

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173613 [details] [diff] [review]
Patch rv.2.0

I can't meaningfully sr this code.  Someone marginally familiar with the Win32
api would be a much better choice... Perhaps jag?  Or sspitzer?  Or darin?  Or
jst?

Comment 32

[Jungshik Shin]

Comment on attachment 173613 [details] [diff] [review]
Patch rv.2.0

re: comment #31
Perhaps, Neil can sr this. 

To bz:
Actually, if you look at the surrounding code (not in the patch), the second
part of the patch is just protecting array-index from going out-of-bound (which
caused the crash I reported). And the first part is about fixing an invalid
casting (plus passing the correct ATOM* to a Win32 API FilterXXXX)

(In reply to comment #30)
Thanks for the explanation. I was so distracted by the shuffling that I missed
the obvious.
> > -  nsToolkit::gAIMMApp->FilterClientWindows((ATOM*)&nsWindow::sIsRegistered,1);
> > +if (nsToolkit::gAIMMApp && general)
> > +  nsToolkit::gAIMMApp->FilterClientWindows(&general, 1);

In short, the casting of |BOOL*| to |ATOM*| (|(ATOM*)&nsWindow::sIsRegistered|)
is  wrong.

Comment 33

[[e-mail]]

Comment on attachment 173613 [details] [diff] [review]
Patch rv.2.0

That fallout from bug 9449 was just waiting to happen...

It occurred to me that the variable name "general" sounds a bit vague to me,
and I thought that perhaps it should be "generalClass" or "generalClassAtom".

However, I'm concerned that you should be filtering all those registered
classes, not just the last one. Is that the only one of our classes that takes
the focus?

Comment 34

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

> However, I'm concerned that you should be filtering all those registered
> classes, not just the last one. Is that the only one of our classes that takes
> the focus?

I don't know that the only one of our classes that takes the focus.
But there is no problem of the patch.

Comment 35

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

Attachment: Patch rv2.1

Comment 36

[[e-mail]]

Comment on attachment 174996 [details] [diff] [review]
Patch rv2.1

Ah, it looks like nsWindow::SetFocus only sets focus to the top level HWND so
only that one class needs IME activated on it.

Comment 37

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

Jungshik:

Could you check-in the patch?

Comment 38

[Jungshik Shin]

landed on the trunk