Bug 119448 – [Gtk] Suppress irrelevant or known leaks for Valgrind

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Brian Holt]

Valgrind often works too well and reports memory leaks and violations that might not be relevant or might already be known to the application that is being analysed.

To make this easier Valgrind supports a the notion of suppressions.

[Brian Holt]

Created attachment 208029 [details]
Patch

[Mario Sanchez Prada]

Comment on attachment 208029 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208029&action=review

Thanks for the patch, Brian. Informal review here:

First of all, the patch looks good in general, although there are some problems such as the fact that the patch does not seem to be consistent with the fact that this bug depends on bug 118785 (you're adding some code here that is already proposed as part of the latest patch for that other bug, instead of proposing just the incremental diff compared to that one), or that there are some issues related to coding style that would need fixing before landing.

See my comments below...

> Tools/Scripts/webkitpy/port/gtk.py:31
> +import uuid

This line is already included as part of the latest patch for bug 118785. Please remove it from here.

> Tools/Scripts/webkitpy/port/gtk.py:94
> +        if self.get_option("leaks"):
> +            environment['G_SLICE'] = 'always-malloc'
> +            xmlfile = "".join(('\"', self.results_directory(), "/drt-%p-",
> +                               uuid.uuid1().hex, "-leaks.xml", '\"'))
> +            suppressionsfile = self.path_from_webkit_base(
> +                'Tools', 'valgrind', 'memcheck', 'suppressions.txt')
> +            environment['VALGRIND_OPTS'] = "--tool=memcheck " \
> +            "--num-callers=40 --demangle=no --trace-children=no " \
> +            "--smc-check=all-non-file --leak-check=yes " \
> +            "--leak-resolution=high --show-possibly-lost=no " \
> +            "--show-reachable=no --leak-check=full " \
> +            " --undef-value-errors=no " \
> +             "--gen-suppressions=all --xml=yes --xml-file=%s " \

As in the previous case commented above, these lines are already included as part of the latest patch for bug 118785. Please remove it from here and leave only the part that is actually added (the --suppressions=%s part)

> Tools/Scripts/webkitpy/port/gtk.py:99
> +            # FIXME: instead we should be using "jhbuild-wrapper --gtk run"
> +            # to ensure we get the correct library path.
> +            #environment['LD_LIBRARY_PATH'] = self.path_from_webkit_base(
> +            # 'WebKitBuild', 'Dependencies', 'Root', 'lib64')

These lines are already part of the other patch too. Please remove them

> Tools/Scripts/webkitpy/port/gtk.py:101
> +

Extra line.

> Tools/valgrind/memcheck/suppressions.txt:1
> +# There are four kinds of suppressions in this file.

Please fix this sentence: You mention there are four kinds of suppressions but I could only find three.

> Tools/valgrind/memcheck/suppressions.txt:2
> +# 1. third party stuff we have no control over

Bad sentence: Start with capitalized word ("Third") and finish with a period.

> Tools/valgrind/memcheck/suppressions.txt:5
> +# 2. intentional unit test errors, or stuff that is somehow a false positive
> +# in our own code, or stuff that is so trivial it's not worth fixing

Likewise.

> Tools/valgrind/memcheck/suppressions.txt:8
> +# These should all be in webkit's bug tracking system (but a few aren't yet).

webkit -> WebKit

> Tools/valgrind/memcheck/suppressions.txt:13
> +# 1. third party stuff we have no control over

Bad sentence: Start with capitalized word ("Third") and finish with a period.

> Tools/valgrind/memcheck/suppressions.txt:15
> +   #gtk developers don't like cleaning up one-time leaks.  See http://mail.gnome.org/archives/gtk-devel-list/2004-April/msg00230.html

Missing space at the beginning, missing period at the end and bad capitalization: "#gtk developers" -> "# GTK developers"

Also, you have two spaces between the period and "See". Please replace those spaces with a line break instead

> Tools/valgrind/memcheck/suppressions.txt:29
> +   FcConfigAppFontAddFile_leak

This underscore here is not needed (and inconsistent with what you are doing in other places (e.g. the previous two suppressions). Also, I would probably call this "Fontconfig leak 1" for consistency with the rest of the file (see my following comments below)

> Tools/valgrind/memcheck/suppressions.txt:36
> +   # See also http://www.gnome.org/~johan/gtk.suppression
> +   # (which has a smattering of similar pango suppressions)

Missing period at the end of the line

> Tools/valgrind/memcheck/suppressions.txt:37
> +   pango_font_leak_todo

Underscores not needed, and name not consistent with others. What about "Fontconfig leak 2"?

> Tools/valgrind/memcheck/suppressions.txt:45
> +   pango_font_leak_todo_2

"Fontconfig leak 3"?

> Tools/valgrind/memcheck/suppressions.txt:55
> +   pango_font_leak_todo_3

"Fontconfig leak 4"?

Same applies to many other names in this file. Please try to get a rule for naming suppressions and stick to it, for the sake of consistency (and remember you do not need to use underscores here)

> Tools/valgrind/memcheck/suppressions.txt:63
> +   pango_font_leak_todo_4

"Fontcontig leak 5"? (last nit of this kind that I mention, please fix the others)

> Tools/valgrind/memcheck/suppressions.txt:88
> +   # Similar to fontconfig_bug_8428 below. Reported in
> +   # https://bugs.freedesktop.org/show_bug.cgi?id=8215

Missing period at the end.

> Tools/valgrind/memcheck/suppressions.txt:98
> +   # Fontconfig leak, seen in shard 16 of 20 of ui_tests
> +   # See https://bugs.freedesktop.org/show_bug.cgi?id=8428
> +   # and http://www.gnome.org/~johan/gtk.suppression

Likewise

> Tools/valgrind/memcheck/suppressions.txt:116
> +   fontconfig_bug   (Third Party)

Please notice you have extra unneeded spaces in this line while fixing its name

> Tools/valgrind/memcheck/suppressions.txt:176
> +   dlopen leak on error. See http://sourceware.org/bugzilla/show_bug.cgi?id=12878.

Be consistent: Please either you put the "See http://..." string in the previous line as a comment preceeded by "#", or you just avoid using comments at all and you always bundle the "Seeh http://..." string along with the name of the rules. I personally think the first option is better.

> Tools/valgrind/memcheck/suppressions.txt:203
> +   # array of weak references freed but not processed?

Bad capitalization at the beginning. Missing period at the end of the line.

> Tools/valgrind/memcheck/suppressions.txt:211
> +   http://sources.redhat.com/bugzilla/show_bug.cgi?id=5171

You never used an URL before as the name of the suppression. As I mentioned before, please be consistent.

> Tools/valgrind/memcheck/suppressions.txt:283
> +    Gtk leaks an X11 window bug_96402 (Third party)

Extra space before "Gtk". Also, it is GTK or GTK+, but not Gtk.

> Tools/valgrind/memcheck/suppressions.txt:299
> +   Gtk leaks a cairo context (large leak)

Gtk -> GTK

> Tools/valgrind/memcheck/suppressions.txt:309
> +   leaks in bash

leaks -> Leaks

> Tools/valgrind/memcheck/suppressions.txt:343
> +# XRandRInfo object seems to be leaking inside XRRFindDisplay.  This happens the
> +# first time it is called, no matter who the caller is.  We have observed this
> +# problem with both XRRSelectInput and XRRQueryExtension.

Extra blank spaces that are not needed after the two first periods.

> Tools/valgrind/memcheck/suppressions.txt:351
> +   Flash Player Leak

Flash Player Leak -> Flash player leak

> Tools/valgrind/memcheck/suppressions.txt:426
> +   cairo pixman image allocate

Part of this sentence is a function name, so I would put something like "Cairo _pixman_image_allocate leak"

> Tools/valgrind/memcheck/suppressions.txt:514
> +   AtkProperty is cached but not released (see https://bugs.webkit.org/show_bug.cgi?id=118567#c2)

Again, please be consistent with naming and URLs

> Tools/valgrind/memcheck/suppressions.txt:526
> +   AtkRelationSet is never freed because the target AtkObject is cached https://bugs.webkit.org/show_bug.cgi?id=118567

Ditto.

> Tools/valgrind/memcheck/suppressions.txt:545
> +# 2. intentional unit test errors, or stuff that is somehow a false positive
> +# in our own code, or stuff that is so trivial it's not worth fixing

Bad sentence: Start with capitalized word ("Third") and finish with a period.

> Tools/valgrind/memcheck/suppressions.txt:646
> +   Leak webkitAccessibleNew https://bugs.webkit.org/show_bug.cgi?id=118382

Same comment about consistency on names and URLs

> Tools/valgrind/memcheck/suppressions.txt:655
> +   Leak atk title https://bugs.webkit.org/show_bug.cgi?id=118385

Likewise

> Tools/valgrind/memcheck/suppressions.txt:665
> +   Leak PluginObject in exceptional circumstances https://bugs.webkit.org/show_bug.cgi?id=118528

Likewise

> Tools/valgrind/memcheck/suppressions.txt:675
> +   TextCodecICU::registerCodecs is leaking https://bugs.webkit.org/show_bug.cgi?id=118505

Likewise

[Brian Holt]

Created attachment 208195 [details]
Suppress known leaks

[Brian Holt]

(In reply to comment #2)
> (From update of attachment 208029 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208029&action=review
> 
> Thanks for the patch, Brian. Informal review here:
> 
> First of all, the patch looks good in general, although there are some problems such as the fact that the patch does not seem to be consistent with the fact that this bug depends on bug 118785 (you're adding some code here that is already proposed as part of the latest patch for that other bug, instead of proposing just the incremental diff compared to that one), or that there are some issues related to coding style that would need fixing before landing.

Thanks for the review.  In light of the comments especially about style I've made all comments for leaks start with Uppercase and end with a period. I've also chosen the offending function as the leak name with and optional "(Third party)" or "(Intentional)".

[Brian Holt]

Comment on attachment 208195 [details]
Suppress known leaks

Cancelling review: DRT now times out Valgrind only when using suppressions.  Will submit a new patch that addresses this as well.

[Brian Holt]

Created attachment 208197 [details]
Suppress known leaks

[Dirk Pranke]

Comment on attachment 208197 [details]
Suppress known leaks

View in context: https://bugs.webkit.org/attachment.cgi?id=208197&action=review

I'll let some gtk reviewer review the actual suppressions for this ... the python code looks a bit odd (not sure why you're using default_timeout_ms instead of driver_stop_timeout in either case).

> Tools/Scripts/webkitpy/port/gtk.py:77
> +        return super(GtkPort, self).default_timeout_ms()

Should this be super(...).driver_stop_timeout() ?

[Brian Holt]

Comment on attachment 208197 [details]
Suppress known leaks

View in context: https://bugs.webkit.org/attachment.cgi?id=208197&action=review

>> Tools/Scripts/webkitpy/port/gtk.py:77
>> +        return super(GtkPort, self).default_timeout_ms()
> 
> Should this be super(...).driver_stop_timeout() ?

You're right - it should be.

> Tools/Scripts/webkitpy/port/gtk.py:103
> +            suppressionsfile = self.path_from_webkit_base('Tools', 'Scripts', 'valgrind', 'suppressions.txt')

Is the path for the new suppressions file ok?

[Brian Holt]

Created attachment 208245 [details]
Suppress known leaks v2

[Dirk Pranke]

Comment on attachment 208245 [details]
Suppress known leaks v2

Tools/Scripts/valgrind seems fine to me. Python change also looks good.

[Brian Holt]

Any chance of a review by a Gtk reviewer?

[Brian Holt]

Thanks Martin! Would you mind setting cq+?

[WebKit Commit Bot]

Comment on attachment 208245 [details]
Suppress known leaks v2

Clearing flags on attachment: 208245

Committed r154387: <http://trac.webkit.org/changeset/154387>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.