Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory leak in ibus-ui-gtk3 #1899

Closed
wants to merge 2 commits into from
Closed

Fix memory leak in ibus-ui-gtk3 #1899

wants to merge 2 commits into from

Conversation

ers35
Copy link

@ers35 ers35 commented Feb 26, 2017

get_global_engine() returns a new object with a reference count of 1. Vala
adds another reference because "new IBus.EngineDesc()" isn't used. Remove the
additional reference so the engine is eventually freed.

Valgrind:

22,555 (4,160 direct, 18,395 indirect) bytes in 65 blocks are definitely lost in loss record 8,904 of 8,914
   at 0x733B3E7: g_type_create_instance (gtype.c:1845)
   by 0x731CCD7: g_object_constructor (gobject.c:2068)
   by 0x731D028: g_object_new_with_custom_constructor (gobject.c:1701)
   by 0x731D028: g_object_new_internal (gobject.c:1781)
   by 0x731EC0C: g_object_newv (gobject.c:1930)
   by 0x731F3C3: g_object_new (gobject.c:1623)
   by 0x68F5FDB: ibus_serializable_deserialize (ibusserializable.c:292)
   by 0x690AF7B: ibus_bus_get_global_engine (ibusbus.c:2002)
   by 0x120E2A: panel_real_state_changed (panel.c:5371)
   by 0x7317F74: g_closure_invoke (gclosure.c:804)
   by 0x732A37C: signal_emit_unlocked_R (gsignal.c:3673)
   by 0x7332BCB: g_signal_emit_valist (gsignal.c:3391)
   by 0x7332FAE: g_signal_emit (gsignal.c:3447)

get_global_engine() returns a new object with a reference count of 1. Vala
adds another reference because "new IBus.EngineDesc()" isn't used. Remove the
additional reference so the engine is eventually freed.
@@ -1419,6 +1419,12 @@ class Panel : IBus.PanelService {

var engine = m_bus.get_global_engine();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the patch.
I think unowned should be used here.
unowned IBus.EngineDesc engine = m_bus.get_global_engine();

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't unowned turn off reference counting and the engine will never be freed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right.
And then I think it's an annotation problem.
The get_global_engine() should be '(transfer full)':
https://github.com/ibus/ibus/blob/master/src/ibusbus.h

fujiwarat pushed a commit that referenced this pull request Mar 1, 2017
Fix the memory leak by setting the transfer mode annotation to full.

BUG=#1899
R=Shawn.P.Huang@gmail.com

Review URL: https://codereview.appspot.com/312520043

Patch from Eric R. Schulz <eric@ers35.com>.
@fujiwarat fujiwarat closed this Mar 1, 2017
@ers35 ers35 deleted the memory-leak branch March 1, 2017 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants