Fix BusEngineProxy instance leak.

Fix BusEngineProxy instance leak.


BUG=none
TEST=manually with / without global-engine setting

[Daiki Ueno, 2011-06-23 01:51:56]

(Superseding https://codereview.appspot.com/4660041/, Cc'ing Sato-san in case
that this is related to Chromium OS)

I noticed that context->engine refcount does never reach to zero and engine
instances leak if user repeatedly switch one engine to another.

This patch will fix this, but I'm unsure the original code is intentional or
not.  There is a comment:

/* Do not destroy the engine anymore, because of global engine feature */

before g_object_unref (engine) in bus_input_context_unset_engine().  However,
since "Destroy" D-Bus signal is emitted when an IBusProxy object is disposed,
the comment won't make sense if the refcount is supposed to become zero.

[Daiki Ueno, 2011-06-23 02:24:20]

Hmm, sorry.  I got a crash if I select the same engine twice - I'll look into it
further.

[Daiki Ueno, 2011-06-27 08:37:17]

OK, I found two more issues regarding this.  First, when BusEngineProxy is
destroyed, "Destroy" D-Bus signal is sent twice in
bus_engine_proxy_real_destroy() and ibus_proxy_real_destroy().  Secondly, in
inputcontext.c:_ic_set_engine, an IBusEngineDesc instance passed to
bus_input_context_set_engine_by_desc() is unref'ed, though it is not ref'ed in
that function.

[Peng, 2011-06-28 04:30:46]

http://codereview.appspot.com/4662043/diff/4001/bus/inputcontext.c
File bus/inputcontext.c (left):

http://codereview.appspot.com/4662043/diff/4001/bus/inputcontext.c#oldcode1024
bus/inputcontext.c:1024: g_object_unref (desc);
The desc is returned from g_signal_emit(). Could you double check if it needs
unref? THX

[Daiki Ueno, 2011-06-30 09:19:37]

I also noticed one more unnecessary g_object_unref in bus_input_context_enable. 
I'll update the patchset soon.

https://codereview.appspot.com/4662043/diff/4001/bus/inputcontext.c
File bus/inputcontext.c (left):

https://codereview.appspot.com/4662043/diff/4001/bus/inputcontext.c#oldcode1024
bus/inputcontext.c:1024: g_object_unref (desc);
On 2011/06/28 04:30:46, Peng wrote:
> The desc is returned from g_signal_emit(). Could you double check if it needs
> unref? THX

Sorry for late response.  I checked the refcount in both the signal handler and
the g_signal_emit() side and it didn't change.

IIUC, every engine desc keeps 2 refs throughout ibus-daemon is running, as
BusIBusImpl and IBusComponent maintain the list of engine descs.  With my test,
it does not count over 2 even after I remove this unref.

[Peng, 2011-07-03 19:27:14]

On 2011/06/30 09:19:37, Daiki Ueno wrote:
> I also noticed one more unnecessary g_object_unref in
bus_input_context_enable. 
> I'll update the patchset soon.
> 
> https://codereview.appspot.com/4662043/diff/4001/bus/inputcontext.c
> File bus/inputcontext.c (left):
> 
>
https://codereview.appspot.com/4662043/diff/4001/bus/inputcontext.c#oldcode1024
> bus/inputcontext.c:1024: g_object_unref (desc);
> On 2011/06/28 04:30:46, Peng wrote:
> > The desc is returned from g_signal_emit(). Could you double check if it
needs
> > unref? THX
> 
> Sorry for late response.  I checked the refcount in both the signal handler
and
> the g_signal_emit() side and it didn't change.
> 
> IIUC, every engine desc keeps 2 refs throughout ibus-daemon is running, as
> BusIBusImpl and IBusComponent maintain the list of engine descs.  With my
test,
> it does not count over 2 even after I remove this unref.

THX. LGTM

[Daiki Ueno, 2011-07-04 00:45:38]

Thanks for the review and commit.  Closing.