gtk: Fix memory leak in ThemeServiceGtk::LoadGtkValues.

gtk: Fix memory leak in ThemeServiceGtk::LoadGtkValues.

BUG=120118
R=erg@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=129414

[John Knottenbelt, 2012-03-27 09:30:42]

Thanks for taking this on, Thiago! This does fix the memory leak for me.

I made one inline comment. 

There are other cases in this file where gtk_widget_style_get is called,
probably with similar memory leaks. Do you think we should try to address all of
them, or just this one location? 

If we should address all of them, what do you think about introducing a wrapper
function for gtk_widget_style_get, or perhaps some kind of scoped_ptr variant
that will automatically call GdkColorFree ?

http://codereview.chromium.org/9855028/diff/1/chrome/browser/ui/gtk/theme_ser...
File chrome/browser/ui/gtk/theme_service_gtk.cc (right):

http://codereview.chromium.org/9855028/diff/1/chrome/browser/ui/gtk/theme_ser...
chrome/browser/ui/gtk/theme_service_gtk.cc:781:
gdk_color_free(const_cast<GdkColor*>(link_color));
We shouldn't free link_color if it is &kDefaultLinkColor.

[tfarina, 2012-03-27 13:34:00]

On 2012/03/27 09:30:42, John Knottenbelt wrote:
> Thanks for taking this on, Thiago! This does fix the memory leak for me.
> 
> I made one inline comment. 
> 
> There are other cases in this file where gtk_widget_style_get is called,
> probably with similar memory leaks. Do you think we should try to address all
of
> them, or just this one location? 
> 
I tried to fix them in patch set 2.

> If we should address all of them, what do you think about introducing a
wrapper
> function for gtk_widget_style_get, or perhaps some kind of scoped_ptr variant
> that will automatically call GdkColorFree ?
>
I'm not sure, maybe Elliot has a different opinion about this. My view is that
we don't need a scoped class for this.
 
>
http://codereview.chromium.org/9855028/diff/1/chrome/browser/ui/gtk/theme_ser...
> File chrome/browser/ui/gtk/theme_service_gtk.cc (right):
> 
>
http://codereview.chromium.org/9855028/diff/1/chrome/browser/ui/gtk/theme_ser...
> chrome/browser/ui/gtk/theme_service_gtk.cc:781:
> gdk_color_free(const_cast<GdkColor*>(link_color));
> We shouldn't free link_color if it is &kDefaultLinkColor.

Done.

[John Knottenbelt, 2012-03-27 14:45:46]

http://codereview.chromium.org/9855028/diff/4001/chrome/browser/ui/gtk/theme_...
File chrome/browser/ui/gtk/theme_service_gtk.cc (right):

http://codereview.chromium.org/9855028/diff/4001/chrome/browser/ui/gtk/theme_...
chrome/browser/ui/gtk/theme_service_gtk.cc:781: if (!gdk_color_equal(link_color,
&kDefaultLinkColor))
We need to compare address, rather than value. 
Also, gdk_color_free shouldn't be called on a NULL pointer (See
http://git.gnome.org/browse/gtk+/tree/gdk/gdkcolor.c#n70 and
http://git.gnome.org/browse/glib/tree/glib/gmessages.h#n275 ). This could work:

if (link_color && link_color != &kDefaultLinkColor)

Alternatively, it might be neater to introduce a separate variable for the
result of gtk_widget_style_get (up to you).

http://codereview.chromium.org/9855028/diff/4001/chrome/browser/ui/gtk/theme_...
chrome/browser/ui/gtk/theme_service_gtk.cc:825: gdk_color_free(theme_frame);
We need to check for NULL here and in the other calls to gdk_color_free below
too.

[tfarina, 2012-03-27 15:29:06]

http://codereview.chromium.org/9855028/diff/4001/chrome/browser/ui/gtk/theme_...
File chrome/browser/ui/gtk/theme_service_gtk.cc (right):

http://codereview.chromium.org/9855028/diff/4001/chrome/browser/ui/gtk/theme_...
chrome/browser/ui/gtk/theme_service_gtk.cc:781: if (!gdk_color_equal(link_color,
&kDefaultLinkColor))
On 2012/03/27 14:45:47, John Knottenbelt wrote:
> We need to compare address, rather than value. 
> Also, gdk_color_free shouldn't be called on a NULL pointer (See
> http://git.gnome.org/browse/gtk+/tree/gdk/gdkcolor.c#n70 and
> http://git.gnome.org/browse/glib/tree/glib/gmessages.h#n275 ). This could
work:
> 
> if (link_color && link_color != &kDefaultLinkColor)
> 
> Alternatively, it might be neater to introduce a separate variable for the
> result of gtk_widget_style_get (up to you).

Done.

http://codereview.chromium.org/9855028/diff/4001/chrome/browser/ui/gtk/theme_...
chrome/browser/ui/gtk/theme_service_gtk.cc:825: gdk_color_free(theme_frame);
On 2012/03/27 14:45:47, John Knottenbelt wrote:
> We need to check for NULL here and in the other calls to gdk_color_free below
> too.

Done.

[John Knottenbelt, 2012-03-27 15:37:08]

LGTM. Thanks Thiago!

[John Knottenbelt, 2012-03-27 16:43:00]

https://chromiumcodereview.appspot.com/9491009/ just landed which added a
suppression for this bug in tools/valgrind/memcheck/suppressions.txt - See
https://chromiumcodereview.appspot.com/9491009/diff/41001/tools/valgrind/memc...

Would be awesome to remove this suppression as part of this patch.

[tfarina, 2012-03-27 17:01:26]

On 2012/03/27 16:43:00, John Knottenbelt wrote:
> https://chromiumcodereview.appspot.com/9491009/ just landed which added a
> suppression for this bug in tools/valgrind/memcheck/suppressions.txt - See
>
https://chromiumcodereview.appspot.com/9491009/diff/41001/tools/valgrind/memc...
> 
> Would be awesome to remove this suppression as part of this patch.

Yup, patch set 4 removed it. Running linux_valgrind.

[Elliot Glaysher, 2012-03-27 21:43:00]

lgtm

http://codereview.chromium.org/9855028/diff/8002/chrome/browser/ui/gtk/theme_...
File chrome/browser/ui/gtk/theme_service_gtk.cc (right):

http://codereview.chromium.org/9855028/diff/8002/chrome/browser/ui/gtk/theme_...
chrome/browser/ui/gtk/theme_service_gtk.cc:781: if (link_color && link_color !=
&kDefaultLinkColor)
Better to set a bool if &kDefaultLinkColor set above. link_color could easily be
kDefaultLinkColor.

[tfarina, 2012-03-27 22:27:00]

On 2012/03/27 21:43:00, Elliot Glaysher wrote:
> lgtm
> 
>
http://codereview.chromium.org/9855028/diff/8002/chrome/browser/ui/gtk/theme_...
> File chrome/browser/ui/gtk/theme_service_gtk.cc (right):
> 
>
http://codereview.chromium.org/9855028/diff/8002/chrome/browser/ui/gtk/theme_...
> chrome/browser/ui/gtk/theme_service_gtk.cc:781: if (link_color && link_color
!=
> &kDefaultLinkColor)
> Better to set a bool if &kDefaultLinkColor set above. link_color could easily
be
> kDefaultLinkColor.

Done, please could you take another look?

[Elliot Glaysher, 2012-03-27 22:29:02]

slgtm

[John Knottenbelt, 2012-03-28 05:41:40]

lgtm