Closed Bug 400249 Opened 17 years ago Closed 17 years ago

crash on shutdown and before restart (gdk_display_close) with gtk iiim module

Categories

(Core :: Widget: Gtk, defect)

x86
OpenSolaris
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: ginnchen+exoracle, Assigned: ginnchen+exoracle)

References

Details

(Keywords: crash)

Attachments

(1 file, 4 obsolete files)

Reproduce with GTK_IM_MODULE=iiim in the environment.

See also bug 398512.

It's a bug of iiim.
I think we should work around it.

I should file a bug to iiim.
But I've trouble with http://www.openi18n.org/bugzilla/ right now.
Attached patch patch based on bug 398512 (obsolete) — Splinter Review
Assignee: nobody → ginn.chen
Status: NEW → ASSIGNED
Attachment #285329 - Flags: review?(roc)
Thanks for submitting a patch.  I think a work-around would be good.

> // Work around gtk bug http://bugzilla.gnome.org/show_bug.cgi?id=483223:
>+// (and same bug of gtk iiim)
> // The gtk xim module registers closed signal handlers on the display, but
> // there are two problems:
> //  * The signal handlers are not disconnected when the module is unloaded.
> //  * When the signal handler is run (with the module loaded) it tries
> //    XFree (and fails) on a pointer that did not come from Xmalloc.

It's worth updating the comment to make it clear that only the first problem
applies to the iiim module, as it influences how we solve the problem.

> +    struct GtkIMContextIIIM
> +    {
> +        GtkIMContext parent;
> +        GtkIMContext* slave;
> +        gpointer private_data;
> +        // ... other fields

This looks like the struct for GtkIMMulticontext, but
the header file I have from im-sdk-r12_1-svn2002 has:

struct _GtkIMContextIIIM
{
  GtkIMContext object;

  GtkIIIMInfo *iiim_info;
  GdkWindow *client_window;
  GtkWidget *client_widget;

so I fear private_data is client_window not the intended iiim_info and so
the handlers being removed are not the intended.

But we probably don't need to remove the handlers anyway.
The handler for iiim looks safe to call provided the module is not unloaded,
so there are two possible solutions here:

1) disconnect the handlers, or
2) prevent the module from being unloaded.

If one is done the other is unnecessary.

There are advantages with each, but I prefer (2) because it doesn't require any
assumptions about the structures used by the module.  Making these assumptions
about the xim module was just acceptable because we can check that the module
is an older version for which we have seen the structure used in the source
code - we don't need to make assumptions about newer versions of xim (and also
because I couldn't think of any other workaround).  With iiim we need to keep
the workaround for future versions (because we don't know module version) but
the structures used could change in the future.

Preventing the module from being unloaded merely requires the g_type_class_ref.  Hopefully the ref count is a 32-bit int, so we're unlikely to overflow by opening and closing 2^31 windows.  Maybe its more elegant to store the result of
g_type_class_ref in a static variable and only add a ref when we don't already
have one, but I don't care too much either way.

And if we don't actually disconnect the handlers for iiim, then I guess the function could be called something like workaround_gtk_im_display_closed (I'm open to better suggestions on that).
Comment on attachment 285329 [details] [diff] [review]
patch based on bug 398512

Address Karl's comments, then rerequest review from him and sr from me. Thanks.
Attachment #285329 - Flags: review?(roc)
Attached patch patch v2 (obsolete) — Splinter Review
Thanks for your help!
Attachment #285329 - Attachment is obsolete: true
Attachment #285453 - Flags: review?(mozbugz)
Comment on attachment 285453 [details] [diff] [review]
patch v2

This is doing the right thing for the iiim module now, but the changes I
requested in comment 2 I meant for iiim only.  xim does need the signal
handlers disconnected, so the code should function more or less the same for
xim.

+// The gtk im module registers closed signal handlers on the display, but:

s/im module registers/xim and iiim modules register/
Other im modules don't register such signal handlers.

+// To prevent the signal handler from being run, we use a static variable
+// to hold ref of the GtkIMContext clasee

The signal handler will be run for iiim.  The ref merely ensures the module is
still loaded so the code is still available to run the handler.

+        static gpointer gtk_im_context_class = g_type_class_ref(slaveType);

That's concise :) (I assume that means g_type_class_ref is only run once.)
I don't know that we can preclude both xim and xiim being loaded though, so
the different modules (and g_type_classes) would each need their own static
gpointer.
Attachment #285453 - Flags: review?(mozbugz)
Attached patch patch v3 (obsolete) — Splinter Review
Thank you!
Attachment #285453 - Attachment is obsolete: true
Attachment #285715 - Flags: review?(mozbugz)
Comment on attachment 285715 [details] [diff] [review]
patch v3

Thanks for making the changes.  This should function correctly.  Just a few
small things:

+// The gtk xim and iiim module registers closed signal handlers on the

s/module registers/modules register/

+        guint disconnected =

+        if (disconnected) {

The conditional test and the variable shouldn't be needed now with the static
gpointer (and the version check makes sure we don't do this when unnecessary).

+            // opens (and doesn't close) new XOpenIM connections.
+            static gpointer gtk_im_context_class =

_im_ -> _xim_

+    if (strcmp(im_type_name, "GtkIMContextIIIM") == 0) {

I'd prefer an "else if" here to clarify that in no cases will both blocks be
executed, and so ...

+    if ((strcmp(im_type_name, "GtkIMContextXIM") == 0) &&
+         (gtk_check_version(2,12,1) != NULL)) { // need gtk bug workaround

could become:

    if (strcmp(im_type_name, "GtkIMContextXIM") == 0) {
        if (gtk_check_version(2,12,1) == NULL) // gtk bug has been fixed
            return;

r=me with these changes.
Request sr from roc when you've made these changes.
Attachment #285715 - Flags: review?(mozbugz) → review+
Attachment #285715 - Attachment is obsolete: true
Attachment #285830 - Flags: superreview?(roc)
Comment on attachment 285830 [details] [diff] [review]
patch v4 addressed Karl's comments

Is "closed signal handlers" a technical term? I suspect you mean "display closed" - "signal handlers"

Google says the only instance of this string is from the previous patch to this file. As such, I claim the string is almost certainly wrong.

FWIW, I believe it should be "GTK XIM" and "GTK IIIM" (note the case) - Google agrees.

+// To prevent the module 

Which module is "the module" here? If you mean "the modules" or both modules or something, please write that.

And why do you suddenly start talking about "gtk xim" here:
+// For gtk xim module, 

+    if (strcmp(im_type_name, "GtkIMContextXIM") == 0) {
+        if (gtk_check_version(2,12,1)

I believe style says to use spaces between arguments, unless gtk_check_version is really special. (Yes, I can see you're moving it, but that doesn't mean the code shouldn't be fixed.)

Do we really want to have our private GtkIMContextXIM
share the name with the normal structure? (not sure, just asking)

...prevent the xim module being...

please fix this to say "from being" (x2) if you're going to fix most of the other lines :|.

and change "reloaded:" to "reloaded, because"

please add -p to the diff flags you use (yes, I should use Bugzilla's patch viewer, but NoScript blocked that :()
Attachment #285830 - Flags: review?(mozbugz)
Comment on attachment 285830 [details] [diff] [review]
patch v4 addressed Karl's comments

+// The gtk xim and iiim module register closed signal handlers on the display,

module -> modules

(In reply to comment #9)
> (From update of attachment 285830 [details] [diff] [review])
> Is "closed signal handlers" a technical term? I suspect you mean "display
> closed" - "signal handlers"
> 
> Google says the only instance of this string is from the previous patch to this
> file. As such, I claim the string is almost certainly wrong.

The "signal handlers" are called when the "closed" signal is emitted on the GdkDisplay.  I'm open to suggestions for better ways to express this.  Perhaps the following?:

  handlers for the "closed" signal on the display

> FWIW, I believe it should be "GTK XIM" and "GTK IIIM" (note the case) - Google
> agrees.

I understand GTK+ is more correct.
XIM may be appropriate for the Xlib input method but gtk.immodules actually calls the GTK+ module xim.  I don't know about IIIM/iiim.

> 
> +// To prevent the module 
> 
> Which module is "the module" here? If you mean "the modules" or both modules or
> something, please write that.

"these modules" would be good.

> 
> And why do you suddenly start talking about "gtk xim" here:
> +// For gtk xim module, 

That clause is there because the following clause applies only to the xim GTK+ module, not iiim/IIIM.

timeless: is there a clearer way to put that?

> 
> +    if (strcmp(im_type_name, "GtkIMContextXIM") == 0) {
> +        if (gtk_check_version(2,12,1) [// gtk bug had been fixed]
> 
> I believe style says to use spaces between arguments, unless gtk_check_version
> is really special. (Yes, I can see you're moving it, but that doesn't mean the
> code shouldn't be fixed.)

I don't care here: spaces are more consistent with the file style but I don't believe they would help the readability. 

I do prefer "has been" to "had been" though.

> Do we really want to have our private GtkIMContextXIM
> share the name with the normal structure? (not sure, just asking)

I wondered about that.  The typedef's definition is not available in any installed header files, and the scope of this struct is the block, so I didn't think it mattered.
However, I'm open to suggestions for a better name that means OurPeekAtGtkIMContextXIM.

> 
> ...prevent the xim module being...
> 
> please fix this to say "from being" (x2) if you're going to fix most of the
> other lines :|.

My little Oxford dictionary says this:

  "The use of _prevent_ without _from_ as in _She prevented me going_ is
   informal.  An acceptable further alternative is _She prevented my going_."

So, if we'd like to be formal, we could either add "from" or make it "prevent unloading and reloading of the module".  The "from" is fine.

> 
> and change "reloaded:" to "reloaded, because"

I can't see anything grammatically incorrect about the colon
(see rule 4 at http://www.grammarbook.com/punctuation/colons.asp),
and I think using a conjunction would make a long sentence that is harder to understand.
Attachment #285830 - Flags: review?(mozbugz)
Attached patch patch v5Splinter Review
Attachment #285830 - Attachment is obsolete: true
Attachment #285870 - Flags: superreview?(roc)
Attachment #285830 - Flags: superreview?(roc)
Attachment #285870 - Flags: superreview?(roc) → superreview+
Comment on attachment 285870 [details] [diff] [review]
patch v5

Drivers, please consider this patch for beta, as I understand this crash affects most Solaris users.

Risk should be low given we've already done something similar for the xim module.
Attachment #285870 - Flags: approvalM9?
Comment on attachment 285870 [details] [diff] [review]
patch v5

a=endgame drivers for M9
Attachment #285870 - Flags: approvalM9?
Attachment #285870 - Flags: approvalM9+
Attachment #285870 - Flags: approval1.9+
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: