I'm experiencing this bug too. With the upgrade to gtk 3.10.1 the system has become unusable, crashing every time that you try to show the activities view. Downgrading to 3.10.0 fixes the problem. I'm using the default theme for gtk and gnome-shell, in case it makes any difference.

Review of attachment 257086 [details] [review]:

::: gtk/gtkicontheme.c
@@ +3882,3 @@
+             be possible for the caller to request the error later. */ 
+          if (error)
+            g_propagate_error (error, icon_info->load_error);

This looks wrong to me, because if error isn't NULL you'll get the same double-free crash. Instead of propagate you should do *error = g_error_copy (icon_info->load_error);

You are probably correct. I assumed this line in the docs:

"If dest is NULL, free src; otherwise, moves src into *dest. The error variable dest points to must be NULL."

...meant that g_propagate_error would take care of everything (now I notice g_propagate_error cannot possibly set src to NULL). It is probably meant to be used as one of the last lines in a function (similar to throw in exceptions perhaps?).

Anyway, your suggestion is correct, I'll make a new patch.

Created attachment 257199 [details] [review]
New fix with jjacky recommendation

Patch with jjacky's recommendation. Compiled and tested agains master/HEAD. No crashes in Gnome Shell.

I'm affected too. Gnome-shell with crashing activities is unusable. Reverting to gtk 3.10.0 fixed. Strangely I'm not affected in another computer.

GtkIconView has nothing to do with GtkIconTheme; re-assigning to the right component.

Review of attachment 257199 [details] [review]:

thanks for the patch. it would be great if you could attach a patch formatted using `git format-patch`, or using `git bz`, as it makes it easier for us to review and apply (while also crediting the author).

::: gtk/gtkicontheme.c
@@ +3876,3 @@
+        {
+          /* g_propagate_error would be wrong here as
+             icon_info->load_error gets freed in the

g_propagate_error() clears the original error, but does not set it to NULL. g_clear_error() is NULL safe, so this should really be:

  if (icon_info->load_error != NULL)
    {
      g_propagate_error (error, icon_info->load_error);
      icon_info->load_error = NULL;
    }

@@ +3877,3 @@
+          /* g_propagate_error would be wrong here as
+             icon_info->load_error gets freed in the
+             finalizer. */

coding style: multi-line comments should be in the form:

  /* line 1
   * line 2
   */

@@ +3879,3 @@
+             finalizer. */
+          if (error)
+            *error = g_error_copy(icon_info->load_error);

coding style: missing space between function name and parenthesis.

> g_propagate_error() clears the original error, but does not set it to NULL.
> g_clear_error() is NULL safe, so this should really be:
> 
>   if (icon_info->load_error != NULL)
>     {
>       g_propagate_error (error, icon_info->load_error);
>       icon_info->load_error = NULL;
>     }

Are you sure? It seemed to me that the expected behavior was that icon_info->load_error should remain set, to indicate a load attempt was done & failed; Hence my suggestion of copying the error.

(In reply to comment #8)
> > g_propagate_error() clears the original error, but does not set it to NULL.
> > g_clear_error() is NULL safe, so this should really be:
> > 
> >   if (icon_info->load_error != NULL)
> >     {
> >       g_propagate_error (error, icon_info->load_error);
> >       icon_info->load_error = NULL;
> >     }
> 
> Are you sure? It seemed to me that the expected behavior was that
> icon_info->load_error should remain set, to indicate a load attempt was done &
> failed; Hence my suggestion of copying the error.

the intent seems to be that one, yes. as far as I can see it, the only case where load_icon() can be called multiple times after an error is that somebody is mixing the synchronous and asynchronous API in the same process, using the same GtkIconInfo. the whole design seems to be a bit messy to me, with far too much caching going on.

anyway, g_error_copy() seems to be what gtk_icon_info_load_icon_finish() uses, so we can use it in load_icon() as well.

could you please amend your patch so that it follows the GTK coding style, and use git format-patch? that would be most helpful.

I see this was fixed in d967266b772f3050dffae98aa449128f63055fc4 in master, so there's no need to resubmit the patch I guess. Thanks for reviewing the patch, I'll make sure to follow the guidelines in the future.

Whoa, is this line right in the commit?

if (*error)

I though error was a pointer to a pointer, so you could pass NULL as a parameter in case you did not want to receive the error. If so, this would crash? I'm at work right now, so I cannot take a deeper look right now.

Yeah, that commit will cause a segfault if error is NULL, needs to be fixed.

Reopening (see previous comments).

Created attachment 257393 [details] [review]
New patch. Properly formatted.

Attached properly formatted patch. If you want a fix against master HEAD, let me know.

Thanks, fixed