Issue 6573051: Fix SIGABRT by finalized IBusIMContext during _request_surrounding_text (Closed)
|
Created:
11 years, 9 months ago by fujiwara Modified:
11 years, 8 months ago Reviewers:
shawn.p.huang CC:
shawn.p.huang_gmail.com Base URL:
git@github.com:ibus/ibus.git@master Visibility:
Public. |
DescriptionFix SIGABRT by finalized IBusIMContext during _request_surrounding_text
BUG=RH#859879
TEST=Manually
Patch Set 1 #
Total comments: 2
Patch Set 2 : Updated with the more investigation. #
Total comments: 4
Patch Set 3 : Updated with message #6, #7. #
Total comments: 2
Patch Set 4 : Updated with message #10 #MessagesTotal messages: 11
I went through the reproduce steps in bugzilla. I don't understand why the context is finalized. I think it is better to investigate more on it. https://codereview.appspot.com/6573051/diff/1/client/gtk2/ibusimcontext.c File client/gtk2/ibusimcontext.c (right): https://codereview.appspot.com/6573051/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:385: /* _request_surrounding_text spends time */ The comment is not clear. https://codereview.appspot.com/6573051/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:386: if (!IBUS_IS_INPUT_CONTEXT (ibuscontext)) { I think it is better to use g_object_add_weak_pointer/g_object_remove_weak_pointer to track the context.
Sign in to reply to this message.
On 2012/09/26 15:23:53, Peng wrote:
> I went through the reproduce steps in bugzilla. I don't understand why the
> context is finalized. I think it is better to investigate more on it.
>
_request_surrounding_text() called ibus_im_context_finalize():
#0 ibus_im_context_finalize (obj=0x2aa90b0) at ibusimcontext.c:647
#1 0x0000003c82a148cb in g_object_unref () from /lib64/libgobject-2.0.so.0
#2 0x0000003c82a34b13 in g_value_unset () from /lib64/libgobject-2.0.so.0
#3 0x0000003c82a297a1 in g_signal_emit_valist ()
from /lib64/libgobject-2.0.so.0
#4 0x0000003c82a29942 in g_signal_emit () from /lib64/libgobject-2.0.so.0
#5 0x00007f87eae1b094 in _request_surrounding_text (context=0x2aa90b0)
at ibusimcontext.c:278
#6 0x00007f87eae1b4b8 in _key_snooper_cb (widget=0x25a6090, event=0x296a070,
user_data=0x0) at ibusimcontext.c:380
#7 0x0000003c84da20e4 in gtk_main_do_event () from /lib64/libgtk-3.so.0
#8 0x0000003c83a4e1d2 in gdk_event_source_dispatch ()
from /lib64/libgdk-3.so.0
#9 0x0000003c82251722 in g_main_dispatch (context=0x245f910) at gmain.c:2707
In case the return value in the signal function is not void, it seems the signal
function calls g_object_unref.
In my test, the GObject->ref_count is normally greater than 1 in the non-void
signal function.
But GOIbject->ref_count is 1 in this bug, the finalize function is called.
I don't know where GObject->ref_count is increased.
I'm not sure but currently gtk_key_snooper_install is a deprecated function.
I can reproduce this problem in both gnome-shell and non-gnome-shell.
> https://codereview.appspot.com/6573051/diff/1/client/gtk2/ibusimcontext.c
> File client/gtk2/ibusimcontext.c (right):
>
>
https://codereview.appspot.com/6573051/diff/1/client/gtk2/ibusimcontext.c#new...
> client/gtk2/ibusimcontext.c:385: /* _request_surrounding_text spends time */
> The comment is not clear.
>
>
https://codereview.appspot.com/6573051/diff/1/client/gtk2/ibusimcontext.c#new...
> client/gtk2/ibusimcontext.c:386: if (!IBUS_IS_INPUT_CONTEXT (ibuscontext)) {
> I think it is better to use
> g_object_add_weak_pointer/g_object_remove_weak_pointer to track the context.
I don't understand your suggestion. I will ask you later.
Sign in to reply to this message.
On 2012/09/27 11:31:27, fujiwara wrote:
> On 2012/09/26 15:23:53, Peng wrote:
> > I went through the reproduce steps in bugzilla. I don't understand why the
> > context is finalized. I think it is better to investigate more on it.
> >
>
> _request_surrounding_text() called ibus_im_context_finalize():
>
> #0 ibus_im_context_finalize (obj=0x2aa90b0) at ibusimcontext.c:647
> #1 0x0000003c82a148cb in g_object_unref () from /lib64/libgobject-2.0.so.0
> #2 0x0000003c82a34b13 in g_value_unset () from /lib64/libgobject-2.0.so.0
> #3 0x0000003c82a297a1 in g_signal_emit_valist ()
> from /lib64/libgobject-2.0.so.0
> #4 0x0000003c82a29942 in g_signal_emit () from /lib64/libgobject-2.0.so.0
> #5 0x00007f87eae1b094 in _request_surrounding_text (context=0x2aa90b0)
> at ibusimcontext.c:278
> #6 0x00007f87eae1b4b8 in _key_snooper_cb (widget=0x25a6090, event=0x296a070,
> user_data=0x0) at ibusimcontext.c:380
> #7 0x0000003c84da20e4 in gtk_main_do_event () from /lib64/libgtk-3.so.0
> #8 0x0000003c83a4e1d2 in gdk_event_source_dispatch ()
> from /lib64/libgdk-3.so.0
> #9 0x0000003c82251722 in g_main_dispatch (context=0x245f910) at gmain.c:2707
>
OK, I found the g_object_unref is pared with g_object_ref correctly in
g_signal_emit_valist.
void
g_signal_emit_valist (gpointer instance,
guint signal_id,
GQuark detail,
va_list var_args)
{
...
g_value_set_instance (instance_and_params, instance);
...
signal_emit_unlocked_R (node, detail, instance, &return_value,
instance_and_params);
...
g_value_unset (instance_and_params);
}
g_value_set_instance() can call the ref.
The problem is that gtk_im_multicontext_get_slave() can call unref in during
"retrieve-surrounding" signal because priv->context_id is not the latest than
global_context_id in GtkIMMulticontext and then g_signal_emit_valist() can call
ibus_im_context_finalize() finally.
#0 gtk_im_multicontext_set_slave (multicontext=0x1e8c330 [GtkIMMulticontext],
slave=0x0, finalizing=0) at gtkimmulticontext.c:211
#1 0x00007f5f0b428f9b in gtk_im_multicontext_get_slave (multicontext=
0x1e8c330 [GtkIMMulticontext]) at gtkimmulticontext.c:293
#2 0x00007f5f0b429532 in gtk_im_multicontext_set_surrounding (context=
0x1e8c330 [GtkIMMulticontext], text=0x200c0b0 "", len=0, cursor_index=0)
at gtkimmulticontext.c:519
#3 0x00007f5f0b425333 in gtk_im_context_set_surrounding (context=
0x1e8c330 [GtkIMMulticontext], text=0x200c0b0 "", len=0, cursor_index=0)
at gtkimcontext.c:663
#4 0x00007f5f0b55bc06 in gtk_text_view_retrieve_surrounding_handler (context=
0x1e8c330 [GtkIMMulticontext], text_view=0x1eb40d0 [GeditView])
at gtktextview.c:8146
#5 0x00007f5f0b44cc09 in _gtk_marshal_BOOLEAN__VOID (closure=0x1eb3700,
return_value=0x7fff1f8300c0, n_param_values=1, param_values=
0x7fff1f8301f0, invocation_hint=0x7fff1f8300f0, marshal_data=0x0)
at gtkmarshalers.c:2003
#6 0x00007f5f0ad923db in g_closure_invoke (closure=0x1eb3700, return_value=
0x7fff1f8300c0, n_param_values=1, param_values=0x7fff1f8301f0,
invocation_hint=0x7fff1f8300f0) at gclosure.c:788
#7 0x00007f5f0adb02c3 in signal_emit_unlocked_R (node=0x1e21000, detail=0,
instance=0x1e8c330, emission_return=0x7fff1f8302c0, instance_and_params=
0x7fff1f8301f0) at gsignal.c:3591
#8 0x00007f5f0adaf24a in g_signal_emit_valist (instance=0x1e8c330, signal_id=
372, detail=0, var_args=0x7fff1f8304f0) at gsignal.c:3326
#9 0x00007f5f0adafab8 in g_signal_emit_by_name (instance=0x1e8c330,
detailed_signal=0x7f5f0b6fe602 "retrieve-surrounding") at gsignal.c:3425
#10 0x00007f5f0b429631 in gtk_im_multicontext_retrieve_surrounding_cb (slave=
0x21126b0 [IBusIMContext], multicontext=0x1e8c330 [GtkIMMulticontext])
at gtkimmulticontext.c:560
#11 0x00007f5f0b44cc09 in _gtk_marshal_BOOLEAN__VOID (closure=0x2119690,
return_value=0x7fff1f8307e0, n_param_values=1, param_values=
0x7fff1f830910, invocation_hint=0x7fff1f830810, marshal_data=0x0)
at gtkmarshalers.c:2003
#12 0x00007f5f0ad923db in g_closure_invoke (closure=0x2119690, return_value=
0x7fff1f8307e0, n_param_values=1, param_values=0x7fff1f830910,
invocation_hint=0x7fff1f830810) at gclosure.c:788
#13 0x00007f5f0adb02c3 in signal_emit_unlocked_R (node=0x1e21000, detail=0,
instance=0x21126b0, emission_return=0x7fff1f8309e0, instance_and_params=
0x7fff1f830910) at gsignal.c:3591
#14 0x00007f5f0adaf24a in g_signal_emit_valist (instance=0x21126b0, signal_id=
372, detail=0, var_args=0x7fff1f830c18) at gsignal.c:3326
#15 0x00007f5f0adaf95a in g_signal_emit (instance=0x21126b0, signal_id=372,
detail=0) at gsignal.c:3388
#16 0x00007f5eeda90094 in _request_surrounding_text (context=
0x21126b0 [IBusIMContext]) at ibusimcontext.c:278
#17 0x00007f5eeda904da in _key_snooper_cb (widget=0x1b7a020 [GeditWindow],
event=0x1dc56c0, user_data=0x0) at ibusimcontext.c:381
#18 0x00007f5f0b448a45 in gtk_invoke_key_snoopers (grab_widget=
0x1b7a020 [GeditWindow], event=0x1dc56c0) at gtkmain.c:2259
As I think the root cause is that priv->context_id and get_effective_context_id
(multicontext) are not sync, but basically I agree to ref _focus_im_context
during the snooper to avoid the unexpected unref.
Updated the patch.
Sign in to reply to this message.
On 2012/10/02 03:15:40, fujiwara wrote:
> On 2012/09/27 11:31:27, fujiwara wrote:
> > On 2012/09/26 15:23:53, Peng wrote:
> > > I went through the reproduce steps in bugzilla. I don't understand why the
> > > context is finalized. I think it is better to investigate more on it.
> > >
> >
> > _request_surrounding_text() called ibus_im_context_finalize():
> >
> > #0 ibus_im_context_finalize (obj=0x2aa90b0) at ibusimcontext.c:647
> > #1 0x0000003c82a148cb in g_object_unref () from /lib64/libgobject-2.0.so.0
> > #2 0x0000003c82a34b13 in g_value_unset () from /lib64/libgobject-2.0.so.0
> > #3 0x0000003c82a297a1 in g_signal_emit_valist ()
> > from /lib64/libgobject-2.0.so.0
> > #4 0x0000003c82a29942 in g_signal_emit () from /lib64/libgobject-2.0.so.0
> > #5 0x00007f87eae1b094 in _request_surrounding_text (context=0x2aa90b0)
> > at ibusimcontext.c:278
> > #6 0x00007f87eae1b4b8 in _key_snooper_cb (widget=0x25a6090,
event=0x296a070,
> > user_data=0x0) at ibusimcontext.c:380
> > #7 0x0000003c84da20e4 in gtk_main_do_event () from /lib64/libgtk-3.so.0
> > #8 0x0000003c83a4e1d2 in gdk_event_source_dispatch ()
> > from /lib64/libgdk-3.so.0
> > #9 0x0000003c82251722 in g_main_dispatch (context=0x245f910) at
gmain.c:2707
> >
>
> OK, I found the g_object_unref is pared with g_object_ref correctly in
> g_signal_emit_valist.
>
> void
> g_signal_emit_valist (gpointer instance,
> guint signal_id,
> GQuark detail,
> va_list var_args)
> {
> ...
> g_value_set_instance (instance_and_params, instance);
>
> ...
> signal_emit_unlocked_R (node, detail, instance, &return_value,
> instance_and_params);
> ...
> g_value_unset (instance_and_params);
> }
>
> g_value_set_instance() can call the ref.
>
> The problem is that gtk_im_multicontext_get_slave() can call unref in during
> "retrieve-surrounding" signal because priv->context_id is not the latest than
> global_context_id in GtkIMMulticontext and then g_signal_emit_valist() can
call
> ibus_im_context_finalize() finally.
>
> #0 gtk_im_multicontext_set_slave (multicontext=0x1e8c330 [GtkIMMulticontext],
> slave=0x0, finalizing=0) at gtkimmulticontext.c:211
> #1 0x00007f5f0b428f9b in gtk_im_multicontext_get_slave (multicontext=
> 0x1e8c330 [GtkIMMulticontext]) at gtkimmulticontext.c:293
> #2 0x00007f5f0b429532 in gtk_im_multicontext_set_surrounding (context=
> 0x1e8c330 [GtkIMMulticontext], text=0x200c0b0 "", len=0, cursor_index=0)
> at gtkimmulticontext.c:519
> #3 0x00007f5f0b425333 in gtk_im_context_set_surrounding (context=
> 0x1e8c330 [GtkIMMulticontext], text=0x200c0b0 "", len=0, cursor_index=0)
> at gtkimcontext.c:663
> #4 0x00007f5f0b55bc06 in gtk_text_view_retrieve_surrounding_handler (context=
> 0x1e8c330 [GtkIMMulticontext], text_view=0x1eb40d0 [GeditView])
> at gtktextview.c:8146
> #5 0x00007f5f0b44cc09 in _gtk_marshal_BOOLEAN__VOID (closure=0x1eb3700,
> return_value=0x7fff1f8300c0, n_param_values=1, param_values=
> 0x7fff1f8301f0, invocation_hint=0x7fff1f8300f0, marshal_data=0x0)
> at gtkmarshalers.c:2003
> #6 0x00007f5f0ad923db in g_closure_invoke (closure=0x1eb3700, return_value=
> 0x7fff1f8300c0, n_param_values=1, param_values=0x7fff1f8301f0,
> invocation_hint=0x7fff1f8300f0) at gclosure.c:788
> #7 0x00007f5f0adb02c3 in signal_emit_unlocked_R (node=0x1e21000, detail=0,
> instance=0x1e8c330, emission_return=0x7fff1f8302c0, instance_and_params=
> 0x7fff1f8301f0) at gsignal.c:3591
> #8 0x00007f5f0adaf24a in g_signal_emit_valist (instance=0x1e8c330, signal_id=
> 372, detail=0, var_args=0x7fff1f8304f0) at gsignal.c:3326
> #9 0x00007f5f0adafab8 in g_signal_emit_by_name (instance=0x1e8c330,
> detailed_signal=0x7f5f0b6fe602 "retrieve-surrounding") at gsignal.c:3425
> #10 0x00007f5f0b429631 in gtk_im_multicontext_retrieve_surrounding_cb (slave=
> 0x21126b0 [IBusIMContext], multicontext=0x1e8c330 [GtkIMMulticontext])
> at gtkimmulticontext.c:560
> #11 0x00007f5f0b44cc09 in _gtk_marshal_BOOLEAN__VOID (closure=0x2119690,
> return_value=0x7fff1f8307e0, n_param_values=1, param_values=
> 0x7fff1f830910, invocation_hint=0x7fff1f830810, marshal_data=0x0)
> at gtkmarshalers.c:2003
> #12 0x00007f5f0ad923db in g_closure_invoke (closure=0x2119690, return_value=
> 0x7fff1f8307e0, n_param_values=1, param_values=0x7fff1f830910,
> invocation_hint=0x7fff1f830810) at gclosure.c:788
> #13 0x00007f5f0adb02c3 in signal_emit_unlocked_R (node=0x1e21000, detail=0,
> instance=0x21126b0, emission_return=0x7fff1f8309e0, instance_and_params=
> 0x7fff1f830910) at gsignal.c:3591
> #14 0x00007f5f0adaf24a in g_signal_emit_valist (instance=0x21126b0, signal_id=
> 372, detail=0, var_args=0x7fff1f830c18) at gsignal.c:3326
> #15 0x00007f5f0adaf95a in g_signal_emit (instance=0x21126b0, signal_id=372,
> detail=0) at gsignal.c:3388
> #16 0x00007f5eeda90094 in _request_surrounding_text (context=
> 0x21126b0 [IBusIMContext]) at ibusimcontext.c:278
> #17 0x00007f5eeda904da in _key_snooper_cb (widget=0x1b7a020 [GeditWindow],
> event=0x1dc56c0, user_data=0x0) at ibusimcontext.c:381
> #18 0x00007f5f0b448a45 in gtk_invoke_key_snoopers (grab_widget=
> 0x1b7a020 [GeditWindow], event=0x1dc56c0) at gtkmain.c:2259
>
> As I think the root cause is that priv->context_id and
get_effective_context_id
> (multicontext) are not sync, but basically I agree to ref _focus_im_context
> during the snooper to avoid the unexpected unref.
>
> Updated the patch.
lgtm. Thanks.
BTW, why the priv->context_id and get_effective_context_id(multicontext) are not
same? Is it a bug in gtk+?
Sign in to reply to this message.
https://codereview.appspot.com/6573051/diff/6001/client/gtk2/ibusimcontext.c File client/gtk2/ibusimcontext.c (right): https://codereview.appspot.com/6573051/diff/6001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:301: ibusimcontext->has_focus == TRUE) { I think if the ibusimcontext does not have focus or _use_key_snooper is false, we should not call _request_surrounding_text(ibusimcontext) https://codereview.appspot.com/6573051/diff/6001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:304: ibuscontext = ibusimcontext->ibuscontext; It is better set ibusimcontext to NULL if _use_key_snooper is false https://codereview.appspot.com/6573051/diff/6001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:308: ibuscontext = _fake_context; It is better set ibusimcontext to NULL here.
Sign in to reply to this message.
https://codereview.appspot.com/6573051/diff/6001/client/gtk2/ibusimcontext.c File client/gtk2/ibusimcontext.c (right): https://codereview.appspot.com/6573051/diff/6001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:303: if (_use_key_snooper) Should we test _user_key_snooper at the beginning of this function? if not, just return?
Sign in to reply to this message.
On 2012/10/02 12:58:42, Peng wrote: > https://codereview.appspot.com/6573051/diff/6001/client/gtk2/ibusimcontext.c > File client/gtk2/ibusimcontext.c (right): > > https://codereview.appspot.com/6573051/diff/6001/client/gtk2/ibusimcontext.c#... > client/gtk2/ibusimcontext.c:303: if (_use_key_snooper) > Should we test _user_key_snooper at the beginning of this function? if not, just > return? I updated the patch.
Sign in to reply to this message.
On 2012/10/02 12:41:23, Peng wrote: > BTW, why the priv->context_id and get_effective_context_id(multicontext) are not > same? Is it a bug in gtk+? when GtkIMMuticontext receives 'notify::gtk-im-module', it just frees global_context_id but does not update priv->context_id. And then priv->context_id and get_effective_context_id (multicontext) are not sync. I think it's the root cause of this bug. I got the reply in red hat bug and seems it makes sense below. They are not in sync because they don't really have to be, i.e. the change is done lazily just when it needs to be done. If we make them be in sync as in your previous patch, all gtk+ applications would be doing the same work at the same time (when the GtkSetting changes) which would cause spikes of CPU usage that we can otherwise avoid.
Sign in to reply to this message.
lgtm with one suggestion https://codereview.appspot.com/6573051/diff/12001/client/gtk2/ibusimcontext.c File client/gtk2/ibusimcontext.c (right): https://codereview.appspot.com/6573051/diff/12001/client/gtk2/ibusimcontext.c... client/gtk2/ibusimcontext.c:304: ibusimcontext->has_focus == TRUE) { One suggestion, we initialize ibusimcontext to NULL first, if _focus_im_context != NULL && it has focus, and then we set ibusimcontext to _focus_im_context. It a little readable. https://codereview.appspot.com/6573051/diff/12001/client/gtk2/ibusimcontext.c... client/gtk2/ibusimcontext.c:533: /* always install snooper */ I think in ibus-1.5, we don't need always install key snooper anymore.
Sign in to reply to this message.
On 2012/10/03 02:05:03, Peng wrote: > lgtm with one suggestion > > https://codereview.appspot.com/6573051/diff/12001/client/gtk2/ibusimcontext.c > File client/gtk2/ibusimcontext.c (right): > > https://codereview.appspot.com/6573051/diff/12001/client/gtk2/ibusimcontext.c... > client/gtk2/ibusimcontext.c:304: ibusimcontext->has_focus == TRUE) { > One suggestion, we initialize ibusimcontext to NULL first, > if _focus_im_context != NULL && it has focus, and then we set ibusimcontext to > _focus_im_context. It a little readable. done. > > https://codereview.appspot.com/6573051/diff/12001/client/gtk2/ibusimcontext.c... > client/gtk2/ibusimcontext.c:533: /* always install snooper */ > I think in ibus-1.5, we don't need always install key snooper anymore.
Sign in to reply to this message.
|
