Created attachment 84639 [details] [review]
Handle recursing into ensure_valid_themes()

Nasty bit of recursion into ensure_valid_themes().

The patch moves the 'theme-changed' emission til after the ensure_valid_themes() has reload the theme and performs no work if called recursively. This allows apps to reload their icons from within the theme changed handler without triggering further work. Though in this case the first caller would blow_themes() and the second caller would load_themes() and not trigger the broadcast.

* tries to rewrite the previous comment, trying this time to make sense.

The patch moves the 'theme-changed' emission til after the
ensure_valid_themes() has reloaded the theme and performs no work if called
recursively. This should allows apps to reload their icons from within the theme
changed handler without rescanning the theme directories.

Thanks, that looks good. 

Please commit to both branches.

Committed to gtk+-2.10 r17522 and trunk r17523:

2007-03-15  Chris Wilson  <chris@chris-wilson.co.uk>

	* gtk/gtkicontheme.c (ensure_valid_themes), (rescan_themes),
	(gtk_icon_theme_rescan_if_needed): Protect ensure_valid_themes()
	from recursion, which can happen for example if the app tries to
	reload an icon from within a theme-changed handler. (#418531)

Thanks

Sorry Matthias, I didn't immediately close this bug as I felt the recursion was only half the problem.

I've been able to trigger recursion within the unpatched ensure_valid_themes() by touching an icon directory before calling gtk_icon_theme_lookup_icon() within the theme-changed handler. However it doesn't die with the same segfault. 

At the moment I can't see how that stack got into the mess it did, so I'd leave the code as is until we get a new bug report with a nice clean stack trace...

That's still happening with a patched GTK 2.10.11, reopening

Sebastien, is the stacktrace identical?

Look similar:

==6332== Invalid read of size 1
==6332==    at 0x402257E: strcmp (mc_replace_strmem.c:341)
==6332==    by 0x48737B3: g_str_equal (gstring.c:77)
==6332==    by 0x484A9FE: g_hash_table_insert (ghash.c:240)
==6332==    by 0x438661E: insert_theme (gtkicontheme.c:2244)
==6332==    by 0x438642A: insert_theme (gtkicontheme.c:997)
==6332==    by 0x438642A: insert_theme (gtkicontheme.c:997)
==6332==    by 0x438642A: insert_theme (gtkicontheme.c:997)
==6332==    by 0x438642A: insert_theme (gtkicontheme.c:997)
==6332==    by 0x43876BE: ensure_valid_themes (gtkicontheme.c:1047)
==6332==    by 0x4387B51: _gtk_icon_theme_check_reload (gtkicontheme.c:3097)
==6332==    by 0x44E8D36: gtk_window_client_event (gtkwindow.c:4873)
==6332==    by 0x43BE6AF: _gtk_marshal_BOOLEAN__BOXED (gtkmarshalers.c:84)
==6332==    by 0x47F7E48: g_type_class_meta_marshal (gclosure.c:567)
==6332==    by 0x47F962A: g_closure_invoke (gclosure.c:490)
==6332==    by 0x480A752: signal_emit_unlocked_R (gsignal.c:2478)
==6332==    by 0x480B3EE: g_signal_emit_valist (gsignal.c:2209)
==6332==    by 0x480B7E8: g_signal_emit (gsignal.c:2243)
==6332==    by 0x44D2E17: gtk_widget_event_internal (gtkwidget.c:3915)
==6332==    by 0x43B8CB8: gtk_main_do_event (gtkmain.c:1576)
==6332==    by 0x4618129: gdk_event_dispatch (gdkevents-x11.c:2318)
==6332==    by 0x4856DF1: g_main_context_dispatch (gmain.c:2045)
==6332==    by 0x4859DCE: g_main_context_iterate (gmain.c:2677)
==6332==    by 0x485A178: g_main_loop_run (gmain.c:2881)
==6332==    by 0x43B9043: gtk_main (gtkmain.c:1177)
==6332==    by 0x806399B: main (main.c:98)
==6332==  Address 0x89A4A11 is 1 bytes inside a block of size 7 free'd
==6332==    at 0x402123A: free (vg_replace_malloc.c:233)
==6332==    by 0x485E130: g_free (gmem.c:187)
==6332==    by 0x484A910: g_hash_table_replace (ghash.c:390)
==6332==    by 0x4386643: insert_theme (gtkicontheme.c:2245)
==6332==    by 0x438642A: insert_theme (gtkicontheme.c:997)
==6332==    by 0x438642A: insert_theme (gtkicontheme.c:997)
==6332==    by 0x438642A: insert_theme (gtkicontheme.c:997)
==6332==    by 0x438642A: insert_theme (gtkicontheme.c:997)
==6332==    by 0x43876BE: ensure_valid_themes (gtkicontheme.c:1047)
==6332==    by 0x4387B51: _gtk_icon_theme_check_reload (gtkicontheme.c:3097)
==6332==    by 0x44E8D36: gtk_window_client_event (gtkwindow.c:4873)
==6332==    by 0x43BE6AF: _gtk_marshal_BOOLEAN__BOXED (gtkmarshalers.c:84)
==6332==    by 0x47F7E48: g_type_class_meta_marshal (gclosure.c:567)
==6332==    by 0x47F962A: g_closure_invoke (gclosure.c:490)
==6332==    by 0x480A752: signal_emit_unlocked_R (gsignal.c:2478)
==6332==    by 0x480B3EE: g_signal_emit_valist (gsignal.c:2209)
==6332==    by 0x480B7E8: g_signal_emit (gsignal.c:2243)
==6332==    by 0x44D2E17: gtk_widget_event_internal (gtkwidget.c:3915)
==6332==    by 0x43B8CB8: gtk_main_do_event (gtkmain.c:1576)
==6332==    by 0x4618129: gdk_event_dispatch (gdkevents-x11.c:2318)
==6332==    by 0x4856DF1: g_main_context_dispatch (gmain.c:2045)
==6332==    by 0x4859DCE: g_main_context_iterate (gmain.c:2677)
==6332==    by 0x485A178: g_main_loop_run (gmain.c:2881)
==6332==    by 0x43B9043: gtk_main (gtkmain.c:1177)
==6332==    by 0x806399B: main (main.c:98)

Aye, it's the same bug without the nasty recursion...

Sebastien, I think I can finally see what the real problem is here... The two keys for icon_theme->all_icons and dir->icons must be kept synced as they share the same string. So by inserting in the first we kept the original string, but then freed it when replacing in the second... Bang.

Trivial patch to try:
Index: gtk/gtkicontheme.c
===================================================================
--- gtk/gtkicontheme.c  (revision 17549)
+++ gtk/gtkicontheme.c  (working copy)
@@ -2241,7 +2241,7 @@ scan_directory (GtkIconThemePrivate *ico
       base_name = strip_suffix (name);
 
       hash_suffix = GPOINTER_TO_INT (g_hash_table_lookup (dir->icons, base_name));
-      g_hash_table_insert (icon_theme->all_icons, base_name, NULL);
+      g_hash_table_replace (icon_theme->all_icons, base_name, NULL);
       g_hash_table_replace (dir->icons, base_name, GUINT_TO_POINTER (hash_suffix| suffix));
     }

the new patch seems to work correctly, no valgrind error with it

Applied to 2.10 (r17625) and trunk (r17626):
2007-04-24  Chris Wilson  <chris@chris-wilson.co.uk>

	* gtk/gtkicontheme.c (scan_directory): Ensure the 
	icon_theme->all_icons and dir->icons hash tables use the same string
	as their keys. (#418531)