GNOME Bugzilla – Bug 352628
Memory leak due to the addition of menu accelerators
Last modified: 2006-08-29 01:17:45 UTC
Please describe the problem: The problem has been detected in Evolution. Every switch between components causes an increase of the time of switching. This behaviour can be observed here: http://m3gumi.wordpress.com/files/2006/07/mixed-real.png This is caused by the addition of accelerators in menu item many times when once was enough (bonobo-ui-sync-menu.c:impl_bonobo_ui_sync_menu_state()). Moreover, some of these accelerators are never removed, causing memory leak problems which lead to time increasing in the function that add the accelerators. Steps to reproduce: 1. Automate the switching of Evolution. 2. Get samples of time of every switch in gtk_widget_add_accelerator of function impl_bonobo_ui_sync_menu_state(). Or better, in g_memmove() of quick_accel_add() (gtkaccelgroup.c). 3. Watch the increase of memory with command top. Actual results: Every switch, time for adding accelerators and resident memory of evolution increase. Expected results: Time should be constant in every switch. Memory should not grow so much. Does this happen every time? Yes. Other information:
Created attachment 71499 [details] [review] Avoid the useless adding of menu accelerators. The patch respects the fact that we can have a widget with multiple accelerators and that the same accelerator can be added many times in the same accelerator group. The code only avoid the accelerator addition if it has been added previously to the same widget (which is the common case observed).
Created attachment 71500 [details] Changelog of the submitted patch
That's great result and patch, but shouldn't the fix go into gtk?
I suppose it is slightly odd that gtk+ will allow this multiple identical binding thing to happen, but the patch is fine and at least we prolly shouldn't abuse gtk+ in this way :-) Please do commit [ is there a similar problem for toolbar icons ? ]
Cecilia, do you have an account on cvs.gnome.org? I can commit it for you if you don't, and we can start the process of getting you an account.
Hi Federico, I don't have a cvs account.
Notice that hard code freeze is next monday; someone should commit this soon..
Commited. I'll do another release to get this out the door as soon as possible.
Thanks for committing this, Kjartan :) And again, thanks to Cecilia for the wonderful analysis and patch!
Comment on attachment 71499 [details] [review] Avoid the useless adding of menu accelerators. --- cvs/libbonoboui/bonobo/bonobo-ui-sync-menu.c_old 2006-08-22 11:33:16.000000000 +0100 +++ cvs/libbonoboui/bonobo/bonobo-ui-sync-menu.c 2006-08-28 13:41:10.000000000 +0100 @@ -282,6 +282,33 @@ label_same (GtkBin *menu_widget, const c !strcmp (((GtkLabel *)label)->label, txt); } +static gboolean +widget_has_accel (GtkWidget *widget, + GtkAccelGroup *accel_group, + guint key, + GdkModifierType mods) +{ + int i; + guint n_entries; + GList *l_tmp, *l_closures = gtk_widget_list_accel_closures (widget); + GtkAccelGroupEntry *entry = gtk_accel_group_query (accel_group, + key, mods, + &n_entries); + + if (n_entries) + for (l_tmp = l_closures; l_tmp; l_tmp = l_tmp->next) + for (i=0; i<n_entries; i++) + if (entry[i].closure == l_tmp->data) + { + g_list_free (l_closures); + return TRUE; + } + + g_list_free (l_closures); + + return FALSE; +} + static void impl_bonobo_ui_sync_menu_state (BonoboUISync *sync, BonoboUINode *node, @@ -392,11 +419,12 @@ impl_bonobo_ui_sync_menu_state (BonoboUI if (!key) /* FIXME: this looks strange */ return; - gtk_widget_add_accelerator (menu_widget, - "activate", - sync_menu->accel_group, - key, mods, - GTK_ACCEL_VISIBLE); + if (! widget_has_accel (menu_widget, sync_menu->accel_group, key, mods)) + gtk_widget_add_accelerator (menu_widget, + "activate", + sync_menu->accel_group, + key, mods, + GTK_ACCEL_VISIBLE); } bonobo_ui_engine_queue_update (
Comment on attachment 71499 [details] [review] Avoid the useless adding of menu accelerators. Edited! It had a problem of memory leaking.
cecilia, this means that this has to get into CVS again, right? :-) (asking because hardcode freeze will take place today at 23:59UTC)
Andre, yes, it should be re-commited.
Committed! Thanks for the fix, Cecilia.