global IME cannot be activated

RESOLVED FIXED in mozilla1.8beta1

Status

()

Core
Widget: Win32
--
major
RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: Jungshik Shin, Assigned: masayuki)

Tracking

({inputmethod, intl, regression})

Trunk
mozilla1.8beta1
x86
Windows ME
inputmethod, intl, regression
Points:
---
Bug Flags:
blocking1.8b2 +
blocking-aviary1.5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

13 years ago
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.
(Reporter)

Comment 1

13 years ago
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. 
Summary: global IME cannot be activated → global IME cannot be activated
(Reporter)

Comment 2

13 years ago
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.
Keywords: helpwanted, regression
I can reproduce on WinMe-Ja with Global IME.
This bug can be reproduced on 1.8a1(20040520).
(Reporter)

Comment 5

13 years ago
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.
 
Flags: blocking1.8b?
Flags: blocking-aviary1.1?
(Reporter)

Comment 7

13 years ago
Thanks for tracking it down. Why did that break? Does our IME code rely on the
Window class name somehow? 
Created attachment 173549 [details] [diff] [review]
Patch rv1.0
Assignee: win32 → masayuki
Status: NEW → ASSIGNED
Attachment #173549 - Flags: review?(emaijala)
(Assignee)

Updated

13 years ago
Keywords: helpwanted
Target Milestone: --- → mozilla1.8beta
Comment on attachment 173549 [details] [diff] [review]
Patch rv1.0

Oops.. Please wait.
Attachment #173549 - Attachment is obsolete: true
Attachment #173549 - Flags: review?(emaijala)
Created attachment 173551 [details] [diff] [review]
Patch rv1.1
Attachment #173551 - Flags: review?(emaijala)

Comment 11

13 years ago
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 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.
Attachment #173551 - Flags: review?(emaijala) → review?(jshin1987)
(Reporter)

Comment 13

13 years ago
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.

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

I try to fix the crash.
(Assignee)

Updated

13 years ago
Attachment #173551 - Attachment is obsolete: true
Attachment #173551 - Flags: review?(jshin1987)
Created attachment 173613 [details] [diff] [review]
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?
(Assignee)

Updated

13 years ago
Attachment #173613 - Flags: review?(smontagu)

Comment 16

13 years ago
Aaron, can you help out on this regression?
Flags: blocking1.8b?
Flags: blocking1.8b+
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.1+

Comment 17

13 years ago
(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?

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

13 years ago
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 21

13 years ago
(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

13 years ago
See comment 11.

Comment 23

13 years ago
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?
Jungshik will return.
He can test it.
(Reporter)

Comment 25

13 years ago
Sorry for taking long. I tested the patch on Win ME to find it fixes the problem. 
(Assignee)

Updated

13 years ago
Attachment #173613 - Flags: review?(smontagu) → review?(jshin1987)
(Reporter)

Comment 26

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

Give it to Roy. (welcome back !)
Attachment #173613 - Flags: review?(jshin1987) → review?(tetsuroy)

Updated

13 years ago
Flags: blocking1.8b+ → blocking1.8b2+

Comment 27

13 years ago
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.
Attachment #173613 - Flags: review?(tetsuroy)
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.
Attachment #173613 - Flags: review?(jshin1987)
(Reporter)

Comment 29

13 years ago
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.
Attachment #173613 - Flags: review?(jshin1987) → review+
> 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);
(Assignee)

Updated

13 years ago
Attachment #173613 - Flags: superreview?(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?
Attachment #173613 - Flags: superreview?(bzbarsky)
(Reporter)

Comment 32

13 years ago
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.
Attachment #173613 - Flags: superreview?(neil.parkwaycc.co.uk)

Comment 33

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

Updated

13 years ago
Attachment #173613 - Flags: superreview?(neil.parkwaycc.co.uk)
Created attachment 174996 [details] [diff] [review]
Patch rv2.1
Attachment #173613 - Attachment is obsolete: true
Attachment #174996 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #174996 - Flags: review+

Comment 36

13 years ago
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.
Attachment #174996 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Jungshik:

Could you check-in the patch?
(Reporter)

Comment 38

13 years ago
landed on the trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Keywords: inputmethod
You need to log in before you can comment on or make changes to this bug.