Fix ibus-daemon deadlock in engineproxy.c.

Fix ibus-daemon deadlock in engineproxy.c.

How to reproduce the deadlock on a desktop Linux like Ubuntu Maverick:

1. Add 20 seconds sleep in the beginning of the main() function of an engine (e.g. ibus-engine-mozc). See http://crosbug.com/11379#c16 .
2. Set preload_engines to "mozc" using ibus-setup.
3. Start ibus-daemon.
4. press the trigger hotkey twice within the 20 seconds.

Expected:
The second trigger hotkey press cancels the bus_engine_proxy_new operation started by the first one.

Actual:
ibus-daemon freezes. Stack trace: http://crosbug.com/11379#c20

BUG=http://crosbug.com/11379
TEST=see the steps above.

[Yusuke Sato, 2011-02-07 13:32:46]

found a problem in the patch...
please stop reviewing for now.

[Yusuke Sato, 2011-02-07 17:03:34]

I'll fix/check the followings tomorrow. Peng, if you have comments, please let
me know in advance.

http://codereview.appspot.com/4125053/diff/2001/bus/engineproxy.c
File bus/engineproxy.c (right):

http://codereview.appspot.com/4125053/diff/2001/bus/engineproxy.c#newcode624
bus/engineproxy.c:624: g_simple_async_result_set_from_error (data->simple,
error);
This function is called even if the dbus_proxy_call() is cancelled.
In this case, g_simple_async_result_complete (data->simple); might be already
called and therefore the |data| structure might already be freed.

Probably we should first check whether the error code is G_IO_ERROR_CANCELLED,
then skip the g_simple calls if it's G_IO_ERROR_CANCELLED.

http://codereview.appspot.com/4125053/diff/2001/bus/engineproxy.c#newcode724
bus/engineproxy.c:724: g_object_unref (data->cancellable);
make sure if it's really necessary.

http://codereview.appspot.com/4125053/diff/2001/bus/engineproxy.c#newcode782
bus/engineproxy.c:782: bus_factory_proxy_create_engine (data->factory,
I think we should also connect to the cancelled_cb() function.
(or disconnect cancelled_cb() in notify_factory_cb().)

http://codereview.appspot.com/4125053/diff/2001/bus/engineproxy.c#newcode827
bus/engineproxy.c:827: return engine;
we should unref() data->simple?

[Peng, 2011-02-07 18:52:25]

http://codereview.appspot.com/4125053/diff/2001/bus/engineproxy.c
File bus/engineproxy.c (right):

http://codereview.appspot.com/4125053/diff/2001/bus/engineproxy.c#newcode624
bus/engineproxy.c:624: g_simple_async_result_set_from_error (data->simple,
error);
You are right. We need deal the cancellable carefully. Maybe it is better to
pass a new cancellable object to bus_factory_proxy_create_engine.

On 2011/02/07 17:03:34, Yusuke Sato wrote:
> This function is called even if the dbus_proxy_call() is cancelled.
> In this case, g_simple_async_result_complete (data->simple); might be already
> called and therefore the |data| structure might already be freed.
> 
> Probably we should first check whether the error code is G_IO_ERROR_CANCELLED,
> then skip the g_simple calls if it's G_IO_ERROR_CANCELLED.
>

http://codereview.appspot.com/4125053/diff/2001/bus/engineproxy.c#newcode782
bus/engineproxy.c:782: bus_factory_proxy_create_engine (data->factory,
Yeah. We need diconnect the cancelled_cb() in notify_factory_cb()
On 2011/02/07 17:03:34, Yusuke Sato wrote:
> I think we should also connect to the cancelled_cb() function.
> (or disconnect cancelled_cb() in notify_factory_cb().)
>

http://codereview.appspot.com/4125053/diff/2001/bus/engineproxy.c#newcode827
bus/engineproxy.c:827: return engine;
On 2011/02/07 17:03:34, Yusuke Sato wrote:
> we should unref() data->simple?

Right

http://codereview.appspot.com/4125053/diff/2001/bus/inputcontext.c
File bus/inputcontext.c (right):

http://codereview.appspot.com/4125053/diff/2001/bus/inputcontext.c#newcode2046
bus/inputcontext.c:2046: g_object_ref (context->engine);
I think this should be removed

[Yusuke Sato, 2011-02-08 08:02:57]

Fixed all. Please review this.

http://codereview.appspot.com/4125053/diff/2001/bus/inputcontext.c
File bus/inputcontext.c (right):

http://codereview.appspot.com/4125053/diff/2001/bus/inputcontext.c#newcode2046
bus/inputcontext.c:2046: g_object_ref (context->engine);
On 2011/02/07 18:52:25, Shawn.P.Huang wrote:
> I think this should be removed

Tried, but got many "(lt-ibus-daemon:22372): GLib-GObject-CRITICAL **:
g_object_unref: assertion `G_IS_OBJECT (object)' failed" errors... any ideas?

[Yusuke Sato, 2011-02-09 12:00:57]

Received a revised patch from Peng (patch set #3), and it looks good to me. I'll
submit it soon.

On 2011/02/08 08:02:57, Yusuke Sato wrote:
> Fixed all. Please review this.
> 
> http://codereview.appspot.com/4125053/diff/2001/bus/inputcontext.c
> File bus/inputcontext.c (right):
> 
> http://codereview.appspot.com/4125053/diff/2001/bus/inputcontext.c#newcode2046
> bus/inputcontext.c:2046: g_object_ref (context->engine);
> On 2011/02/07 18:52:25, Shawn.P.Huang wrote:
> > I think this should be removed
> 
> Tried, but got many "(lt-ibus-daemon:22372): GLib-GObject-CRITICAL **:
> g_object_unref: assertion `G_IS_OBJECT (object)' failed" errors... any ideas?