109723 - Mozilla will crash when the XIM server is terminated. - N621, M097 [@ libX11.so.6 - nsIMEGtkIC::ResetIC]

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Tung-Han Hsieh]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.5) Gecko/20011023
BuildID:    Mozilla 0.9.5

Although mozilla depends on gtk/gdk, but I found that mozilla still
want to control the XIM actions directly (it calls gdk_ic_new() to
return the XIC). This is OK, but should be careful. For example, when
the XOpenIM() is called, the XIM client should set a destroy_callback
to Xlib (see, source of gtk/gdk: gdkim.c: gdk_im_real_open()). Then
when the XIM server is suddently unavailable (e.g., it quits), the
Xlib could use the destroy_callback to tell the XIM client about this, then the
XIM client could do something properly and everything goes on happily.

The above machenism worked perfectly in gdk. But mozilla seems not take this
into consideration. When the XIM server terminates, gdk actually gets the
message from the Xlib and handles things properly. But mozilla does not get this
message. It still believe that the XIM server is still there, so it continuously
send/get events to the XIM server (e.g., call the ResetIC()). But the XIM server
is disappeared, so this cause mozilla crash.
   
This is a very common XIM programming problem. To see it, just start a XIM
server (for example, http://xcin.linux.org.tw) and run mozilla, and put the
input cursor into, say, the URL input area of the browser, and type several
characters, and terminate the XIM server, and continue typing. You will see 
mozilla crashed.
   
To fix this problem, I suggest the following: Since gdk could receive the
message from Xlib, we might find a way that when gdk receive such message, it
could tell mozilla via, say, some callbacks. This might involve some API
modification and/or addition, and might need more discussions.
   



Reproducible: Always
Steps to Reproduce:
1.Start an XIM server (e.g., xcin)
2.Start mozilla.
3.Click mouse to put the input focus into any text input area (e.g.,
  editor, URL input area .... etc) and type some characters.
4.Terminate the XIM server.
5.Try to continue the text input in the same area. The mozilla 
  crashed.

Actual Results:  Mozilla crashed.

Expected Results:  Mozilla should not crash. The user should still input into the
text input area without the XIM server (of course, now only
English could be input), without any difficulty.

For more information about the testing XIM server: xcin, see

http://xcin.linux.org.tw/

In the cvs server:

http://xcin.linux.org.tw/download.html#cvs

It contains the most recent development which have been tested with
mozilla. To build the whole xcin package, should have to download
both "libtabe" and "xcin" packages from the cvs server.
Or, if you are more familiar with other XIM servers, say kinput2, it
is also encouraged to test for this bug. I haven't tested all of the
available XIM servers, but I expected that the result will be the same.

Comment 1

[Masaki Katakai]

Tung-Han Hsieh, do you have any suggestion codes?

Comment 2

[Tung-Han Hsieh]

Hi, this is my first time to use bugzilla to discuss. I may make mistakes.
So please inform me if I really do so :-))

Because in theory, gdk is the first place to know that the XIM server is
suddently not available, and I saw that gdk provide a function:

         gboolean gdk_im_ready(void);

which could tell us if the XIM server is ready or not, so I think we may
call this function everytime before we need to access the XIM server 
directly. For example, in nsIMEGtkIC::ResetIC(), just before the call to
XGetICValues() and XSetICValues(), and so does in nsIMEGtkIC::nsIMEGtkIC()
(yes, I saw that nsIMEGtkIC::nsIMEGtkIC() already called it in the very
beginning). I am not sure if this is enough or not, but I could try it
later.

The very crucial point is that the XIM events include two types, the
synchronized events and non-synchronized events. The synchronized ones
should always expect reply from the XIM server. If the XIM server is
suddently not there, the synchronized event requests will cause the XIM
client crash. The XSetICValues() and XGetICValues() both will emit
synchronized events.

Comment 3

[Tung-Han Hsieh]

Hello,

Please try my patch, for mozilla-0.9.5. It contains two parts, to fix
two problems:

1. patch for widget/src/gtk/nsGtkIMEHelper.cpp:

   This is mainly to fix the problem of crash when XIM server is terminated.

==========================================================================
--- nsGtkIMEHelper.cpp.orig     Tue Nov 13 23:58:56 2001
+++ nsGtkIMEHelper.cpp  Wed Nov 14 23:15:07 2001
@@ -1277,6 +1277,9 @@
   if (IsPreeditComposing() == PR_FALSE) {
     return 0;
   }
+  if (!gdk_im_ready()) {
+    return 0;
+  }
 
   if (!mPreedit) {
     mPreedit = new nsIMEPreedit();
@@ -1408,6 +1411,11 @@
 
 void
 nsIMEGtkIC::SetPreeditArea(int aX, int aY, int aW, int aH) {
+  static int paX, paY, paW, paH;
+
+  if (!gdk_im_ready()) return;
+  if (aX!=paX || aY!=paY || aW!=paW || aH!=paH) return;
+
   GdkICAttr *attr = gdk_ic_attr_new();
   if (attr) {
     GdkICAttributesType attrMask = GDK_IC_PREEDIT_AREA;
@@ -1417,6 +1425,11 @@
     attr->preedit_area.height = aH;
     gdk_ic_set_attr((GdkIC*)mIC, attr, attrMask);
     gdk_ic_attr_destroy(attr);
+
+    paX = aX;
+    paY = aY;
+    paW = aW;
+    paH = aH;
   }
 }
===========================================================================

   Note that in this patch I only fix the most crucial parts which are
   directly responsible for the crash. There are still many other similar
   places to be fixed. But before I actually do that, please take a look
   at the above to see if it is acceptable or not.

   The reason to do the above patch is:

   a. Make sure that the XIM server is really there before do any
      communication to the XIM server. For example, before the call of
      XSetICValues(), XGetICValues(). Besides, I found that the call of
      gdk_ic_set_attr(), which will call XSetICValues(), will not check
      the XIM server in advance. If this is the spec of gdk, then we have
      to check before calling gdk_ic_set_attr(), too. It is easy to do the
      checking. Just call gdk_im_ready().

   b. Reduce the frequency of the communication to the XIM server. That is
      what I did in SetPreeditArea(). Although I called gdk_im_ready() in
      the beginning, but it is still very likely that mozilla will crash
      when the XIM server is terminated. This is the race condition. Because
      when the XIM server is suddently terminated, this "event" may be
      queued in the event queue of Xlib. So in fact, the XIM client cannot
      receive this event "immediately". It may need a few "XNextEvent()"
      time then receive it. If before that the XIM client called
      XSetICValues(), then surely it will crash (because the XIM server
      is already terminated). So we must reduce the probability to actually
      call the XSetICValues(), i.e., remove the unnecessary calls.

      For this case, if we use mouse to click the buttom of the XIM server
      window provided by the window manager to terminate it, and if at this
      time the input-focus is the mozilla window. Then mozilla will suddently
      lose focus first, then the buttom is pressed and the XIM server is
      gone, then the mozilla window will gain the focus again. This action
      will happen in a very short time. However, everytime the mozilla
      window gains the focus, it will do something and communicate to the
      XIM server. So it is very probably to crash. So I made the above change
      to avoid this condition, and it works for me.

      You may ask, is there more elegent way to let the XIM client know that
      the XIM server is gone before doing anything, and without any race
      condition? Well, I don't know. And unfortunately there is NO WAY,
      because this could be the WEAKNESS of the XIM protocol. If there is
      any better idea, I would be very glad to know. :-))

2. patch for widget/src/gtk/nsWindow.cpp

===========================================================================
--- nsWindow.cpp.orig   Wed Nov 14 22:35:06 2001
+++ nsWindow.cpp        Wed Nov 14 23:14:49 2001
@@ -3886,6 +3886,8 @@
 nsWindow::IMEUnsetFocusWindow()
 {
   KillICSpotTimer();
+  nsIMEGtkIC *xic = IMEGetInputContext(PR_TRUE);
+  xic->UnsetFocusWindow();
 }
 
 void
============================================================================

   Sorry that I might need to fire another bugzilla number, because this
   patch fixed a unrelated problem. It is better that the XIM client have
   to call XUnsetICFocus() when the its window lose the input focus.
   Otherwise the XIM server may have difficult to trace the preedit status
   between each XIM client. For example, xcin needs this feature. It
   maintains a list of preedit status for each running XIM clients. So
   when any focus changes, it has to know this information.
Please exam my patch and give me comments.


Best Regards,

T.H.Hsieh

Comment 4

[Masaki Katakai]

Thank you for providing the codes.

Actually the codes of gdk_im_ready() will solve the scrash issues
and it can be minimum solution. We should put this codes. However,
the default input style of Mozilla is on-the-spot (callbacks) so
we need to sync the state between Mozilla and XIM server. It means
when XIM server is terminated, we need to reset the state of
input field of Mozilla because Mozilla itself draws string.
There is no problem when we use over-the-spot (position) because
XIM server itself draws the preedit. We need to think about the sync
but yes, anyway, the codes you provided can be a solution.

Regarding nsIMEGtkIC::SetPreeditArea() codes, I also belive it
can much reduce the traffic between Mozilla and XIM server.
I'll try to put the codes to trunk.

About nsWindow::IMEUnsetFocusWindow() changes, I'm not sure
it really needed because gdk_im_begin() in gdk/gtk library
will call XUnsetICFocus() when focus window is changed.
So, when the focus is remaining on Mozilla, we don't need to
call any XUnsetICFocus(). What do you think? The problem would
happen when input focus is changed to other X client on desktop.
It would be reasonable to call XUnsetICFocus() but we need to
consider the performance and other XIM servers.

Comment 5

[Tung-Han Hsieh]

I agree that if the XIM server is suddently unavailable, mozilla have to
reset its input-fields for, say, OnTheSpot input style. I think this is
not hard to do. We can add such a function:

int nsCheckXIMServer( .... )
{
    if (!gdk_im_ready()) {
        // reset mozilla input fields.
        return False;
    }
    else {
        return True;
    }
}
and replace most (but not all, depends on the situation taken into
consideration) of the statements

    if (!gdk_im_ready())  return 0;

But since I am not familiar with the detailed implementation of mozilla,
so I will not touch this part. I believe that you can fill up this part.

Since you agree with my changes in nsIMEGtkIC::SetPreeditArea(), then
there are still many similar places need the same modification. For
example:

nsIMEGtkIC::SetPreeditSpotLocation()
nsIMEGtkIC::SetStatusFont()
nsIMEGtkIC::SetStatusText()
....

I will make a patch for all of these parts. Then you could take a look
and decide if it needs further modification or not.


Finally, for the nsWindow::IMEUnsetFocusWindow() part, I think you cannot
just consider mozilla itself (which means, the user only works in mozilla).
The user may type in mozilla for a while, and then switch to, say, xterm
or rxvt to do some works. But in this case, mozilla does not send the
XUnsetICFocus() to the XIM server. Because both xterm and rxvt do not
depend on gdk, so the XIM server cannot receive the XUnsetICFocus() at
all. Some XIM server need the XUnsetICFocus() to signal it that an XIC is
focused out, so that it have to clean out its data structer and change its
status state. Suppose that the user use the XIM server to type Chinese into
mozilla, and he changes to use xterm. But xterm is not an XIM client, so
naturally the XIM server needs to change its status from the Chinese mode
to the English mode and present the change in its window. If it does not
do this, although everything seems OK, but the user may ask: 'Heh! I am
typing English in xterm, but why the XIM server window showed me that it
is still in Chinese input mode??'

More worse, since the Chinese input methods have many very complicated
preediting status. Some XIM server maintained the preediting status for
each XIC (i.e., the preediting status for each XIC is independent). So
to switch between these XIC requires the correct switching between the
preediting status list. The correct switching releys on the XSetICFocus
and XUnsetICFocus. If the XUnsetICFocus is not called when the XIC is
focused out, these kinds of XIM server will be confused.

You don't have to worry about the duplicating calling the XUnsetICFocus
when you call the gdk_im_end() everytime when any mozilla window is
focused out. Look at the source of gdk carefully, in gdkim.c:

void 
gdk_im_end (void)
{
  if (gdk_xim_ic && gdk_xim_ic->xic)
    {
      XUnsetICFocus (gdk_xim_ic->xic);
      GDK_NOTE (XIM, g_message ("im_end unfocus : %p\n", gdk_xim_ic->xic));
    }
  gdk_xim_ic = NULL;
  gdk_xim_window = NULL;
}

where gdk_xim_ic is a global variable of the current on-focus XIC. If the
XIC is focused out and this function is called, the XUnsetICFocus() will be
called and the gdk_xim_ic will be set to NULL. When another XIC is focused
in and this function is called again, the XUnsetICFocus() will not be
called again due to the NULL value of gdk_xim_ic. So everything is OK and
there is no performance issue here.

Comment 6

[Tung-Han Hsieh]

Hello,

Here is my new patch for nsGtkIMEHelper.cpp

===========================================================================
--- nsGtkIMEHelper.cpp.orig     Tue Nov 13 23:58:56 2001
+++ nsGtkIMEHelper.cpp  Fri Nov 16 00:00:42 2001
@@ -1277,6 +1277,9 @@
   if (IsPreeditComposing() == PR_FALSE) {
     return 0;
   }
+  if (!gdk_im_ready()) {
+    return 0;
+  }
 
   if (!mPreedit) {
     mPreedit = new nsIMEPreedit();
@@ -1352,6 +1355,7 @@
 
 void
 nsIMEGtkIC::SetPreeditFont(GdkFont *aFontset) {
+  if (!gdk_im_ready()) return;
   GdkICAttr *attr = gdk_ic_attr_new();
   if (attr) {
     attr->preedit_fontset = aFontset;
@@ -1384,6 +1388,7 @@
       gStatus->SetFont(aFontset);
     }
   } else {
+    if (!gdk_im_ready()) return;
     GdkICAttr *attr = gdk_ic_attr_new();
     if (attr) {
       attr->preedit_fontset = aFontset;
@@ -1396,6 +1401,11 @@
 
 void
 nsIMEGtkIC::SetPreeditSpotLocation(unsigned long aX, unsigned long aY) {
+  static unsigned long paX, paY;
+
+  if (!gdk_im_ready()) return;
+  if (aX == paX && aY == paY) return;
+
   GdkICAttr *attr = gdk_ic_attr_new();
   if (attr) {
     GdkICAttributesType attrMask = GDK_IC_SPOT_LOCATION;
@@ -1403,11 +1413,19 @@
     attr->spot_location.y = aY;
     gdk_ic_set_attr((GdkIC*)mIC, attr, attrMask);
     gdk_ic_attr_destroy(attr);
+
+    paX = aX;
+    paY = aY;
   }
 }
 
 void
 nsIMEGtkIC::SetPreeditArea(int aX, int aY, int aW, int aH) {
+  static int paX, paY, paW, paH;
+
+  if (!gdk_im_ready()) return;
+  if (aX==paX && aY==paY && aW==paW && aH==paH) return;
+
   GdkICAttr *attr = gdk_ic_attr_new();
   if (attr) {
     GdkICAttributesType attrMask = GDK_IC_PREEDIT_AREA;
@@ -1417,6 +1435,11 @@
     attr->preedit_area.height = aH;
     gdk_ic_set_attr((GdkIC*)mIC, attr, attrMask);
     gdk_ic_attr_destroy(attr);
+
+    paX = aX;
+    paY = aY;
+    paW = aW;
+    paH = aH;
   }
 }
============================================================================

I only search for all the gdk_ic_set_attr() calls and place the check
gdk_im_ready() before it. For the ones which may cause a lot of traffic,
i.e., nsIMEGtkIC::SetPreeditArea() and nsIMEGtkIC::SetPreeditSpotLocation(),
I also use the static variables to save their previous values such that
the traffic could be reduced. Please note that in my previous patch for
the nsIMEGtkIC::SetPreeditArea(), there is a serious mistake and I have
corrected it here.

Hope that the above changes are safe enough. Please give me comments if
any. :-))

Comment 7

[Masaki Katakai]

I've filed new two bug reports for performance and gdk_im_end() issue.
bug 110413 and bug 110411.

Comment 8

[Tung-Han Hsieh]

I found another unnecessary traffic of nsIMEGtkIC::SetPreeditSpotLocation().

Suppose, in the browser, I put my input focus in the URL input field.
Then the SpotLocation will be set to, say, (320,61). Now I use mouse
to click, say, xterm. So mozilla is focused out. And again I focus back
to mozilla, into the same URL input field with the same cursor position.
But I found that the SpotLocation will initially be set to (0,0), and
then (320,61). I think this is non-necessary.

Another place: The Address Book, is even worse. If I open the Address
Book and open the "New Card", then in one of the input area, when I
focus out and focus in again, the XSetICValues() will be called in the
following sequence:

SetPreeditArea: 0 0 584 641 (aX, aY, aW, aH)
SetPreeditSpotLocation: 0 0 (aX, aY)
SetPreeditArea: 0 0 544 499
SetPreeditSpotLocation: 141 57
SetPreeditArea: 0 0 584 641
SetPreeditSpotLocation: 0 0
SetPreeditArea: 0 0 544 499
SetPreeditSpotLocation: 141 57

So, a lot of dummy XSetICValues() calls. In this situation my patch could
not protect mozilla from crash when the XIM server terminated. So it is
suggested to look into these problem in details.

Comment 9

[Masaki Katakai]

Good point! We are well aware that it's Mozilla implementation and limitation.
If you start hacking Mozilla XIM codes, you can find the
limitation first. Unfortunately there is no way to fix this.
Mozilla uses Widget for all components and there is no
difference between normal widget and text widget, which
mean we can not find the widget that is being focused
needs XIM input focus.
nsWindow::SetFocus() is called in many places when
the focus is back to Mozilla but it's not possible
to find it really needs to call XIM SetFocus().

Comment 10

[Masaki Katakai]

Not really sure the bug 110559 is being caused by this problem.
But we should fix this problem. XIM server on Linux actually
sometimes dies.

I'll attach the patch.

Comment 11

[Masaki Katakai]

Attachment: propsed patch

Comment 12

[Masaki Katakai]

I attached the patch and what I made changes are,

gdk_im_ready() checking before
  XGetICValues()
  XSetICValues()
  gdk_ic_get_attr()
  gdk_ic_set_attr()

gdk_ic_get_attr() and gdk_ic_set_attr() call
XGetICValues() and XSetICValues() internally
however they check whether ic is valid or not
by using destroy callback of R6IM feature.
So in R6IM, we don't need to call gdk_im_ready()
for gdk_ic_get_attr() and gdk_ic_set_attr().

Comment 13

[Stuart Parmenter]

Comment on attachment 63780 [details] [diff] [review]
propsed patch

r=pavlov

Comment 14

[Christopher Blizzard (:blizzard)]

Comment on attachment 63780 [details] [diff] [review]
propsed patch

sr=blizzard

Comment 15

[Masaki Katakai]

Thank you very much for reviewing.

Comment 16

[Masaki Katakai]

*** Bug 110559 has been marked as a duplicate of this bug. ***

Comment 17

[Jay Patel [:jay]]

added topcrash keyword and N621, M097 [@ libX11.so.6 - nsIMEGtkIC::ResetIC]
since bug 110559 was marked a dup of this one.  This has been a topcrasher with
Mozilla 0.9.7, so we need to verify this fix in Mozilla 0.9.8.  Here's a stack
from M097 for future reference:

libX11.so.6 + 0x519c7 (0x4041f9c7)  
	 nsIMEGtkIC::ResetIC()  
	 nsWindow::ResetInputState()  
	 nsEditor::ForceCompositionEnd()  
	 nsTextEditorMouseListener::MouseClick()  
	 nsEventListenerManager::HandleEvent()  
	 nsGenericElement::HandleDOMEvent()  
	 nsHTMLInputElement::HandleDOMEvent()  
	 nsGenericElement::HandleDOMEvent()  
	 PresShell::HandleEventInternal()  
	 PresShell::HandleEventWithTarget()  
	 nsEventStateManager::CheckForAndDispatchClick()  
	 nsEventStateManager::PostHandleEvent()  
	 PresShell::HandleEventInternal()  
	 PresShell::HandleEvent()  
	 nsView::HandleEvent()  
	 nsViewManager::DispatchEvent()  
	 HandleEvent()  
	 nsWidget::DispatchEvent()  
	 nsWidget::DispatchWindowEvent()  
	 nsWidget::DispatchMouseEvent()  
	 nsWidget::OnButtonReleaseSignal()  
	 nsWindow::HandleGDKEvent()  
	 dispatch_superwin_event()  
	 handle_gdk_event()   

Comment 18

[Teruko Kobayashi]

Verified as fixed 04-02 Linux trunk build.