Created attachment 17526 [details] [review]
a patch to prevent the loop.

Created attachment 17527 [details] [review]
a correct patch to prevent the loop.

Created attachment 17528 [details] [review]
Also, found a serious typo in the finalized method...

Created attachment 17529 [details] [review]
this should be last spam - a proposed patch for gtk-2-2 and HEAD.

Someone should probably look into why Nautilus is unsetting
the input method *after* destroying the GDK window, since
there is likely is an uninitialized memory reference involved
even with the fix.

Attaching to nautilus with gdb and getting a backtrace would
be useful.

The problem is presumably triggered by:

Fri Jun  6 11:07:33 2003  Owen Taylor  <otaylor@redhat.com>
                                                                     
          
        * gdk/gdkwindow.c (_gdk_window_destroy_hierarchy):
        NULL out private->parent, since after destruction
        it might not be valid any more.
 
As as gtkimcontextxim.c goes.

- while (TRUE)
+  while (TRUE && toplevel_gdk)

the 'TRUE &&'  superfluous

I don't think the shutdown change is right ... the point of:

  while (status_windows)
    status_window_free (status_windows->data);

Is that status_window_free() should remove the status window
from the list. What was the problem with the current code?

Created attachment 17563 [details] [review]
a patch to avoid the crash

> while (TRUE && toplevel)
Shame on me.. Attached a new patch to avoid the crash. Nothing was
wrong in finalized code..
I was confused with status_windows GSList and 
status_window function argument.

Created attachment 17615 [details] [review]
also, found a bug in get_im() when searching im_info in a list.

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

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

Hi,
  the patch 17615 is still not correct. The following modification
should be applied on top of 17615:

--- gtkimcontextxim.c.old       2003-06-23 11:05:43.000000000 +0800
+++ gtkimcontextxim.c   2003-06-23 11:06:36.000000000 +0800
@@ -1312,6 +1312,7 @@
   GtkWidget *status_label;
   GdkScreen *screen;
   GdkWindow *root_window;
+  GdkWindow *parent;
   
   if (!context_xim->client_window)
     return NULL;
@@ -1320,13 +1321,14 @@
   screen = gdk_drawable_get_screen (toplevel_gdk);
   root_window = gdk_screen_get_root_window (screen);
   
-  while (toplevel_gdk)
+  parent = gdk_window_get_parent (toplevel_gdk);
+  while (parent)
     {
-      GdkWindow *parent = gdk_window_get_parent (toplevel_gdk);
       if (parent == root_window)
        break;
       else
        toplevel_gdk = parent;
+      parent = gdk_window_get_parent (toplevel_gdk);
     }
 
   gdk_window_get_user_data (toplevel_gdk, (gpointer *)&toplevel);

getting on cc

These patches can avoid endless roop, but I think these aren't real
solutions, and I also think it's a not GtkIMContext's bug (and,
probably also not Nuatilus's bug). Because, context_xim->client_window
and its parents should exit until exit
gtk_im_conext_ime_set_client_window() as Owen pointed out.

I'm now suspecting unrealize process of some container widgets which
overrides GtkWidgetClass's unrealize method. Please try following test
code without above patches.

Created attachment 17719 [details]
test code for bug115077

I think some container widget which has two or more GdkWindow such as
GtkHandleBox should unrealize by following proccess:

  1. unrealize its all child widgets
  2. destroy own GdkWindow except its toplevel window ((GtkWidget
*)widget->window)
  3. destroy its own toplevel window ((GtkWidget *)widget->window)

But these container seems urnealize by following sequence: 2 -> 1 -> 3.
Because although it simply calls parent's (GtkWidget's) method after
destroying its own GdkWindow, but GtkWidget's method perform 1 and 3
at same function.

As long as I checked, following container widgets seems have same bug.

  GtkHandleBox
  GtkViewport
  GtkTreeView
  GtkTextView

For example, in the case of GtkHandleBox,


static void
gtk_handle_box_unrealize (GtkWidget *widget)
{
  GtkHandleBox *hb = GTK_HANDLE_BOX (widget);
                                                                     
          
  if (GTK_WIDGET_CLASS (parent_class)->unrealize)
    (* GTK_WIDGET_CLASS (parent_class)->unrealize) (widget);
                                                                     
          
  gdk_window_set_user_data (hb->bin_window, NULL);
  gdk_window_destroy (hb->bin_window);
  hb->bin_window = NULL;
  gdk_window_set_user_data (hb->float_window, NULL);
  gdk_window_destroy (hb->float_window);
  hb->float_window = NULL;
}


but, at gtk_handle_box_realize (),


  if (GTK_BIN (hb)->child)
    gtk_widget_set_parent_window (GTK_BIN (hb)->child, hb->bin_window);
                                                                     
          

Sorry, above gtk_handle_box_unrealize () was my patched version
(This patch is still wrong, because bin_window and float_window become
orphan).
Real gtk+-2.2.2's code is following:

static void
gtk_handle_box_unrealize (GtkWidget *widget)
{
  GtkHandleBox *hb = GTK_HANDLE_BOX (widget);
                                                                     
          
  gdk_window_set_user_data (hb->bin_window, NULL);
  gdk_window_destroy (hb->bin_window);
  hb->bin_window = NULL;
  gdk_window_set_user_data (hb->float_window, NULL);
  gdk_window_destroy (hb->float_window);
  hb->float_window = NULL;

  if (GTK_WIDGET_CLASS (parent_class)->unrealize)
    (* GTK_WIDGET_CLASS (parent_class)->unrealize) (widget);
                                                                     
          }
                                                                     
          

Created attachment 17726 [details] [review]
correct unrealizing seaquence of GtkHandleBox

The bug also apear on Galeon.
GtkPlug or GtkSocket also has same bug?

Created attachment 17729 [details]
stack trace of Galeon

The analysis looks correct. Various comments:

 - We should fix the widgets we can for this problem; something
   like GtkViewport is easy. 

 - I think your first simple patch for GtkHandleBox is correct.
   It's OK for the windows to become "orphan", we just don't
   want them to be orphan when the child widgets are unrealized.

 - However, it's going to be hard to fix the problem for
   all widgets, and there are also potentially problems
   for third-party widgets we don't control.

 - I think the GtkHandleBox problems are deeper. When a 
   GtkHandleBox is detached, you can't walk up to 
   a GtkWindow by walking up the GdkWindow hierarchy  
   even before destruction.

   [ Note that GtkHandleBox really should be rewritten
     so that it *has* a separate GtkWindow for the 
     detached widget. Currently, any fix is going
     to result in the status window appearing on
     the parent window, not the tearoff window ]

 - GtkSocket is something like GtkHandleBox - when 
   the parent of the GtkSocket is unrealized, the
   GtkSocket might stay realized but simply detached
   from any toplevel. (It actually *becomes* a toplevel
   in that case)

 - Just checking for NULL as in James Su's patch won't
   work because we need to find the prior toplevel
   to get the status window and hide it.

Thinking about all of the above, I suspect the right thing
to do is to rewrite the code to track the toplevel. The
better way to do it is to track it at the GtkWidget level.

Sketch of how this works:

 - We get the GtkWidget for the client window by
   using gdk_window_get_user_data()

 - When we have the GtkWidget, we connect to 
   ::hierarchy-changed so that we can accurately track
   the toplevel to which the widget belongs. See
   gtk_menu_bar_hierarchy_changed() for how to do this.

Created attachment 17744 [details] [review]
a patch to track client_window changes.

I put the wrong patch. will attach the one I'd want to propose.

Created attachment 17745 [details] [review]
the 2nd "track_client_window change" patch.

Sorry for not responding earlier on this. I wasn't thinking
of adding something extra to the existing code, I was 
suggesting replacing the current mechanism (walking up
the window hierarchy in status_window_get()) with a 
the new mechanism.

I'll attach a patch in a second that substantially
rewrites the handling of the status window. When I 
first wrote the code several years ago, I got the
organization wrong, and we've been suffering from
that ever since.

The summary of my new changes is:

 - Store the current StatusWindow in the
   GtkIMContextXIM structure and vice-versa, so we
   don't have to hunt the window hierarchy on
   cleanup.
 - Use the Gtkidget hierarchy instead of/or as well
   as the GdkWindow hierarchy when finding the toplevel;
   this helps for things like GtkHandlebox
 - Watch GtkWidget::hierarchy_changed to catch
   changes in the toplevel without changes in the
   GdkWindow (reparenting)
 - Never create the GtkWindow for the status window
   unless we have text to display.
 - Various cleanups, add lots of comments.

There is a long block comment in the patch that describes
the handling of the status window in some detail.

Unfortunately, because there are a lot of changes
here, there is a good chance that I introduced new
bugs. 

It seems to work pretty well with some limiting
testing with kinput2; I tried opening and closing
windows, moving them between different displays
switching input methods on the fly. I also tested
it with Takuro Ashie's GtkHandleBOx test.

But I'd certainly appreciate if people could try it
out more thoroughly.

Created attachment 19321 [details] [review]
Patch reworking status window handling

XIM Server cannot get KeyRelease event anymore with the new im-xim
module and this patch.

in gtkimcontextxim.c, try changing:

 	  context_xim->filter_key_release = (mask & KeyReleaseMask);

to:

 	  context_xim->filter_key_release = (mask & KeyReleaseMask) != 0;

That should fix the problem.

I've committed my patch now. Testing still much appreciated.

Mon Aug 18 17:19:12 2003  Owen Taylor  <otaylor@redhat.com>
 
        * modules/input/gtkimcontextxim.[ch]: Substantially
        rework the handling of status windows:
         
         - Store the current StatusWindow in the
           GtkIMContextXIM structure and vice-versa, so we
           don't have to hunt the window hierarchy on
           cleanup.
         - Use the Gtkidget hierarchy instead of/or as well
           as the GdkWindow hierarchy when finding the toplevel;
           this helps for things like GtkHandlebox
         - Watch GtkWidget::hierarchy_changed to catch
           changes in the toplevel without changes in the
           GdkWindow (reparenting)
         - Never create the GtkWindow for the status window
           unless we have text to display.
         - Various cleanups, add lots of comments.
 
        (#115077, much help from Takuro Ashie and Hidetoshi
        Tajima in tracking this down and figuring out a fix.)
 
        * modules/input/gtkimcontextxim.c (gtk_im_context_xim_focus_in):
 
        * modules/input/gtkimcontextxim.c: Track the current
        screen for each toplevel so that we show the status
        window on the right screen. (#116340, James Su)

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

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